feat(menu): add keyboard accessibility to menus #1533
Conversation
|
🚀 Deployed on https://pr-1533--dhis2-ui.netlify.app |
d-rita
changed the title
d-rita
force-pushed
the
LIBS-623/menus-keyboard-accessibility
branch
from
348ec89
to
4ed9a58
Compare
d-rita
force-pushed
the
LIBS-623/menus-keyboard-accessibility
branch
from
4ed9a58
to
543feab
Compare
d-rita
force-pushed
the
LIBS-623/menus-keyboard-accessibility
branch
from
543feab
to
2ccf6d0
Compare
components/menu/src/menu/use-menu.js
Outdated
| const handleFocus = (event) => { | ||
| if (event.target === menuRef.current) { | ||
| const firstItemIndex = focusableItemsIndices?.[0] | ||
| menuRef.current.children[firstItemIndex].focus() |
There was a problem hiding this comment.
this bit errors in the storybook for AutoHideFirstSectionHeaderDivider. it is because firstItemIndex in this case is undefined. Maybe we want to wrap this in a conditional?
| menuRef.current.children[firstItemIndex].focus() | |
| firstItemIndex && menuRef.current.children[firstItemIndex].focus() |
There was a problem hiding this comment.
Okay, so the AutoHideFirstSectionHeaderDivider only contains 2 section headers as its children and in my implementation, those do not receive visual focus. So i have to adjust for cases where the menu does not contain any focusable items. Good catch, thank you
| @@ -69,6 +77,7 @@ export interface MenuItemProps { | |||
| * When true, nested menu items are shown in a Popper | |||
| */ | |||
| showSubMenu?: boolean | |||
| tabIndex?: number | |||
There was a problem hiding this comment.
i think suffix (from a previous pr is missing here as well, can we add it as part of this pr?
|
Thanks @d-rita . Looks good to me. I only have some minor comments above |
- Navigate menu items using Up and Down arrow keys - Space and Enter keys trigger the onClick handler attached to the menu items - Extract navigation and action logic into custom useMenuNavigation hook - Handle visual focus of menu items by tracking only the focusable items - Pass only native props to the native child elements, i.e. tabIndex - Implement tests simulating keyboard interactions
- wrap non-li children in li tags for consistency - assign tabIndex for visual focus to children with menuitem role - add active CSS class to focused MenuItem component - modify tests
- apply focus and keyboard navigation within the menu iff it contains focusable items
d-rita
force-pushed
the
LIBS-623/menus-keyboard-accessibility
branch
from
efb9c88
to
72ec46e
Compare
| const Menu = ({ children, className, dataTest, dense }) => { | ||
| const { menuRef, focusedIndex } = useMenuNavigation(children) | ||
|
|
||
| const childrenToRender = Children.map(children, (child, index) => { |
There was a problem hiding this comment.
as discussed privately, it'd be good to:
- memoise
childrenToRenderto make sure we're not recalculating it unnecessarily - add some comments for the reasons for the extra checks (handling warning when passing regular html etc...)
but this can be done in the subsequent PR
|
There definitely was a bit more to this task. 😅 Thank you for the review @flaminic and @kabaros. 🎉
I have implemented this feedback in this PR. I have also added the menu children role checks and warning if none is provided as was discussed. |
Implements LIBS-623
Description
This feature handles keyboard accessibility of the menu and its children components in the following ways:
Checklist