-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Menubar] Adds keyboard behaviors to the menubar, improves accessibility #3320
base: develop
Are you sure you want to change the base?
[Menubar] Adds keyboard behaviors to the menubar, improves accessibility #3320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Extract logo to header level for better semantic structure
- Ensure arrow keys only cycle through menu items, excluding the logo
…w on an unopened submenu
…d item if within submenu
@@ -200,7 +205,20 @@ function Menubar({ children, className }) { | |||
|
|||
return ( | |||
<MenubarContext.Provider value={contextValue}> | |||
<div className={className} ref={nodeRef} onFocus={handleFocus}> | |||
<div |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues:
- originally we were using
useKeyDownHandlers
to handle theMenubar
keyboard events. This hook captured events globally, which conflicted with keyboard behaviors for the rest of the page, including the editor itself.
Changes:
- Added
e.stopPropagation()
to all handlers - Attached handlers directly to the
Menubar
container withrole='menubar'
,aria-orientation='horizontal'
, andtabIndex='0'
- removed use of
useKeyDownHandlers
- added checks for menu and focus state
New issues:
role='menubar'
is now in two places: within the div inMenubar.jsx
and in aul
withinNav.jsx
These changes should make it so that Menubar
keyboard events are only handled when the actual component or related elements are focused, while preserving the keyboard behaviors for the rest of the page. Instead of using the useKeyDownHandlers
hook, events are now handled directly by the Menubar
element with checks for focus and menu state.
We might need to take a second look at the way we compose the Menubar
component in Nav.jsx
to resolve the new issues.
const [activeIndex, setActiveIndex] = useState(0); | ||
const prevIndex = usePrevious(activeIndex); | ||
|
||
// old state variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version of the Menubar
did not keep track of its menu items. However, we needed a way to manage this collection to implement keyboard navigation. Arrays were useful for basic keyboard features but sets allow for more predictable registration / cleanup operations and guarantee uniqueness.
Changes:
- array-based management of state renamed to maintain functionality during migration
- added new state variables for managing collections using sets
Keyboard navigation is eventually reimplemented with the new set data structure. The old state variables are cleaned up in a future commit.
Adding a video here to see the changes so far --- I'm still testing focus management and evaluating state variables against certain race conditions, but most keyboard behaviors have been implemented. Will slowly work on adding tests edit: since this pr is getting long, I'll open a new one to add the tests menubar_test.mov |
Progresses #2933
Just setting this up to track next steps for the component! This PR focuses on adding proper keyboard behaviors to the recently refactored Menubar. Currently in progress
from wai aria menubar pattern
Keyboard Interactions
Tab
andShift + Tab
menubar
:menubar
for the first time, focus is set on the firstmenuitem
.menubar
has previously contained focus, focus is optionally set on themenuitem
that last had focus. Otherwise, it is set on the firstmenuitem
that is not disabled.menuitem
in amenu
ormenubar
, move focus out of the menu ormenubar
, and close all menus and submenus.Enter
menuitem
that has a submenu, opens the submenu and places focus on its first item.Space
menuitem
that has a submenu, opens the submenu and places focus on its first item.menuitem
that does not have a submenu, activates themenuitem
and closes the menu.Down Arrow
menuitem
in amenubar
, and thatmenuitem
has a submenu, opens the submenu and places focus on the first item in the submenu.Up Arrow
menuitem
in amenubar
, and thatmenuitem
has a submenu, opens the submenu and places focus on the last item in the submenu.Right Arrow
menubar
, moves focus to the next item, optionally wrapping from the last to the first.- [ ] When focus is in a menu and on amenuitem
that has a submenu, opens the submenu and places focus on its first item.menubar
.menuitem
with a submenu, either: (Recommended) opens the submenu of thatmenuitem
without moving focus into the submenu, or opens the submenu of thatmenuitem
and places focus on the first item in the submenu.Left Arrow
menubar
, moves focus to the previous item, optionally wrapping from the first to the last.- [ ] When focus is in a submenu of an item in a menu, closes the submenu and returns focus to the parentmenuitem
menubar
, performs the following 3 actions:menubar
.menuitem
with a submenu, either: (Recommended) opens the submenu of thatmenuitem
without moving focus into the submenu, or opens the submenu of thatmenuitem
and places focus on the first item in the submenu.Home
menubar
.End
menubar
.Escape
menuitem
, from which the menu was opened.Mouse Interactions
menuitem
with a submenu opens the submenu and focuses thatmenuitem
.menuitem
within a submenu activates that item and closes the submenu.menuitem
with a submenu closes that submenu.menubar
closes all submenus.JS Docs
edit: this pr is getting rather large, I'm going to move the tests and other enhancements into a separate PR.
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123