[EuiPopover][EuiContextMenu] Attempt to manually restore focus to the toggling button on popover close if focus becomes stranded #5760

cee-chen · 2022-04-05T18:44:57Z

Summary

This PR addresses EuiContextMenu (and other potential popovers with complex content that may inadvertantly hijack focus or confuse the popover focus trap) by always attempting to manually focus back to the passed button element (or any focusable children within the passed button).

This PR also upgrades our tabbable dependency to the latest so that we can obtain its new focusable API.

Before

After

Checklist

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Updated the Figma library counterpart~~

Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress tests
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

+ update import/fix types

…gle button on trap deactivation

cee-chen · 2022-04-05T18:49:33Z

package.json

@@ -91,7 +91,7 @@
    "remark-emoji": "^2.1.0",
    "remark-parse": "^8.0.3",
    "remark-rehype": "^8.0.0",
-    "tabbable": "^3.0.0",
+    "tabbable": "^5.2.1",


I upgraded tabbable because I wanted their new focusable API, and also generally just to get us on the latest. This has some amount of risk but I've briefly QA'd potentially affected components (EuiContextMenu, EuiDataGrid, EuiComboBox, EuiSuperSelect) and have not found any UX issues.

cee-chen · 2022-04-05T18:54:16Z

src/components/popover/popover.tsx

+      const focusableItems = focusable(this.button);
+      if (focusableItems.length) {
+        const toggleButton = focusableItems[0];
+        requestAnimationFrame(() => toggleButton.focus(returnFocusConfig));


A couple notes here:

Q: Why focusable and not tabbable?
A: I actually ran into this on EuiDataGrid while testing/attempting to fix some of its focus shenanigans (separate from this PR). If a consumer sets tabIndex={-1} on a button (or in my case, a grid cell), it is still focusable but not tabbable - and it is still better for keyboard users to attempt to return to this element rather than being stranded at the top of the document - thus the need for specifying focusable over tabbable.

Q: Why the requestAnimationFrame delay?
A: This one comes from react-focus-lock's docs:

[...] if you are using the disabled prop to control FocusLock, you will need a zero-timeout to correctly restore focus.

cee-chen · 2022-04-05T18:55:56Z

src/components/popover/popover.test.tsx

+    it('refocuses the first nested toggle button on focus trap deactivation', () => {
+      const toggleButtonEl = React.createRef<HTMLButtonElement>();
+      const toggleDiv = (
+        <div>
+          <button ref={toggleButtonEl} tabIndex={-1} />


Just want to note that this button DOM setup isn't common, and is most likely to come from EuiWrappingPopover (which adds a div wrapper as a button prop) - hence the need for using focusable() in the first place over simply attempting to focus directly onto the button prop

kibanamachine · 2022-04-05T19:21:40Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5760/

thompsongl · 2022-04-05T20:03:52Z

This is definitely edge-case-y, but maybe you can explain or have ideas. It looks like when a form element has focus prior to esc closing the panel, focus is not returned. It also happens in the very last context menu example when the tabbed content is focused.

I'm not terribly worried because this will be a huge improvement regardless

Screen.Recording.2022-04-05.at.2.59.42.PM.mov

cee-chen · 2022-04-05T20:29:51Z

Ah nice catch, thanks Greg! Looking into this now, also confused as to why it would be happening 🤔

cee-chen · 2022-04-05T20:43:18Z

It looks like it's react-focus-lock's returnFocus prop causing the above behavior. If I comment returnFocus out, refocusing works without issue. I can also replace requestAnimationFrame with a a setTimeout() with a timeout of 1 and it starts working (presumably because 1 runs later than returnFocus true's setTimeout of 0).

Any preference? With this new code on place, passing returnFocus honestly seems somewhat superfluous at this point and I'm tempted to set it to false.

cee-chen · 2022-04-05T20:48:33Z

Actually, I also just realized I should be putting this code in the onEscapeKey function only and not in onDeactivation (which also triggers on mouse clicks). If you mouse click out of an open popover into another interactive item the focus jumps back to the toggle button which is not necessarily always desired.

+ improve unit tests to use .invoke()

thompsongl · 2022-04-05T21:55:02Z

Actually, I also just realized I should be putting this code in the onEscapeKey function only and not in onDeactivation (which also triggers on mouse clicks). If you mouse click out of an open popover into another interactive item the focus jumps back to the toggle button which is not necessarily always desired.

That sounds right

With this new code on place, passing returnFocus honestly seems somewhat superfluous at this point and I'm tempted to set it to false.

Let's try that. It'd be cool if the function version of returnFocus could return elementToFocus as part of its config, taking priority over the originating element:

https://github.com/theKashey/react-focus-lock/blob/9363340ed3155a00269538be3d06e4924df195ae/src/Lock.js#L73-L90

- Instead of always running this logic, wait the popover to finish closing and check for stranded document.activeElement first before manually restoring focus

i've come to update you again because a code change softly creeping left its diffs while I was sleeping

cee-chen · 2022-04-05T22:19:06Z

src/components/popover/popover.tsx

+  handleStrandedFocus = () => {
+    setTimeout(() => {
+      // If `returnFocus` failed and focus was stranded on the body,
+      // attempt to manually restore focus to the toggle button


@thompsongl I ended up doing a complete 180 (🤠) and decided to leave in returnFocus as-is and instead narrowed down the scope of this bugfix further. This was for a few reasons:

Removing returnFocus ended up stranding cell focus completely for EuiDataGrid, since the button anchor it passes is the child of the interactive cell and so it never returns a focusable element. In theory [EuiDataGrid] When a cell expansion popover is closed, ensure focus is always returned to the originating cell for keyboard users #5761 addresses this but as a separate issue - but I got worried at that point there were other cases where returnFocus was working better than this code and decided to leave it in.

focusable() iteration is slightly less performant than the simpler ref tracking react-focus-lock uses, so it makes sense to prefer it as a fallback and not as a default

Also, it turns out focus doesn't get 'fully' stranded back to the document body until the close popover animation finishes running, so I had to abstract out that duration (250ms) and use that as the setTimeout for this check, which handles the issue of this logic fighting returnFocus.

Thoughts?

This all makes sense to me 👍

Another idea: Could we use the function version of returnFocus, which would get the to-be-focused element as its argument, and then return false if it's document.body and run your stranded focus function? If the arg to returnFocus is valid return the returnFocus object; then no need to run all focusable stuff.

I can definitely give that a shot! I'm definitely curious to see what it sends as the returnFocusTo arg

Sadly, it looks like from my logging that EuiContextMenu is the receiving the popover content as the returnFocusTo (see first 3 log lines) and not the toggling button (see last 2 logs, which are coming from a standard EuiPopover).

That does explain why the focus is getting stranded; it's attempting to return to a DOM node that's getting deleted essentially. My best guess is that because EuiContextMenu does so much focus hijacking that it's focusing on the panel too early and causing document.activeElement to be incorrect on activation.

Ah yep that makes sense. Thanks for investigating!

@thompsongl So it looks like if we comment out lines 268-271 of context_menu_panel.tsx:

eui/src/components/context_menu/context_menu_panel.tsx

Lines 268 to 271 in b60e810

// Focus on the panel as a last resort.

if (this.panel && !this.panel.contains(document.activeElement)) {

this.panel.focus();

}

Then none of the code in this PR is specifically needed. The above lines specifically are what's causing the early focus hijacking. I don't see a huge downside to removing the above lines as EuiPopover/EuiFocusTrap handles focusing the popover wrapper in any case if no children are focusable.

However, I still feel this PR is overall a more robust solution: it's 100% possible for any consumer to .focus() something randomly upon opening a popover and cause react-focus-lock's document.activeElement to be incorrect. This PR handles cases that we don't control and with fairly limited scope, and IMO is a net benefit to not just EuiContextMenu.

WDYT?

Let's move forward with this PR. EuiContextMenu is not necessarily always in an EuiPopover so we can't rely entirely on the focus mechanism(s) it provides.

kibanamachine · 2022-04-05T22:59:26Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5760/

+ improve listener cleanup unit tests

kibanamachine · 2022-04-06T20:21:20Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5760/

thompsongl

There's some on-close logic in componentDidUpdate. We don't need to account for stranded focus in that scenario, right?

src/components/popover/popover.tsx

kibanamachine · 2022-04-06T22:25:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5760/

thompsongl · 2022-04-07T14:24:08Z

jenkins test this

kibanamachine · 2022-04-07T14:56:12Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5760/

thompsongl

LGTM! Thanks for the new tests!

- this makes it so EuiContextMenu doesn't call `.focus()` too early and hijack the `returnFocus` element that our focus trap dependency sets via `document.activeElement` - see elastic#5760 (comment)

- this makes it so EuiContextMenu doesn't call `.focus()` too early and hijack the `returnFocus` element that our focus trap dependency sets via `document.activeElement` - see elastic#5760 (comment) + Add Cypress E2E tests for popover close focus return (w/ bonus `initialFocus` regression test)

… remove need for `watchedItemProps` (#5880) * [misc] move componentDidUpdate fn - move it closer to updateFocus / componentDidMount for easier context between the other two methods * [setup] Refactor popover parent finding logic - move to separate method - create instance var - specify `initialPopover` and add `transitionType` check, as the popover doesn't re-initialize when moving between panels in the same popover, and we don't want this to re-fire unnecessarily - add E2E tests for popover behavior * [fix] Add a wait condition/state for popover focus - this makes it so EuiContextMenu doesn't call `.focus()` too early and hijack the `returnFocus` element that our focus trap dependency sets via `document.activeElement` - see #5760 (comment) + Add Cypress E2E tests for popover close focus return (w/ bonus `initialFocus` regression test) * [!!!] EuiContextMenuPanels with `children` are still broken and do not correctly return focus :( - this is because of `shouldComponentUpdate` - the `items` API updates focus less than `children`, so `children` is still updating/hijacking focus after the popover focus trap returns focus to the button * [!!!] Remove `shouldComponentUpdate` logic & `watchedItemProps` - replace `updateFocus` with `takeInitialFocus`, and do not continue to update/hijack focus once initial focus has been set - this removes the need to restrict how often `EuiContextMenuPanel` updates (which also requires a bunch of tedious `items` diffing that we will no longer need) * [!!!] Move `tabbable` iteration out of focus logic and into its own method - it shouldn't be tied to the focus call anymore since the focus call no longer occurs after update, and makes more sense as a separate call + updates to logic: - do not run `tabbable` on `children` API since it won't even use the navigation - return early - use `this.backButton` instead for back button focusing, since `children` will no longer have `menuItems` - Check for a valid focusedItem - it's possible for consumers to either pass in bad indices or for `items` to update and the index to no longer exist - Add E2E tests confirming changes & new logic work as expected * [!!!] Restore up/down key navigation behavior - rename `incrementFocusedItemIndex` to `focusMenuItem` and change args to be a bit more human readable - instead of having the previous `updateFocus` handle up/down nav, we can simply call `.focus()` from within this method, and arrow navigation works as before - note `?.focus();` - this is important to keep as users can start mashing up/down before `tabbable` is done running and there are any menu items to focus - no specific E2E tests for this, tests should simply not be failing * changelog * [PR feedback] Prefer not to hook into className for popover panel

cee-chen added 4 commits April 5, 2022 08:47

Upgrade tabbable to latest to get focusable API

b60e810

+ update import/fix types

Update popovers to manually attempt to always focus back onto the tog…

8eb8105

…gle button on trap deactivation

Update downstream snapshots

ac56282

Add changelog entry

a429a4c

cee-chen commented Apr 5, 2022

View reviewed changes

Move refocusButtonOnClose logic to only occur on escape key

ae21121

+ improve unit tests to use .invoke()

cee-chen added 2 commits April 5, 2022 14:59

Narrow down scope of bugfix further

254dc79

- Instead of always running this logic, wait the popover to finish closing and check for stranded document.activeElement first before manually restoring focus

hello snapshots my old friend

75342b4

i've come to update you again because a code change softly creeping left its diffs while I was sleeping

cee-chen commented Apr 5, 2022

View reviewed changes

cee-chen changed the title ~~[EuiPopover][EuiContextMenu] Attempt to force manual focus restoration on toggling button when a popover is closed~~ [EuiPopover][EuiContextMenu] Attempt to manually restore focus to the toggling button on popover close if focus becomes stranded Apr 5, 2022

Tweak changelog entry

c01d2f4

Add clearTimeout for unmount/open of stranded focus logic

39e49e4

+ improve listener cleanup unit tests

thompsongl reviewed Apr 6, 2022

View reviewed changes

src/components/popover/popover.tsx Outdated Show resolved Hide resolved

[PR feedback] fix accidental deletion

148ddb2

cee-chen requested a review from thompsongl April 7, 2022 16:59

thompsongl approved these changes Apr 7, 2022

View reviewed changes

cee-chen merged commit f5f97bc into elastic:main Apr 7, 2022

cee-chen deleted the 5404/popover-refocus branch April 7, 2022 17:22

cee-chen mentioned this pull request Apr 8, 2022

[EuiContextMenu] esc keypress resets focus #4225

Closed

cee-chen mentioned this pull request Apr 13, 2022

[EuiPopover] Focus on parent popover panel by default, instead of first tabbable child #5784

Merged

26 tasks

thompsongl mentioned this pull request Apr 19, 2022

Upgrade EUI to v55.0.1 elastic/kibana#130593

Merged

cee-chen mentioned this pull request May 5, 2022

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for watchedItemProps #5880

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiPopover][EuiContextMenu] Attempt to manually restore focus to the toggling button on popover close if focus becomes stranded #5760

[EuiPopover][EuiContextMenu] Attempt to manually restore focus to the toggling button on popover close if focus becomes stranded #5760

cee-chen commented Apr 5, 2022 •

edited

Loading

cee-chen Apr 5, 2022

cee-chen Apr 5, 2022

cee-chen Apr 5, 2022

kibanamachine commented Apr 5, 2022

thompsongl commented Apr 5, 2022

cee-chen commented Apr 5, 2022

cee-chen commented Apr 5, 2022 •

edited

Loading

cee-chen commented Apr 5, 2022

thompsongl commented Apr 5, 2022 •

edited

Loading

cee-chen Apr 5, 2022 •

edited

Loading

thompsongl Apr 6, 2022 •

edited

Loading

cee-chen Apr 6, 2022

cee-chen Apr 6, 2022 •

edited

Loading

thompsongl Apr 6, 2022

cee-chen Apr 6, 2022 •

edited

Loading

thompsongl Apr 7, 2022 •

edited

Loading

kibanamachine commented Apr 5, 2022

kibanamachine commented Apr 6, 2022

thompsongl left a comment

kibanamachine commented Apr 6, 2022

thompsongl commented Apr 7, 2022

kibanamachine commented Apr 7, 2022

thompsongl left a comment

	// Focus on the panel as a last resort.
	if (this.panel && !this.panel.contains(document.activeElement)) {
	this.panel.focus();
	}

[EuiPopover][EuiContextMenu] Attempt to manually restore focus to the toggling button on popover close if focus becomes stranded #5760

[EuiPopover][EuiContextMenu] Attempt to manually restore focus to the toggling button on popover close if focus becomes stranded #5760

Conversation

cee-chen commented Apr 5, 2022 • edited Loading

Summary

Before

After

Checklist

cee-chen Apr 5, 2022

Choose a reason for hiding this comment

cee-chen Apr 5, 2022

Choose a reason for hiding this comment

cee-chen Apr 5, 2022

Choose a reason for hiding this comment

kibanamachine commented Apr 5, 2022

thompsongl commented Apr 5, 2022

cee-chen commented Apr 5, 2022

cee-chen commented Apr 5, 2022 • edited Loading

cee-chen commented Apr 5, 2022

thompsongl commented Apr 5, 2022 • edited Loading

cee-chen Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

cee-chen Apr 6, 2022

Choose a reason for hiding this comment

cee-chen Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Apr 6, 2022

Choose a reason for hiding this comment

cee-chen Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Apr 5, 2022

kibanamachine commented Apr 6, 2022

thompsongl left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 6, 2022

thompsongl commented Apr 7, 2022

kibanamachine commented Apr 7, 2022

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Apr 5, 2022 •

edited

Loading

cee-chen commented Apr 5, 2022 •

edited

Loading

thompsongl commented Apr 5, 2022 •

edited

Loading

cee-chen Apr 5, 2022 •

edited

Loading

thompsongl Apr 6, 2022 •

edited

Loading

cee-chen Apr 6, 2022 •

edited

Loading

cee-chen Apr 6, 2022 •

edited

Loading

thompsongl Apr 7, 2022 •

edited

Loading