[Menubar] Adds keyboard behaviors to the menubar, improves accessibility

[tespin]

Fixes #2933

This PR focuses on adding proper keyboard behaviors to the recently refactored Menubar.

Update

The Menubar component now has keyboard navigation and enhanced accessibility support. There are some other improvements to make which are listed at the end, but we can work on them in future PRs seeing as this one has gotten rather large.

Before:
Navigation happened purely through Tab / Shift + Tab. Escape functionality worked but conventional Menubar behaviors were not accessible. Focus management was not connected to state.

old_menubar_2.mov

After:
Menubar behaviors from the WAI ARIA authoring guide implemented. Tab behavior works as expected and initial focus styles are available.

menubar_after.mov

Key Changes

• Implemented roving tab index in Menubar.jsx for focus management
• Updated aria roles, such as aria-orientation='horizontal'on Menubar and aria-labels where applicable
• Navigation support, including:
  • Horizontal and vertical navigation
  • Home/end, enter/space support
• Improved HTML structure
  • wrapped Menubar in nav element
  • moved from div-based menubar to ul/li
  • the Logo is moved outside of the nav
  • left and right menu items now wrapped in ul elements with role=group
• added SubmenuContext for managing submenu state
• MenubarSubmenu supports listbox aria roles, such as with the options in LanguageMenu
• basic tests for checking component renders, keyboard behavior, and aria roles
• added keyboard styles
• added basic JSDoc comments for components
• MobileNav uses a simplified version of a menuitem
• menuitems which used to be hidden are now visible in submenus but disabled until their conditions are met. disabled menuitems remain focusable per wcag guidelines.

To Do

• testing:
  • improve Menubar tests
  • test with screen readers
  • verify aria structure and role handling
  • test functionality on mobile
• errors:
  • focus is not properly entering certain modals
  • some timing issues with useEffects and blur/focus handlers
  • editor component rerenders when LanguageMenu is mounted
• optimizations:
  • consider refactoring context structure
  • memoize index and menuItem calculations
  • menuItems and menuItemToId could probably be combined into a better data structure
  • consolidate the more complex state updates with useReducer
• other features/enhancements:
  • add support for matching focused menu item to pressed character key
  • improve focus styles
  • explore options for handling simultaneous styles when hovering over a menuitem and navigating via keyboard
  • tweak hover styles on the "Back to Editor" link (documented below)
  • extract some of the more common logic such as navigation into hooks
  • extract MenubarSubmenu components into separate files
  • extend certain Menubar components to work on mobile (such as MenubarItem)
  • improve JSDocs

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.
• is descriptively named and links to an issue number, i.e. Fixes #123

[tespin]

• Extract logo to header level for better semantic structure
• Ensure arrow keys only cycle through menu items, excluding the logo

[dipamsen]

When navigating the SubMenus, I noticed that both the mouse hover and arrow key inputs are visible, which I tried to depict in the video. I feel like this could get a bit confusing, but unsure if it might've been intentional!

Here's one way it could work: https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/

It only ever highlights a single item at a time, which follows the keyboard navigation, but jumps to a different element on mouse hover. Maybe this is not very easy to implement and requires additional logic.

Another option could be to have different styles on hover and focus, since they have different semantics (eg. pressing enter will select the focused option, not the hovered one)

[tespin]

@dipamsen thanks for linking the example! that's been the model for this menubar's implementation and is aligned with other examples i've seen -- i'll be using the same approach.

@raclim spent a little more time with the navigation issue. i think the problem is in the menuitems which get conditionally rendered; there seems to be a conflict between when the menuitems are registered and when the conditions for hideIf change. hideIf tends to change after the component mounts, so when the submenu item eventually registers, it gets added to the end of the submenuItems set. even if they are rendered a different way they still get navigated to as if they were the last items. e.x. if a user signs in, saves the sketch, then refreshes the page, the items which depend on isUnsaved (Duplicate, Share, etc) are added at the end of the navigation order.

i'm curious how you'd feel if, instead of conditionally rendering the items, maybe keeping them visible but disabled instead? that way the ordering of the menuitems can be stable regardless of visibility. i know that the example menubars we looked at above do something similar as well, depending on the context, so it looks like an established pattern. it will probably be an adjustment though, so i just wanted to check in, in case there was an argument for keeping the conditional rendering.

[tespin]

here's how things look with the disabled but visible approach.

unsaved, unauthenticated

unsaved, authenticated

saved, authenticated

i used some existing inactive styles. if we go this route we should probably push it more since it's still hard to tell if an item is disabled or not. navigation works as expected too. we can add tooltips or modals to clarify why an item is disabled.

[raclim]

spent a little more time with the navigation issue. i think the problem is in the menuitems which get conditionally rendered; there seems to be a conflict between when the menuitems are registered and when the conditions for hideIf change. hideIf tends to change after the component mounts, so when the submenu item eventually registers, it gets added to the end of the submenuItems set. even if they are rendered a different way they still get navigated to as if they were the last items. e.x. if a user signs in, saves the sketch, then refreshes the page, the items which depend on isUnsaved (Duplicate, Share, etc) are added at the end of the navigation order.

Thanks so much for catching this—that explains a lot! It makes so much sense when I interact with the menubar now!!! 😂

i'm curious how you'd feel if, instead of conditionally rendering the items, maybe keeping them visible but disabled instead? that way the ordering of the menuitems can be stable regardless of visibility. i know that the example menubars we looked at above do something similar as well, depending on the context, so it looks like an established pattern. it will probably be an adjustment though, so i just wanted to check in, in case there was an argument for keeping the conditional rendering.

Hmm, yeah since disabling the menuitems seems like an established pattern, I'm totally on board with going that route here! I think the conditional rendering was originally used because the component was first built using toggleDropdown to control when the menu items would render.

i used some existing inactive styles. if we go this route we should probably push it more since it's still hard to tell if an item is disabled or not. navigation works as expected too. we can add tooltips or modals to clarify why an item is disabled.

Thanks so much for linking the screenshots and confirming that it seems to work! I'm wondering do you think it makes sense to break this out into a separate PR, or is it better to keep it within this one?

[tespin]

Thanks so much for linking the screenshots and confirming that it seems to work! I'm wondering do you think it makes sense to break this out into a separate PR, or is it better to keep it within this one?

i thought i'd want to break it out, but i think it makes more sense to keep in the current PR since it feels necessary for the component to work. i'll keep the changes small we can open up another PR in the future in case we need to make improvements.

[tespin]

@raclim these last changes should put the menubar in better condition! all the menuitems are now visible but ones that require certain conditions to be met are disabled. disabled options are still focusable but cannot be activated as described in the menubar pattern and this guide on keyboard interfaces.

i had some state updating issues which are hopefully resolved now but will probably need to be monitored. i didn't address the simultaneous mouse hover / key inputs being visible since that doesn't necessarily impact the component on a functional level; it also appears like there aren't any specific wcag guidelines so we can maybe discuss what feels ideal for the editor environment (the menubar example does show both indicators simultaneously though).

otherwise would love for you to check this out when you can! there's a little more clean up left to do and then i just want to finish documenting any other changes / improvements that can be made in future PRs.

[raclim]

all the menuitems are now visible but ones that require certain conditions to be met are disabled. disabled options are still focusable but cannot be activated as described in the menubar pattern and this guide on keyboard interfaces.

i had some state updating issues which are hopefully resolved now but will probably need to be monitored. i didn't address the simultaneous mouse hover / key inputs being visible since that doesn't necessarily impact the component on a functional level; it also appears like there aren't any specific wcag guidelines so we can maybe discuss what feels ideal for the editor environment (the menubar example does show both indicators simultaneously though).

Thanks so much for touching this up, I think this is really awesome and feels much smoother to me!

I think the only minor tweaks that I'm seeing are styling related, mostly regarding the background when a user hovers over the "Back to Editor" link (pictured below), which I think appears a bit cut off! I'm also wondering this extends to the Upper-right Hand Dropdown menu as well (aslo pictured below), but I think it could also be fine as is!

"Back to Editor" on Hover

Upper-right Hand Dropdown Menu on Hover

Besides this, I feel overall that it's in a pretty good place so far! Thanks so much again for continuing to work on this and bringing some really exciting changes to the navigation!!

[tespin]

ah yeah that is a bit awkward. i'll add to the list for future tasks -- thanks for spotting it!

doing a bit more clean up and i'll ping again when everything feels ready.

[tespin]

@raclim did another pass to clean up what i could ... also, the initial comment here has been updated with recent changes and possible tasks for the future. there are things i may not have caught, but hopefully they'll surface when the changes roll out and we can address them then.

let me know what you think!

[raclim]

Thanks so much for the updates on this!!

I think this overall looks great to me! I think we can probably roll this out if it feels good to you as well!