search

primer / brand

React components and Primitives for GitHub marketing websites
https://primer.style/brand
MIT License
74 stars 33 forks source link

[a11y] `SubNav` accessibility issues #762

Open stamat opened 2 months ago

stamat commented 2 months ago
  1. Focus trap activates even when there is no submenu or popup https://github.com/primer/brand/issues/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! ✨
simurai commented 2 months ago

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