[DropdownMenu] Add aria-labelledby to follow spec
#692
Conversation
a7e12ce to
cb3ad1d
Compare
| @@ -41,7 +41,7 @@ const SUB_CLOSE_KEYS: Record<Direction, string[]> = { | |||
|
|
|||
| const MENU_NAME = 'Menu'; | |||
|
|
|||
| type MenuContextValue = { | |||
| type MenuRootContextValue = { | |||
| isSubmenu: false; | |||
There was a problem hiding this comment.
Would there be any value in updating these to isRootMenu instead for consistency?
One advantage I can think of, which I noted a few times whilst reviewing the PRs but never commented on is that in ternaries root would come before sub which to me is more readable or makes more sense to my brain 😀
But again at the time before I didn't think that was enough of a reason to comment on it or ask for a change.
Perhaps now it is?
There was a problem hiding this comment.
Would there be any value in updating these to
isRootMenuinstead for consistency?
I don't think so. The intent is different here. In DropdownMenu we need to know if it is the root menu to do root menu specific things related to the new trigger part.
In Menu we want to know if they are in a submenu in order to render the submenu content part. I don't really think we need to be consistent, they're different requirements.
We're probably bikeshedding a bit but happy to change it if we think the consistency is preferred.
There was a problem hiding this comment.
Fair enough, I don't think it matters much what you test for as in the end it's the same result but I can see why we'd want to keep it closer to what we're catering for. Looking at it, it would reverse some ternaries in the way I mentioned but more of them would then be the wrong way, so you're probably right 😀
There was a problem hiding this comment.
If you would prefer root parts to render before sub parts in the ternary then I could always do !context.isSubmenu? 😏
* [Menu] Add submenu primitives (#627) Co-authored-by: Jenna Smith <[email protected]> * [Menu] Provide predictable typeahead scoping (#662) * Scope typeahead via collection * Exclude only on typeahead space key * Simplify typeahead * Prevent continuous `updateSearch` call * Inline `onExclusiveKeyDown` functionality * Clear search buffer when leaving * Move `MenuSubTrigger` code below `MenuContent` * `isTypingAhead` conditional clarity * [ContextMenu][DropdownMenu] Integrate new submenu parts (#629) * [ContextMenu][DropdownMenu] Add `TriggerItem` part (#683) * [ContextMenu][DropdownMenu] Don't close on sub trigger pointer down (#684) Co-authored-by: Jenna Smith <[email protected]> * [ContextMenu][DropdownMenu] Add pointer grace for navigation to sub-menus (#688) * [ContextMenu][DropdownMenu] delay sub trigger open via pointer * Add pointer area grace * Use clientX/Y rather than pageX/Y because getBoundingClientRect doesn't include scroll * Update DropdownMenu.stories.tsx * feedback * onItemEnter * Update Menu.tsx * more explicit open timer ref * Remove content rect caching * Move MenuItemImpl down (#689) * [DropdownMenu] Add `aria-labelledby` to follow spec (#692) * Add `aria-labelledby` * Follow `aria-expanded` spec for submenu triggers * Updates based on feedback * Rename root context type * [ContextMenu] Bring inline with `DropdownMenu` changes (#693) * Remove `ArrowUp` to open (#702) * [DropdownMenu] Enable `onEntryFocus` for root menu (#694) * Focus first item when all content types open for keyboard users * Initial feedback addressed * Add `pointerdown` to keyboard user logic * Remove capture * Re-apply capture events with comment * [ContextMenu][DropdownMenu] Improve pointer grace (#691) * Share grace timer ref in content * Align pointer behaviour closer with native * Simplify base story for easier test * Remove export of private func * Tidy from feedback * Minor naming adjustment * Apply `useCallback` Co-authored-by: Andy Hook <[email protected]> * [ContextMenu][DropdownMenu] Fix menu focus after pointer interaction (#707) * Fix menu focus after pointer interaction * Add explainer * [Menu] Remove `onPointerDownOutside` on sub (#708) * [ContextMenu][DropdownMenu] Improve touch/mouse handling separation (#709) * [ContextMenu][DropdownMenu] Improve touch/mouse handling separation * Feedback * [DropdownMenu][ContextMenu] Unwind all nested menus with ESC * [Menu] Tidy `handleSelect` binding * [Menu] Remove callbackRef in handleSelect * [Menu] onSelect optional chain Co-authored-by: Jenna Smith <[email protected]> Co-authored-by: Benoît Grélard <[email protected]> Co-authored-by: Jenna Smith <[email protected]>
This is the first PR of a few that I will be raising with various accessibility improvements.
Adds
aria-labelledbytoMenuSubContentand the rootDropdownMenuContent. Annoyingly the label isn't being read aloud in NVDA but I will look into this separately.Creates a new
DropdownMenuRootpart because there are a bunch of attributes on the root content that are not required in the sub dropdown menus. This was also needed because the rootaria-labelledbywould override the submenuaria-labelledbyinMenuSubContentif this attribute was added to both types within theDropdownMenucomposition which resulted in incorrect ids for submenus (they would be pointing at theDropdownMenuTriggerid).Updates
aria-expandedfor sub menu triggers to follow WAI-ARIA spec