-
Notifications
You must be signed in to change notification settings - Fork 69
fix(ContextualMenuDropdown): Context menu focuses first item on opening #1298
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
base: main
Are you sure you want to change the base?
Conversation
2b6128e to
2d20990
Compare
2d20990 to
c2610dd
Compare
|
A possible improvement we can already add in this PR is to return focus to the element that opened the context menu when it closes. |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
fasih-mehmood
left a comment
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.
I'm not able to close the dropdown using the keyboard. With the trapped focus, the focus switches back to the first option when pressing Tab at the last option. Pressing Esc does not close the dropdown either. Perhaps the dropdown should close if we press Tab at the last option or alternatively pressing Esc should close it. Wdyt?
|
Did you check the CustomSelect component with this change? I suspect it might need to opt out as well -- at least if the search variant of it is in place. |
@fasih-mehmood Which story are you trying this out on, and which browser? In Chromium on the ContextualMenu toggle story, pressing escape closes the dropdown for me |
@edlerd No, I didn't check that one - thanks for flagging, I'll investigate |
@jmuzina I tried the ContextualMenu Toggle story on Chrome. |
Done
ContextualMenuDropdpownto focus the first non-disabled child item after opening. Then the user can tab through the items to access them via keyboard.MultiSelectcomponent to opt-out of the ContextualMenu autofocusing the first element, as this would make text search difficult (focus should move to the text input, not the first option in that case)Future work
There is still more work that can be done to make this component more accessible & WCAG compliant. WCAG specifies that context menus should be navigable with Arrow keys, and Tab should close the context menu and focus the next item after it. This PR at least makes the context menu navigable with Tab - more work will need to be done to comply with this spec.
I've left that for a separate task as it'll take a bit more work and I wanted to get the component at least minimally accessible in the short-term (a screen reader user raised with WPE that a context menu in 360 is almost impossible to navigate because focus does not move to it on opening and there is no focus trap)
QA
Pinging @canonical/react-library-maintainers for a review.
Storybook
To see rendered examples of all react-components, run:
QA in your project
from
react-componentsrun:Install the resulting tarball in your project with:
QA steps
ContextualMenuDropdownstory.MultiSelectcomponent, which consumesContextualMenu.Percy steps
Fixes
Fixes: #1248