fix(icon): fix aria roles and attributes

[clshortfuse]

mdIcon was improperly assuming its parent's text content would be announced didn't apply ARIA roles properly

• Apply img role by default
• Don't change ARIA if it's already valid
• Prioritize alt attribute for aria-label
• Apply aria-hidden attr if parent or parent's parent has ARIA label and is not part of role or tagName whitelist
• Fallback to icon name as an aria label
• Apply aria-hidden attr instead of aria-hidden when all checks fail
• Add spec tests to ensure new functionality
• Remove improper spec test based on text content

Fixes #9629

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[clshortfuse]

@ThomasBurleson @nbtorcol I haven't built the unit tests yet since I want ensure this is the proper logic before I do.

[nbtorcol]

Let me get back to you tomorrow.

[googlebot]

CLAs look good, thanks!

[googlebot]

CLAs look good, thanks!

[nbtorcol]

This is better, but it is still not correct. I will try to explain more
what is wrong.

• Apply img role by default -- yes

• Don't change ARIA if it's already valid -- yes

• Prioritize alt attribute for aria-label -- yes

• Apply presentation role if parent or parent's parent has ARIA label –
no, see note 1 below

• Fallback to icon name as an aria label -- yes

• Apply presentation role instead of aria-hidden when all checks fail –
no, see note 2 below

1.   First and foremost, the original—aria-hidden—was better. If it is
   

   not of significance to screen readers, then just make it go away
   completely; don’t turn it into something else. Second, as I mentioned
   before, this logic is flawed. Yes, the ARIA spec says that you can put
   aria-label/aria-labelledby/aria-describedby on any element, regardless of
   tagName or role, but that is not true for some of the Windows screen
   readers, or more correctly the browsers that provide the information to the
   screen readers. Please consider reading this article:
   http://www.ssbbartgroup.com/blog/how-browsers-interact-with-screen-readers-and-where-aria-fits-in-the-mix/.
   There are several ARIA roles that support these ARIA attributes, but, for
   this situation, the best criteria would be to only use the ARIA roles that
   are controls. I sent example code to this effect, and I do not understand
   why it was not even used as a starting point. If this turns out to be to
   complex, then I would recommend just not trying to do this sort of analysis
   and making sure that aria-hidden works, so that people can hide the icons
   on their own if they think it necessary.

2.   As above, aria-hidden would be better.
   

[clshortfuse]

@nbtorcol I'm sorry I didn't use the code your provided (on the issue page), but that was because it was private link on a Google Drive when I started writing the PR (for future reference, something like pastebin.com would serve better). The use of presentation=role instead of aria-hidden is from research that it is more reliable than aria-hidden (especially with IE and JAWS). After more research, it seems it's better to use both (and even possible tabindex=-1). See: http://john.foliot.ca/aria-hidden/

I believe the reason why we want to use role=presentation is because the spec says:

Skip hidden elements unless the author specifies to use them via an aria-labelledby or aria-describedby being used in the current computation. By default, users of assistive technologies won't receive the hidden information, but an author will be able to explicitly override that and include the hidden text alternative as part of the label string sent to the accessibility API.

Which, to my understanding, means aria-labelledby overrides aria-hidden. This is what brings me to the conclusion it's best to use both.

If I understand correctly, you suggest we should only perform a parent aria check based on a whitelist of tagNames and/or or ARIA. It would reduce the specificity of our upwards search, but I have no problem implementing it.

As for the third point of respecting aria-hidden, I have added a check for the mdIcon itself, but I never checked the parent. I will add this to the code.

Edit: Originally my research of aria-label lead me to examples in the ARIA spec related to how to handle labels and images here: https://www.w3.org/TR/wai-aria/roles#presentation and https://www.w3.org/TR/wai-aria/roles#textalternativecomputation

[nbtorcol]

@nbtorcol <https://github.com/nbtorcol> I'm sorry I didn't use the code your provided (on the issue page), but that was because it was private link on a Google Drive when I started writing the PR (for future reference, something like pastebin.com would serve better). Apologies, that was partially an oversight on my part. I will look at that website, but what I use is driven almost entirely by the level of accessibility. Given that I am doing this as a side project at work, I don’t have a lot of time to learn new stuff. The use of presentation=role instead of aria-hidden is from research that it is more reliable than aria-hidden (especially with IE and JAWS). After more research, it seems I can safely use both (and even possible tabindex=-1 ). The tabIndex attribute is only relevant for controls and should not be used with icons. See: http://john.foliot.ca/aria-hidden/ That article is four years old; both the screen readers and browsers have changed a lot in that time. Please base your conclusions off of more recent articles or conduct your own experiments; NVDA is free, and JAWS has a demo. I can help test if needed and/or point you to relevant articles and information from people. If I understand correctly, you suggest we should only perform a parent aria check based on a whitelist of tagNames and/or or ARIA. It would reduce the specificity of our upwards search. Yes. As for the third point of respecting `aria-hidden, I have added a check for the mdIcon itself, but I never checked the parent. I will add this to the code. Not sure what you mean here; sorry if my prior comments were confusing.

[ThomasBurleson]

@clshortfuse - status update plz ?

[clshortfuse]

@nbtorcol I've added the changed we've discussed. I removed role=presentation since that from an old version of NVDA that didn't support aria-hidden.

I'm a little confused about the whitelisting type attribute, since as far as I could tell, we'd pretty much be performing HTML validation for valid inputs, since we're are listing every possible type value.

I feel a blacklist would work better, or else our code will eventually become incompatible when newer versions of the HTML spec and when new ARIA roles are created.

[nbtorcol]

I will get the blacklist to you today. I was gone on vacation for a while and then had a lot to do when I got back.

[nbtorcol]

Do you still have access to the Google doc that I made before? I updated the code there.

[clshortfuse]

@nbtorcol Yes, I do have access. I'm updating the PR now. Thank you!

[nbtorcol]

Sounds good. Please let me know if you have any questions.

[clshortfuse]

@nbtorcol I just want to confirm a couple of concerns

• ARIA ROLES menu and scrollbar are now being blacklisted, but were whistlisted before
• INPUT TYPES number and time are now being blacklisted, but were whistlisted before
• HTML TAGS menu and time are now being blacklisted, but were whitelisted before
• I couldn't find where to put tu, since it's neither a role, type, or tag.

Perhaps we should

• return false if aria role blacklist fails
• return true if any input type is used
• return false if html tag blacklist fails

Also, I would need some clarification about hwo to use the shared role and tag names.

[nbtorcol]

Oops, number was supposed to not be part of the blacklist. Sorry. Time, as an HTML tag fails. Some of the ARIA roles that were included but now blacklisted are because I ran an actual test with screen readers, and those are the results. - return false if aria role blacklist fails - return true if any input type is used - return false if html tag blacklist fails Sounds good. In that case, per my code, you can just drop retrieving the type attribute.

[clshortfuse]

@nbtorcol @ThomasBurleson All changes have been made and we are ready for review.

[nbtorcol]

Do you mean committed?

[ThomasBurleson]

@clshortfuse - Please move this to the aria utils. This complexity should not be in the iconDirective.

[clshortfuse]

@nbtorcol Generally, we ask another team member to check our work before we commit directly to our master branch

@ThomasBurleson I made the changes, as requested. Both functions are now accessible by any component via the $mdAria service. Also, the number of parents searches are based on a passed variable.

[ThomasBurleson]

LGTM