[EuiContextMenu] Multiple keyboard navigation & focus fixes#5783
Merged
cee-chen merged 9 commits intoelastic:mainfrom Apr 15, 2022
Merged
[EuiContextMenu] Multiple keyboard navigation & focus fixes#5783cee-chen merged 9 commits intoelastic:mainfrom
cee-chen merged 9 commits intoelastic:mainfrom
Conversation
- updateFocus() was being called and focus being blurred between panel transitions. If users were pressing the left/right arrow keys very quickly on the EUI documentation site, this would lead to accidental page navigation which can be quite annoying - It looks like the `hasFocus` prop simply isn't necessary and is already adequately handled by the transition props, hence the complete removal - the added `.focus()` line on 227 keeps focus on the primary panel between transitions instead of stranding focus on removed panels - preventScroll prevents scroll jumping between panels
- by setting this.state.menuItems from `tabbable` instead of from refs, we can capture *all* focusable items (including EuiContextMenuItems wrapped in parents as well as just plain <button>s) + simplify updateFocus flow now that all tabbable items are menuItems + remove behavior where the first tabbable child is focused - we're removing this behavior in EuiPopover shortly as well, so it makes sense to also prune here
- Now that we're always focusing the panel by default regardless of `children`, it really doesn't seem necessary, and may also prevent `tabbable` checking when it shouldn't
- which requires some index +- 1 shenanigans with the parent EuiContextMenu - since this is essentially shifting a new item to the beginning of the array if `onClose` (the prop for the back button) exists + also requires removing item separators from the parent EuiContextMenu indices, to try and get the 2 separate arrays more aligned + requires removing ?? 0 fallback in EuiContextMenu, since we now rely on undefined checks - architecturally this is a bit questionable / shenanigans-y, but the entire EuiContextMenu component likely needs a larger look/overhaul in any case, so this works for now and improves keyboard UX (just IMO) + misc `items?.length` cleanup
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5783/ |
Contributor
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5783/ |
26 tasks
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5783/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5783/ |
thompsongl
approved these changes
Apr 14, 2022
Contributor
thompsongl
left a comment
There was a problem hiding this comment.
Changes make sense to me. Couldn't break anything with manual testing, and the new cypress tests are 🏅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I strongly recommend following along by commit, as there's some cleanup commits and also some specific fixes that needed to be done in the parent
EuiContextMenu.Cypress tests have been added for each major bugfix/behavior.
Bugs fixed:
8babf1a: Stranded focus when pressing left/right arrows very quickly between panels
This probably only affected me as a person who was QAing and spamming left/arrow keys very quickly to swap between more than 2 panels at once, but it annoyed me enough to try and figure out a fix for it 😅
Before (the entire page changes because we hook our left/right arrow keys to documentation navigation):
After (no lost focus):
4a1bcf0: Allow tabbable
itemsthat aren'tEuiContextMenuItems to be arrow key navigableThe impetus for this came from elastic/kibana#129333 - while the request was more specifically for
EuiContextItems within wrappers, e.g.EuiCopyorEuiTooltip, I saw making every tabbable element within anitemnavigable by arrow keys as a more robust outcome.8b33833: Allow the back button panel title to be arrow key navigable
It kind of annoyed me personally that the back button titles weren't part of the up/down arrow key navigation, so I thought it would be smoother to include them. I also frankly prefer the back button being the first focusable item when navigating to a new panel.
This does introduce some architectural spaghetti / a mismatch between the parent
EuiContextMenu's focus state internals andEuiContextMenuPanel's focus state internals, but I think it's worth it.Bugs not fixed:
[EuiContextMenu][COGNITION]: Consider refactoring the EUI Context Menu to WAI spec for navigation menu button #5737: I briefly thought about adding some aria
roles as part of this PR, but quickly realized it was way more out of scope than I wanted to tackle. Chandler did mention EuiContextMenu could generally stand an overall architecture overhaul, and frankly I do agree, but in the interim this PR should smooth over some keyboard UX friction.[EuiContextMenu] Hover/active state shenanigans within EuiContextMenu items #4973: I investigated this issue as part of this which ended up helping inform some of my decisions, but as part of a longer Slack discussion between multiple designers/devs we realized the underlying issue is actually coming from
EuiPopoverwhich is hijacking focus to the first tabbable item in the popover/context menu. We determined that this behavior generally is worse for screen readers than focusing the popover, so as a result I will address this issue with a separate breaking change toEuiPopoverthat removes this logic at a macro level.Checklist
- [ ] Checked in mobile(same as desktop AFAICT)- [ ] Checked in both light and dark modes- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Updated the Figma library counterpart- [ ] Checked for breaking changes and labeled appropriately