Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Apr 7, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b60e810
Upgrade `tabbable` to latest to get `focusable` API
cee-chen Apr 5, 2022
8eb8105
Update popovers to manually attempt to always focus back onto the tog…
cee-chen Apr 5, 2022
ac56282
Update downstream snapshots
cee-chen Apr 5, 2022
a429a4c
Add changelog entry
cee-chen Apr 5, 2022
ae21121
Move `refocusButtonOnClose` logic to only occur on escape key
cee-chen Apr 5, 2022
254dc79
Narrow down scope of bugfix further
cee-chen Apr 5, 2022
75342b4
hello snapshots my old friend
cee-chen Apr 5, 2022
c01d2f4
Tweak changelog entry
cee-chen Apr 5, 2022
39e49e4
Add clearTimeout for unmount/open of stranded focus logic
cee-chen Apr 6, 2022
148ddb2
[PR feedback] fix accidental deletion
cee-chen Apr 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"text-diff": "^1.0.1",
"unified": "^9.2.0",
"unist-util-visit": "^2.0.3",
Expand Down Expand Up @@ -133,7 +133,7 @@
"@types/react-dom": "^17.0.11",
"@types/react-is": "^17.0.3",
"@types/react-router-dom": "^5.1.5",
"@types/tabbable": "^3.1.0",
"@types/tabbable": "^3.1.2",
"@types/url-parse": "^1.4.8",
"@types/uuid": "^8.3.0",
"@typescript-eslint/eslint-plugin": "^5.10.2",
Expand Down
2 changes: 1 addition & 1 deletion src/components/context_menu/context_menu_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import React, {
ReactNode,
} from 'react';
import classNames from 'classnames';
import tabbable from 'tabbable';
import { tabbable } from 'tabbable';

import { CommonProps, NoArgCallback, keysOf } from '../common';
import { EuiIcon } from '../icon';
Expand Down
2 changes: 1 addition & 1 deletion src/components/datagrid/body/data_grid_cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import React, {
MutableRefObject,
} from 'react';
import { createPortal } from 'react-dom';
import tabbable from 'tabbable';
import { tabbable } from 'tabbable';
import { keys } from '../../../services';
import { EuiScreenReaderOnly } from '../../accessibility';
import { EuiFocusTrap } from '../../focus_trap';
Expand Down
2 changes: 1 addition & 1 deletion src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import React, {
useRef,
useState,
} from 'react';
import tabbable from 'tabbable';
import { tabbable } from 'tabbable';
import { keys } from '../../../../services';
import { DataGridFocusContext } from '../../utils/focus';
import { EuiDataGridHeaderCellWrapperProps } from '../../data_grid_types';
Expand Down
2 changes: 1 addition & 1 deletion src/components/datagrid/body/header/header_is_interactive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { useCallback, useEffect, useState } from 'react';
import tabbable from 'tabbable';
import { tabbable } from 'tabbable';

export const useHeaderIsInteractive = (gridElement: HTMLElement | null) => {
const [headerIsInteractive, setHeaderIsInteractive] = useState(false);
Expand Down
2 changes: 1 addition & 1 deletion src/components/datagrid/utils/focus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
MutableRefObject,
} from 'react';
import { GridOnItemsRenderedProps } from 'react-window';
import tabbable from 'tabbable';
import { tabbable } from 'tabbable';
import { keys } from '../../../services';
import {
DataGridFocusContextShape,
Expand Down
4 changes: 2 additions & 2 deletions src/components/popover/input_popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import React, {
useCallback,
} from 'react';
import classnames from 'classnames';
import tabbable from 'tabbable';
import { tabbable, FocusableElement } from 'tabbable';

import { CommonProps } from '../common';
import { EuiFocusTrap } from '../focus_trap';
Expand Down Expand Up @@ -81,7 +81,7 @@ export const EuiInputPopover: FunctionComponent<EuiInputPopoverProps> = ({

const onKeyDown = (event: React.KeyboardEvent) => {
if (panelEl && event.key === cascadingMenuKeys.TAB) {
const tabbableItems = tabbable(panelEl).filter((el: HTMLElement) => {
const tabbableItems = tabbable(panelEl).filter((el: FocusableElement) => {
return (
Array.from(el.attributes)
.map((el) => el.name)
Expand Down
83 changes: 83 additions & 0 deletions src/components/popover/popover.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import React, { ReactNode } from 'react';
import { render, mount } from 'enzyme';
import { requiredProps } from '../../test/required_props';
import { EuiFocusTrap } from '../';

import {
EuiPopover,
Expand Down Expand Up @@ -439,6 +440,88 @@ describe('EuiPopover', () => {
jest.advanceTimersByTime(10);
});
});

describe('onEscapeKey', () => {
const closePopover = jest.fn();
const closingTransitionTime = 250; // TODO: DRY out var when converting to CSS-in-JS

const mockEvent = {
preventDefault: () => {},
stopPropagation: () => {},
} as Event;

beforeAll(() => jest.useFakeTimers());
beforeEach(() => {
jest.clearAllMocks();
(document.activeElement as HTMLElement)?.blur(); // Reset focus between tests
});
afterAll(() => jest.useRealTimers());

it('closes the popover and refocuses the toggle button', () => {
const toggleButtonEl = React.createRef<HTMLButtonElement>();
const toggleButton = <button ref={toggleButtonEl} />;

const component = mount(
<EuiPopover
isOpen={true}
button={toggleButton}
closePopover={closePopover}
{...requiredProps}
/>
);
component.find(EuiFocusTrap).invoke('onEscapeKey')!(mockEvent);
component.setProps({ isOpen: false });
jest.advanceTimersByTime(closingTransitionTime);

expect(closePopover).toHaveBeenCalled();
expect(document.activeElement).toEqual(toggleButtonEl.current);
});

it('refocuses the first nested toggle button on focus trap deactivation', () => {
const toggleButtonEl = React.createRef<HTMLButtonElement>();
const toggleDiv = (
<div>
<button ref={toggleButtonEl} tabIndex={-1} />
Comment on lines +494 to +498
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<button tabIndex={-1} />
</div>
);

const component = mount(
<EuiPopover
isOpen={true}
button={toggleDiv}
closePopover={closePopover}
{...requiredProps}
/>
);
component.find(EuiFocusTrap).invoke('onEscapeKey')!(mockEvent);
component.setProps({ isOpen: false });
jest.advanceTimersByTime(closingTransitionTime);

expect(closePopover).toHaveBeenCalled();
expect(document.activeElement).toEqual(toggleButtonEl.current);
});

it('does not refocus if the toggle button is not focusable', () => {
const toggleDivEl = React.createRef<HTMLDivElement>();
const toggleDiv = <div ref={toggleDivEl} />;

const component = mount(
<EuiPopover
button={toggleDiv}
isOpen={true}
closePopover={closePopover}
{...requiredProps}
/>
);
component.find(EuiFocusTrap).invoke('onEscapeKey')!(mockEvent);
component.setProps({ isOpen: false });
jest.advanceTimersByTime(closingTransitionTime);

expect(closePopover).toHaveBeenCalled();
expect(document.activeElement).not.toEqual(toggleDivEl.current);
});
});
});

describe('getPopoverPositionFromAnchorPosition', () => {
Expand Down
23 changes: 20 additions & 3 deletions src/components/popover/popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import React, {
RefCallback,
} from 'react';
import classNames from 'classnames';
import tabbable from 'tabbable';
import { tabbable, focusable } from 'tabbable';

import { CommonProps, NoArgCallback } from '../common';
import { FocusTarget, EuiFocusTrap, EuiFocusTrapProps } from '../focus_trap';
Expand Down Expand Up @@ -281,6 +281,7 @@ function getElementFromInitialFocus(
}

const returnFocusConfig = { preventScroll: true };
const closingTransitionTime = 250; // TODO: DRY out var when converting to CSS-in-JS

export type Props = CommonProps &
HTMLAttributes<HTMLDivElement> &
Expand Down Expand Up @@ -383,9 +384,26 @@ export class EuiPopover extends Component<Props, State> {
event.preventDefault();
event.stopPropagation();
this.closePopover();
this.handleStrandedFocus();
}
};

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

@cee-chen cee-chen Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. 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.

  2. 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

  3. 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?

Copy link
Contributor

@thompsongl thompsongl Apr 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cee-chen cee-chen Apr 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep that makes sense. Thanks for investigating!

Copy link
Contributor Author

@cee-chen cee-chen Apr 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@thompsongl thompsongl Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (document.activeElement === document.body) {
if (!this.button) return;

const focusableItems = focusable(this.button);
if (!focusableItems.length) return;

const toggleButton = focusableItems[0];
toggleButton.focus(returnFocusConfig);
}
}, closingTransitionTime);
};

onKeyDown = (event: KeyboardEvent) => {
if (event.key === cascadingMenuKeys.ESCAPE) {
this.onEscapeKey((event as unknown) as Event);
Expand Down Expand Up @@ -541,7 +559,7 @@ export class EuiPopover extends Component<Props, State> {
this.setState({
isClosing: false,
});
}, 250);
}, closingTransitionTime);
}
}

Expand Down Expand Up @@ -776,7 +794,6 @@ export class EuiPopover extends Component<Props, State> {
{...focusTrapProps}
returnFocus={returnFocus} // Ignore temporary state of indecisive focus
initialFocus={initialFocus}
onDeactivation={onTrapDeactivation}
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
onClickOutside={this.onClickOutside}
onEscapeKey={this.onEscapeKey}
disabled={
Expand Down
3 changes: 3 additions & 0 deletions upcoming_changelogs/5760.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed focus not being restored to popover toggles for certain complex popover children like `EuiContextMenu`
16 changes: 8 additions & 8 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3465,10 +3465,10 @@
resolved "https://registry.yarnpkg.com/@types/stack-utils/-/stack-utils-1.0.1.tgz#0a851d3bd96498fa25c33ab7278ed3bd65f06c3e"
integrity sha512-l42BggppR6zLmpfU6fq9HEa2oGPEI8yrSPL3GITjfRInppYFahObbIQOQK3UGxEnyQpltZLaPe75046NOZQikw==

"@types/tabbable@^3.1.0":
version "3.1.0"
resolved "https://registry.yarnpkg.com/@types/tabbable/-/tabbable-3.1.0.tgz#540d4c2729872560badcc220e73c9412c1d2bffe"
integrity sha512-LL0q/bTlzseaXQ8j91eZ+Z8FQUzo0nwkng00B8365qULvFyiSOWylxV8m31Gmee3QuidkDqR72a9NRfR8s4qTw==
"@types/tabbable@^3.1.2":
version "3.1.2"
resolved "https://registry.yarnpkg.com/@types/tabbable/-/tabbable-3.1.2.tgz#5046f043fef50961d7727920b0076b37737e31ad"
integrity sha512-Yp+M5IjNZxYjsflBbSalyjUAIqiJyEISg++gLAstGrZlp9lzVi5KAsZvJqThT2qeoqGYnFqdZXorPEYtaVBAkg==

"@types/tapable@*", "@types/tapable@^1.0.5":
version "1.0.6"
Expand Down Expand Up @@ -18843,10 +18843,10 @@ syntax-error@^1.1.1:
dependencies:
acorn-node "^1.2.0"

tabbable@^3.0.0:
version "3.1.2"
resolved "https://registry.yarnpkg.com/tabbable/-/tabbable-3.1.2.tgz#f2d16cccd01f400e38635c7181adfe0ad965a4a2"
integrity sha512-wjB6puVXTYO0BSFtCmWQubA/KIn7Xvajw0x0l6eJUudMG/EAiJvIUnyNX6xO4NpGrJ16lbD0eUseB9WxW0vlpQ==
tabbable@^5.2.1:
version "5.2.1"
resolved "https://registry.yarnpkg.com/tabbable/-/tabbable-5.2.1.tgz#e3fda7367ddbb172dcda9f871c0fdb36d1c4cd9c"
integrity sha512-40pEZ2mhjaZzK0BnI+QGNjJO8UYx9pP5v7BGe17SORTO0OEuuaAwQTkAp8whcZvqon44wKFOikD+Al11K3JICQ==

table@^3.7.8:
version "3.8.3"
Expand Down