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

[a11y] SubNav accessibility issues #762

Open
stamat opened this issue Sep 19, 2024 · 1 comment
Open

[a11y] SubNav accessibility issues #762

stamat opened this issue Sep 19, 2024 · 1 comment
Labels
brand bug Something isn't working

Comments

@stamat
Copy link
Contributor

stamat commented Sep 19, 2024

  1. Focus trap activates even when there is no submenu or popup 🐛 [BUG] - SubNav focus trap issue #761
  2. On mobile, once you enter into a focus trap in a submenu, there is no way to reach the close button if you are using your keyboard. The only way to exit the focus trap is by pressing Esc. Fixing the focus trap issue and rethinking the layout will provide a solution for this. I think focus trap needs to include the item to close the menu.
  3. aria-current="page" should not be a link if it has a submenu it should be a button, if it doesn't have a submenu, if it doesn't have a submenu it can remain a link since it has the aria-current="page". Submenu toggles have a button that activates them which is separate from the link. I think it should all be one button. We should basically discourage the usage of # value for the href.
  4. Submenus don't have their own focus trap and you cannot exit them by pressing Esc.
  5. Mobile dropdown menu toggle button maybe needs an aria-haspopup="menu"? Maybe also aria-controls="ID" if we perform the restructure.
  6. Mobile menu pushes the content down on tablet sizes when toggled on, but then on mobile the whole SubNav becomes absolutely positioned - which is an inconsistent behavior. I suggest that the nav items only are always absolutely positioned on tablet/mobile sizes and the SubNav containing element is positioned relatively. We can always change it's positioning in custom scenarios.
  7. We can maybe have the ul element or nav labeled by the nav heading with aria-labeledby="ID"? Or maybe this is automatically understood since the heading is inside the nav element? We should check how the screen reader reads this. This might be useful if we restructure the SubNav a bit to accommodate for other issues.
  8. Not entirely an a11y issue, but can we have an option for the heading to choose the element type like with an optional attribute as like we have on some other components. Could be useful! ✨
@stamat stamat added the bug Something isn't working label Sep 19, 2024
@simurai
Copy link
Contributor

simurai commented Sep 20, 2024

@stamat 7. We can maybe have the ul element or nav labeled by the nav heading with aria-labeledby="ID"? Or maybe this is automatically understood since the heading is inside the nav element? We should check how the screen reader reads this. This might be useful if we restructure the SubNav a bit to accommodate for other issues.

I ran into something related in this PR.

When trying to add an aria-label, it didn't seem to work:

Image

But is needed to pass the axe check.

I guess we could use aria-labeledby and use the <SubNav.Heading> to label the nav, but in my case, we would like to rather not show a <SubNav.Heading>. Maybe it's ok to use "screen reader only" styles to visually hide it, e.g. <SubNav.Heading className="sr-only>, if we want to not allow directly adding an aria-label.

@rezrah rezrah added the brand label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brand bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants