Skip to content

fix aria-hidden on anchor #7460

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 18, 2024
Merged

fix aria-hidden on anchor #7460

merged 3 commits into from
Dec 18, 2024

Conversation

britt6612
Copy link
Collaborator

What does this PR do?

This PR fixes an console log error that was happening within Layer

Blocked aria-hidden on an element because its descendant retained focus. The focus must not be hidden from assistive technology users. Avoid using aria-hidden on a focused element or its ancestor. Consider using the inert attribute instead, which will also prevent focus. For more details, see the aria-hidden section of the WAI-ARIA specification at https://w3c.github.io/aria/#aria-hidden.
Element with focus: a
Ancestor with aria-hidden:  <a tabindex=​"-1" aria-hidden=​"true" class=​"LayerContainer__HiddenAnchor-sc-1srj14c-0 kwDYmk">​</a>​

Where should the reviewer start?

LayerContainer.js

What testing has been done on this PR?

locally check for console log

How should this be manually tested?

storybook

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

 Avoid using aria-hidden on a focused element or its ancestor. Consider using the inert attribute instead, which will also prevent focus

The anchor element is focusable but is being hidden from assistive technologies with aria-hidden="true"

using inert will prevent focus and hide the element from assistive technologies which is what we intend.

What are the relevant issues?

closes #7459

Screenshots (if appropriate)

Do the grommet docs need to be updated?

no

Should this PR be mentioned in the release notes?

yes

Is this change backwards compatible or is it a breaking change?

backwards compatible

@britt6612 britt6612 requested a review from jcfilben December 11, 2024 02:22
jfeferman
jfeferman reviewed Dec 11, 2024
View reviewed changes
src/js/components/DataFilters/__tests__/__snapshots__/DataFilters-test.tsx.snap Outdated
class="c3"
inert=""
Copy link

@jfeferman jfeferman Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the markup could be just <a class="c3" tabindex="-1" inert>

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/inert

Copy link
Collaborator Author

@britt6612 britt6612 Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to do inert="" to get it to work.
More details here: facebook/react#17157

@taysea
Copy link
Collaborator

taysea commented Dec 11, 2024

@britt6612 Where the suggestion says "Consider using the inert attribute instead, which will also prevent focus", is it incorrect HTML to be programmatically placing focus on an inert element? When I interact with this story, the focus isn't moved to the layer container, so I can't get into the layer

Screen.Recording.2024-12-11.at.2.54.04.PM.mov

@britt6612
Copy link
Collaborator Author

Details

@taysea
Im not seeing this locally I wonder if its related to this comment we saw before?

#7324 (comment)

@jfeferman
Copy link

In the referenced story, when the modal opens, the first child <div> of storybook-root — referred to here as the ancestor — is programmatically set to aria-hidden="true".

However, if you set modal prop to "false", this ancestor is not changed and no error is displayed in the console. Is it necessary to set aria-hidden="true" on the ancestor for the modal option to function correctly?

@taysea
Copy link
Collaborator

taysea commented Dec 12, 2024

Details

@taysea Im not seeing this locally I wonder if its related to this comment we saw before?

#7324 (comment)

You're right, it must've been related to that. If I close out the netlify deploy bar prior to interacting, then I don't have the issue.

@britt6612 britt6612 requested a review from taysea December 12, 2024 17:45
@britt6612
Copy link
Collaborator Author

updated PR:

I removed the hiddenAnchor

This is because the hiddenAnchor was initially added for the following reason:

      // once layer is open we set the focus in the hidden
      // anchor so that you can start tabbing inside the layer

Since this was added we have enhanced our FocusedContainer.js to be able to trapFocus within a modal. Instead of the hiddenAnchor we know we have 2 divs that are pre and post of the modal that is open that is there in order to make sure the tab stays within the modal.

jcfilben
jcfilben approved these changes Dec 17, 2024
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jcfilben jcfilben merged commit c685fe5 into grommet:master Dec 18, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halocline halocline halocline approved these changes

@jcfilben jcfilben jcfilben approved these changes

@taysea taysea Awaiting requested review from taysea

+1 more reviewer

@jfeferman jfeferman jfeferman left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accessibility Issue on Layer Component: Focus Retained on Element with aria-hidden
5 participants
@britt6612 @taysea @jfeferman @halocline @jcfilben