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

fix(modal): place aria-hidden on sentinel nodes #15996

Closed
wants to merge 4 commits into from

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Mar 18, 2024

Testing some feedback from the Carbon Accessibility Guild to place aria-hidden on the sentinel nodes to ensure they're not read by screenreaders when using virtual cursor mode.

If this is successful, We've confirmed this does fix the issue, so we should port it to all other components that use sentinel nodes, which should fix #14203

  • Modal
  • ComposedModal
  • Notification
  • FloatingMenu (used by the legacy non-menu-based OverflowMenu)

You can test this change on the modal story of the deploy preview.

Copy link

netlify bot commented Mar 18, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit a7a4443
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/65fc9a6aac638e00083c66bf
😎 Deploy Preview https://deploy-preview-15996--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tay1orjones tay1orjones changed the title fix(composedmodal): place aria-hidden on sentinel nodes fix(modal): place aria-hidden on sentinel nodes Mar 18, 2024
@tay1orjones tay1orjones self-assigned this Mar 20, 2024
@tay1orjones tay1orjones marked this pull request as ready for review March 20, 2024 18:22
@tay1orjones tay1orjones requested a review from a team as a code owner March 20, 2024 18:22
@tw15egan
Copy link
Collaborator

@tay1orjones looks like tests may need to be updated?

@tay1orjones
Copy link
Member Author

tay1orjones commented Mar 22, 2024

@tw15egan I think I'm remembering now why we didn't do this before ... with the aria-hidden on there the checker reports a violation:

Element "button" should not be focusable within the subtree of an element with an 'aria-hidden' attribute with value 'true'

While this approach might be allowable in our view, we can't ship something that's going to pop violations in consumer environments. Eventually the checker may be updated to handle this use case or provide an exception but for now I think we're blocked on this one.

For now, the best way around this is via

@tay1orjones tay1orjones deleted the aria-hide-sentinels branch August 16, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[a11y]: Modal [all variants] - identifies visually hidden controls
4 participants