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

Jump to bottom

[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 4 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
Member 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
3 changes: 3 additions & 0 deletions src/components/datagrid/controls/__snapshots__/column_selector.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ exports[`useDataGridColumnSelector columnSelector renders a toolbar button/popov
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
Expand All @@ -370,6 +371,7 @@ exports[`useDataGridColumnSelector columnSelector renders a toolbar button/popov
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
Expand All @@ -379,6 +381,7 @@ exports[`useDataGridColumnSelector columnSelector renders a toolbar button/popov
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
Expand Down
3 changes: 3 additions & 0 deletions src/components/datagrid/controls/__snapshots__/column_sorting.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ exports[`useDataGridColumnSorting columnSorting renders a toolbar button/popover
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
Expand All @@ -381,6 +382,7 @@ exports[`useDataGridColumnSorting columnSorting renders a toolbar button/popover
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
Expand All @@ -390,6 +392,7 @@ exports[`useDataGridColumnSorting columnSorting renders a toolbar button/popover
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={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
3 changes: 3 additions & 0 deletions src/components/markdown_editor/__snapshots__/markdown_editor.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,7 @@ exports[`EuiMarkdownEditor custom plugins are excluded and popover is rendered 1
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
Expand All @@ -866,6 +867,7 @@ exports[`EuiMarkdownEditor custom plugins are excluded and popover is rendered 1
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
Expand All @@ -875,6 +877,7 @@ exports[`EuiMarkdownEditor custom plugins are excluded and popover is rendered 1
initialFocus={[Function]}
noIsolation={true}
onClickOutside={[Function]}
onDeactivation={[Function]}
onEscapeKey={[Function]}
returnFocus={false}
scrollLock={false}
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
68 changes: 68 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 @@ -439,6 +439,74 @@ describe('EuiPopover', () => {
jest.advanceTimersByTime(10);
});
});

describe('refocusButtonOnClose', () => {
let rafSpy: jest.SpyInstance;
beforeAll(() => {
rafSpy = jest
.spyOn(window, 'requestAnimationFrame')
.mockImplementation((cb: Function) => cb());
});
afterAll(() => {
rafSpy.mockRestore();
});

it('refocuses the toggle button on focus trap deactivation', () => {
const toggleButtonEl = React.createRef<HTMLButtonElement>();
const toggleButton = <button ref={toggleButtonEl} />;

const component = mount(
<EuiPopover
button={toggleButton}
isOpen={true}
closePopover={() => {}}
{...requiredProps}
/>
);
(component.find('EuiFocusTrap').prop('onDeactivation') as Function)();

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
Member 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
button={toggleDiv}
isOpen={true}
closePopover={() => {}}
{...requiredProps}
/>
);
(component.find('EuiFocusTrap').prop('onDeactivation') as Function)();

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={() => {}}
{...requiredProps}
/>
);
(component.find('EuiFocusTrap').prop('onDeactivation') as Function)();

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

describe('getPopoverPositionFromAnchorPosition', () => {
Expand Down
15 changes: 13 additions & 2 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 @@ -666,6 +666,17 @@ export class EuiPopover extends Component<Props, State> {
this.props.buttonRef && this.props.buttonRef(node);
};

refocusButtonOnClose = () => {
if (this.button) {
const focusableItems = focusable(this.button);
if (focusableItems.length) {
const toggleButton = focusableItems[0];
requestAnimationFrame(() => toggleButton.focus(returnFocusConfig));
Copy link
Member Author

Choose a reason for hiding this comment

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

A couple notes here:

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

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

}
}
this.props.onTrapDeactivation?.();
};

render() {
const {
anchorClassName,
Expand Down Expand Up @@ -776,7 +787,7 @@ 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
onDeactivation={this.refocusButtonOnClose}
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