fix(icon): fix aria roles and attributes #10024

Open
wants to merge 1 commit into
from
@clshortfuse
Collaborator
clshortfuse commented Nov 16, 2016 edited

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

@clshortfuse clshortfuse added this to the 1.1.2 milestone Nov 16, 2016
@googlebot
Collaborator

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
Collaborator

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
Collaborator

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

@nbtorcol
@googlebot
Collaborator

CLAs look good, thanks!

@googlebot
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Nov 16, 2016
@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
Collaborator
clshortfuse commented Nov 21, 2016 edited

@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
@ThomasBurleson
Contributor

@clshortfuse - status update plz ?

@clshortfuse
Collaborator

@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
nbtorcol commented Dec 9, 2016
@ThomasBurleson
Contributor

@clshortfuse - let's finish this PR asap or close it.

@clshortfuse
Collaborator

@jelbourn Can you review this? The solution is ready to merge with the current whitelist solution. Blacklist could be more future-proof, but not entirely needed.

@nbtorcol
nbtorcol commented Jan 4, 2017
@ThomasBurleson ThomasBurleson modified the milestone: 1.1.3, 1.1.2 Jan 4, 2017
@nbtorcol
nbtorcol commented Jan 4, 2017
@clshortfuse
Collaborator

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

@nbtorcol
nbtorcol commented Jan 4, 2017
@clshortfuse
Collaborator

@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
nbtorcol commented Jan 4, 2017
@clshortfuse
Collaborator

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

@nbtorcol
nbtorcol commented Jan 5, 2017
src/components/icon/js/iconDirective.js
+ return false;
+ }
+ /* Perform role blacklist check */
+ if (domElement.attributes.role) {
@ThomasBurleson
ThomasBurleson Jan 5, 2017 edited Contributor

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

@clshortfuse
Collaborator

@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
Contributor

LGTM

@clshortfuse clshortfuse fix(icon): fix aria roles and attributes
mdIcon was improperly assuming its parent's text content would be
announced didn't apply ARIA roles properly

icon component:

* 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 has ARIA level (2 levels deep)
* Fallback to icon name as an aria label
* Apply `aria-hidden` attr instead of `aria-hidden` when all checks fail
* Add spec tests
* Remove improper spec test based on text content

aria service:

* Add hasAriaLabel func to check for valid aria label
* Add parentHasAriaLabel func to check parents for valid aria with role and tagName blacklists
* Add spec tests

Fixes #9629
1b1329c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment