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.

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
4 changes: 2 additions & 2 deletions package.json
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
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
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
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
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
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
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
141 changes: 119 additions & 22 deletions src/components/popover/popover.test.tsx
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 @@ -383,33 +384,36 @@ describe('EuiPopover', () => {
});

describe('listener cleanup', () => {
let _raf: typeof window['requestAnimationFrame'];
let _caf: typeof window['cancelAnimationFrame'];
let rafSpy: jest.SpyInstance;
let cafSpy: jest.SpyInstance;
const activeAnimationFrames = new Map<number, number>();
let nextAnimationFrameId = 0;

beforeAll(() => {
jest.useFakeTimers();
_raf = window.requestAnimationFrame;
_caf = window.cancelAnimationFrame;

const activeAnimationFrames = new Map<number, number>();
let nextAnimationFrameId = 0;
window.requestAnimationFrame = (fn) => {
const animationFrameId = nextAnimationFrameId++;
activeAnimationFrames.set(animationFrameId, setTimeout(fn));
return animationFrameId;
};
window.cancelAnimationFrame = (id: number) => {
const timeoutId = activeAnimationFrames.get(id);
if (timeoutId) {
clearTimeout(timeoutId);
activeAnimationFrames.delete(id);
}
};
jest.spyOn(window, 'clearTimeout');
rafSpy = jest
.spyOn(window, 'requestAnimationFrame')
.mockImplementation((fn) => {
const animationFrameId = nextAnimationFrameId++;
activeAnimationFrames.set(animationFrameId, setTimeout(fn));
return animationFrameId;
});
cafSpy = jest
.spyOn(window, 'cancelAnimationFrame')
.mockImplementation((id: number) => {
const timeoutId = activeAnimationFrames.get(id);
if (timeoutId) {
clearTimeout(timeoutId);
activeAnimationFrames.delete(id);
}
});
});

afterAll(() => {
jest.useRealTimers();
window.requestAnimationFrame = _raf;
window.cancelAnimationFrame = _caf;
rafSpy.mockRestore();
cafSpy.mockRestore();
});

it('cleans up timeouts and rAFs on unmount', () => {
Expand All @@ -422,10 +426,21 @@ describe('EuiPopover', () => {
isOpen={false}
/>
);
expect(window.clearTimeout).toHaveBeenCalledTimes(0);

component.setProps({ isOpen: true });
expect(window.clearTimeout).toHaveBeenCalledTimes(3);
expect(rafSpy).toHaveBeenCalledTimes(1);
expect(activeAnimationFrames.size).toEqual(1);

jest.advanceTimersByTime(10);
expect(rafSpy).toHaveBeenCalledTimes(2);
expect(activeAnimationFrames.size).toEqual(2);

component.unmount();
expect(window.clearTimeout).toHaveBeenCalledTimes(10);
expect(cafSpy).toHaveBeenCalledTimes(2);
expect(activeAnimationFrames.size).toEqual(0);

// EUI's jest configuration throws an error if there are any console.error calls, like
// React's setState on an unmounted component warning
Expand All @@ -436,7 +451,89 @@ describe('EuiPopover', () => {

// execute any pending timeouts or animation frame callbacks
// and validate the timeout/rAF clearing done by EuiPopover
jest.advanceTimersByTime(10);
jest.advanceTimersByTime(300);
});
});

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
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
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);
});
});
});
Expand Down
25 changes: 23 additions & 2 deletions src/components/popover/popover.tsx
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 @@ -346,6 +347,7 @@ export class EuiPopover extends Component<Props, State> {
}

private respositionTimeout: number | undefined;
private strandedFocusTimeout: number | undefined;
private closingTransitionTimeout: number | undefined;
private closingTransitionAnimationFrame: number | undefined;
private updateFocusAnimationFrame: number | undefined;
Expand Down Expand Up @@ -383,9 +385,26 @@ export class EuiPopover extends Component<Props, State> {
event.preventDefault();
event.stopPropagation();
this.closePopover();
this.handleStrandedFocus();
}
};

handleStrandedFocus = () => {
this.strandedFocusTimeout = window.setTimeout(() => {
// If `returnFocus` failed and focus was stranded on the body,
// attempt to manually restore focus to the toggle button
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 @@ -463,6 +482,7 @@ export class EuiPopover extends Component<Props, State> {
}

onOpenPopover = () => {
clearTimeout(this.strandedFocusTimeout);
clearTimeout(this.closingTransitionTimeout);
if (this.closingTransitionAnimationFrame) {
cancelAnimationFrame(this.closingTransitionAnimationFrame);
Expand Down Expand Up @@ -541,13 +561,14 @@ export class EuiPopover extends Component<Props, State> {
this.setState({
isClosing: false,
});
}, 250);
}, closingTransitionTime);
}
}

componentWillUnmount() {
window.removeEventListener('scroll', this.positionPopoverFixed, true);
clearTimeout(this.respositionTimeout);
clearTimeout(this.strandedFocusTimeout);
clearTimeout(this.closingTransitionTimeout);
cancelAnimationFrame(this.closingTransitionAnimationFrame!);
cancelAnimationFrame(this.updateFocusAnimationFrame!);
Expand Down
3 changes: 3 additions & 0 deletions upcoming_changelogs/5760.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed keyboard focus being stranded on `EuiContextMenu` popover close
16 changes: 8 additions & 8 deletions yarn.lock
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