Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(cdkTrapFocus): Sets aria-hidden to true #19502

Closed
jrood opened this issue Jun 1, 2020 · 6 comments
Closed

bug(cdkTrapFocus): Sets aria-hidden to true #19502

jrood opened this issue Jun 1, 2020 · 6 comments

Comments

@jrood
Copy link
Contributor

jrood commented Jun 1, 2020

Expected Behavior

cdkTrapFocus should not set aria-hidden to true because hidden area elements are not supposed to have focusable content https://dequeuniversity.com/rules/axe/3.2/aria-hidden-focus?application=AxeChrome but the point of having a focus trap is so that you can use it with focusable content.

Actual Behavior

cdkTrapFocus sets aria-hidden to true
See comments on this commit f66302d

Environment

  • CDK/Material:
@jrood jrood added the needs triage This issue needs to be triaged by the team label Jun 1, 2020
@jelbourn
Copy link
Member

jelbourn commented Jun 1, 2020

This is working as intended. Focus trapping is a special case where this is necessary and a11y linters often report it as a false positive.

@jelbourn jelbourn closed this as completed Jun 1, 2020
jrood referenced this issue Jun 2, 2020
These anchors at the book ends of a FocusTrap'ed element have focus listeners that redirect focus
back to the element. However, some screen readers may access these focus traps without moving
programmatic focus, leaving the SR user wondering why an empty control lives on the page.  Android
TalkBack currently treats this as a stop with no announcement (because it has no content). Adding
the aria-hidden should prevent these from being accessed in SR contexts while preserving the core
functionality of redirecting focus when it's moved linearly (e.g., with tab).
@jrood
Copy link
Contributor Author

jrood commented Jun 2, 2020

Thanks @jelbourn ! I'm hoping to report this as an issue with axe-puppeteer in that case. Do you know what criteria/marker an a11y linter could use to identify this as a special exception (since, in general, the no-focusable elements rule for aria-hidden should still be enforced)?

@jrood
Copy link
Contributor Author

jrood commented Jun 2, 2020

@jelbourn could you also help clarify why focus trapped elements would need to be aria-hidden at all? It seems like it would be confusing that something could be in focus but not visible.

@junyper
Copy link

junyper commented Jun 2, 2020

@jelbourn I think axe is technically correct. To correctly implement focus trapping we need to implement or use the inert attribute polyfill, which would essentially make focusable elements that are hidden not focusable temporarily. Otherwise, by putting aria-hidden on focusable elements you are building a UI that is confusing for screen reader users.

https://github.com/GoogleChrome/inert-polyfill

@jelbourn
Copy link
Member

jelbourn commented Jun 4, 2020

We have the practical equivalent to the inert polyfill tracked in #9035. The current approach definitely isn't perfect, but has plugged along as "good enough" for the most part. IMO linters like axe are great for capturing oversights and mistakes, but in this case we did deliberately use this pattern and tested with screen-readers we support.

@mmalerba mmalerba removed the needs triage This issue needs to be triaged by the team label Jun 5, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants