Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

[a11y] aria-hidden/aria-label on <md-icon> inside of button is ignored with ligature icon #7376

Closed
calebegg opened this issue Mar 2, 2016 · 11 comments
Assignees
Labels
a11y This issue is related to accessibility P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: merge ready This PR is ready for a caretaker to review type: bug

Comments

@calebegg
Copy link
Member

calebegg commented Mar 2, 2016

See demo:
http://codepen.io/anon/pen/mPJOMX?editors=1000

I want to hide the gear icon entirely from screen readers, as it doesn't add anything. But aria-hidden="true" and aria-label="" don't work. The only thing that works is a non-empty, non-whitespace aria-label. But I don't want the icon to have any label; I just want it ignored, and for the label for the button to just be "Options"

@topherfangio
Copy link
Contributor

To clarify: the button's aria-label is currently

settings

Options

when it should just be Options.

@topherfangio topherfangio added type: bug P1: urgent Urgent issues that should be addressed in the next minor or patch release. labels Mar 24, 2016
@devversion devversion added the a11y This issue is related to accessibility label Mar 25, 2016
@devversion
Copy link
Member

@calebegg @topherfangio I know what you mean, but it is quite difficult to walk through all children and concat the right strings together.

Do you have any other thoughts / or suggestions, instead of iterating through all child elements?

@topherfangio
Copy link
Contributor

@devversion Where is this happening? I'm having trouble tracking down the code.

My first thought would simply be: if the element has aria-hidden, don't concat it's text.

@calebegg
Copy link
Member Author

calebegg commented Apr 4, 2016

Why do we need to do this generated aria-label thing at all? Do screenreaders not just read the children of the button by default? Chromevox seems to.

I feel like even if they don't, this is enough of an edge case that it's best to just require users of the library to put their own aria-label on there if they want something other than the default screenreader behavior. Trying to walk the children to create our own label seems fraught.

@topherfangio
Copy link
Contributor

@calebegg Indeed; it looks like you can just set aria-label="Options" on the button itself and it will work. I'm not 100% sure we are auto-adding the label as I couldn't find the code that does that.

Perhaps this just needs some docs in the icon/button pages to help guide users?

@calebegg
Copy link
Member Author

calebegg commented Apr 4, 2016

I also can't find it, but it has to be somewhere -- how else would the aria-label on my example get added? I did find this test for it:

expect(button.attr('aria-label')).toBe("Hello");

But github's search is awful, so it's hard to find anything.

I could set the aria-label manually, but my point is that I shouldn't have to override some crummy behavior from the system generating the aria-label for buttons.

@devversion
Copy link
Member

@topherfangio I'm not sure, how the browser is building the innerText variable, but maybe it's also a simple iteration on the native layer.

When we filter for aria-hidden, then we need to implement a custom innerText function, which iterates through all childs (even subchilds). I don't know how the performance will decrease.

@topherfangio
Copy link
Contributor

Finally tracked it down; this is happening in the getText(element) function in our $mdAria service: https://github.com/angular/material/blob/master/src/core/services/aria/aria.js#L64

It simply uses the jQuery/jQlite version of .text() which in turn looks at the node.textContent.

@devversion I agree that iterating over all children may cause a significant performance penalty, so it may be something we wish to avoid.

After a bit of thought, the only semi-sane idea I've come up with is to have the icon watch the aria-hidden property and apply some sort of a class/attribute like _md-aria-hidden-text that the $mdAria.getText() function could check for and either:

  1. Search/replace the result of elem.getText() to subtract the unwanted text (a very hacky/error-prone solution), or
  2. Iterate over all sub-nodes (possibly using elem.querySelectorAll('*:not(._md-aria-hidden-text)')) and concatenate their text

Either way, the extra functionality could only be run if we detect the special class so that the performance hit is reduced to this particular instance.

Thoughts?

@devversion
Copy link
Member

@topherfangio I like the second approach. It may be much faster than a plain iteration and also gives us a very easy way to handle a11y texts.

@topherfangio
Copy link
Contributor

@devversion Cool; you want to give it a go and see if it works like we think? 😄

@devversion
Copy link
Member

@topherfangio Yes definitely, will do that and let you know 😄

devversion added a commit to devversion/material that referenced this issue Apr 8, 2016
* $mdAria is currently just using `textContent` for retrieving the `aria-label`.
  This is not valid, since some child-texts can be hidden in aria (`aria-hidden="true")

* Using a TreeWalker is super elegant and performant.
  The current use of the TreeWalker is supported by all browser we support.

Fixes angular#7376
@devversion devversion added the pr: merge ready This PR is ready for a caretaker to review label Apr 8, 2016
devversion added a commit to devversion/material that referenced this issue Apr 9, 2016
* $mdAria is currently just using `textContent` for retrieving the `aria-label`.
  This is not valid, since some child-texts can be hidden in aria (`aria-hidden="true")

* Using a TreeWalker is super elegant and performant.
  The current use of the TreeWalker is supported by all browser we support.

Fixes angular#7376
ilovett pushed a commit to ilovett/material that referenced this issue Apr 22, 2016
* $mdAria is currently just using `textContent` for retrieving the `aria-label`.
  This is not valid, since some child-texts can be hidden in aria (`aria-hidden="true")

* Using a TreeWalker is super elegant and performant.
  The current use of the TreeWalker is supported by all browser we support.

Fixes angular#7376

Closes angular#7957
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y This issue is related to accessibility P1: urgent Urgent issues that should be addressed in the next minor or patch release. pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Development

No branches or pull requests

4 participants