Skip to content

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

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

Merged
merged 10 commits into from
May 10, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a5be54f
[misc] move componentDidUpdate fn
cee-chen May 5, 2022
1e3d257
[setup] Refactor popover parent finding logic
cee-chen May 5, 2022
f6c99b8
[fix] Add a wait condition/state for popover focus
cee-chen May 5, 2022
6ef9342
[!!!] EuiContextMenuPanels with `children` are still broken and do no…
cee-chen May 5, 2022
1fb9b24
[!!!] Remove `shouldComponentUpdate` logic & `watchedItemProps`
cee-chen May 5, 2022
40dc5c4
[!!!] Move `tabbable` iteration out of focus logic and into its own m…
cee-chen May 5, 2022
4b397b0
[!!!] Restore up/down key navigation behavior
cee-chen May 5, 2022
1721abf
changelog
cee-chen May 6, 2022
6a7a464
Merge branch 'main' into contextmenu/focus-fix
cee-chen May 9, 2022
f8e7a5c
[PR feedback] Prefer not to hook into className for popover panel
cee-chen May 9, 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
140 changes: 0 additions & 140 deletions src/components/context_menu/__snapshots__/context_menu_panel.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,143 +130,3 @@ exports[`EuiContextMenuPanel props transitionDirection previous with transitionT
<div />
</div>
`;

exports[`EuiContextMenuPanel updating items and content updates to items should not re-render if any items's watchedItemProps did not change 1`] = `
"<EuiContextMenuPanel items={{...}}>
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
<EuiResizeObserver onResize={[Function: onResize]}>
<div>
<EuiContextMenuItem data-counter={0}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={0}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option A
</span>
</span>
</button>
</EuiContextMenuItem>
<EuiContextMenuItem data-counter={1}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={1}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option B
</span>
</span>
</button>
</EuiContextMenuItem>
</div>
</EuiResizeObserver>
</div>
</EuiContextMenuPanel>"
`;

exports[`EuiContextMenuPanel updating items and content updates to items should not re-render if any items's watchedItemProps did not change 2`] = `
"<EuiContextMenuPanel items={{...}}>
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
<EuiResizeObserver onResize={[Function: onResize]}>
<div>
<EuiContextMenuItem data-counter={0}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={0}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option A
</span>
</span>
</button>
</EuiContextMenuItem>
<EuiContextMenuItem data-counter={1}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={1}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option B
</span>
</span>
</button>
</EuiContextMenuItem>
</div>
</EuiResizeObserver>
</div>
</EuiContextMenuPanel>"
`;

exports[`EuiContextMenuPanel updating items and content updates to items should re-render at all times when children exists 1`] = `
"<EuiContextMenuPanel items={{...}}>
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
<EuiResizeObserver onResize={[Function: onResize]}>
<div>
Hello World
</div>
</EuiResizeObserver>
</div>
</EuiContextMenuPanel>"
`;

exports[`EuiContextMenuPanel updating items and content updates to items should re-render at all times when children exists 2`] = `
"<EuiContextMenuPanel items={{...}}>
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
<EuiResizeObserver onResize={[Function: onResize]}>
<div>
More Salutations
</div>
</EuiResizeObserver>
</div>
</EuiContextMenuPanel>"
`;

exports[`EuiContextMenuPanel updating items and content updates to items should re-render if any items's watchedItemProps did change 1`] = `
"<EuiContextMenuPanel watchedItemProps={{...}} items={{...}}>
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
<EuiResizeObserver onResize={[Function: onResize]}>
<div>
<EuiContextMenuItem data-counter={0}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={0}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option A
</span>
</span>
</button>
</EuiContextMenuItem>
<EuiContextMenuItem data-counter={1}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={1}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option B
</span>
</span>
</button>
</EuiContextMenuItem>
</div>
</EuiResizeObserver>
</div>
</EuiContextMenuPanel>"
`;

exports[`EuiContextMenuPanel updating items and content updates to items should re-render if any items's watchedItemProps did change 2`] = `
"<EuiContextMenuPanel watchedItemProps={{...}} items={{...}}>
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
<EuiResizeObserver onResize={[Function: onResize]}>
<div>
<EuiContextMenuItem data-counter={2}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={2}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option A
</span>
</span>
</button>
</EuiContextMenuItem>
<EuiContextMenuItem data-counter={3}>
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={3}>
<span className=\\"euiContextMenu__itemLayout\\">
<span className=\\"euiContextMenuItem__text\\">
Option B
</span>
</span>
</button>
</EuiContextMenuItem>
</div>
</EuiResizeObserver>
</div>
</EuiContextMenuPanel>"
`;
156 changes: 125 additions & 31 deletions src/components/context_menu/context_menu_panel.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

/// <reference types="../../../cypress/support"/>

import React from 'react';
import React, { useState } from 'react';

import { EuiPopover } from '../popover';
import { EuiContextMenu } from './context_menu';
Expand Down Expand Up @@ -42,15 +42,6 @@ describe('EuiContextMenuPanel', () => {
cy.focused().should('have.attr', 'class', 'euiContextMenuPanel');
});

it('sets initial focus from `initialFocusedItemIndex`', () => {
cy.mount(
<EuiContextMenuPanel initialFocusedItemIndex={2}>
{children}
</EuiContextMenuPanel>
);
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
});

describe('with `children`', () => {
it('ignores arrow key navigation, which only toggles for `items`', () => {
cy.mount(<EuiContextMenuPanel>{children}</EuiContextMenuPanel>);
Expand All @@ -60,6 +51,20 @@ describe('EuiContextMenuPanel', () => {
});

describe('with `items`', () => {
it('sets initial focus from `initialFocusedItemIndex`', () => {
cy.mount(
<EuiContextMenuPanel items={items} initialFocusedItemIndex={2} />
);
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
});

it('falls back to the panel if given an invalid `focusedItemIndex`', () => {
cy.mount(
<EuiContextMenuPanel items={items} initialFocusedItemIndex={99} />
);
cy.focused().should('have.attr', 'class', 'euiContextMenuPanel');
});

it('focuses and registers any tabbable child as navigable menu items', () => {
cy.mount(
<EuiContextMenuPanel
Expand All @@ -75,6 +80,36 @@ describe('EuiContextMenuPanel', () => {
cy.realPress('{downarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
});

it('correctly re-finds navigable menu items if `items` changes', () => {
const DynanicItemsTest = () => {
const [dynamicItems, setDynamicItems] = useState([
items[0],
items[1],
]);
const appendItems = () => setDynamicItems(items);
return (
<>
<EuiContextMenuPanel items={dynamicItems} />
<button data-test-subj="appendItems" onClick={appendItems}>
Append more items
</button>
</>
);
};
cy.mount(<DynanicItemsTest />);
cy.realPress('{downarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
cy.realPress('{downarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemB');
cy.realPress('{downarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');

cy.get('[data-test-subj="appendItems"]').click();
cy.get('[data-test-subj="itemA"]').click();
cy.realPress('{uparrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
});
});

describe('with `panels`', () => {
Expand Down Expand Up @@ -123,17 +158,66 @@ describe('EuiContextMenuPanel', () => {
});
});

describe('when inside an EuiPopover', () => {
it('reclaims focus from the parent popover panel', () => {
cy.mount(
<EuiPopover isOpen={true} button={<button />}>
<EuiContextMenuPanel items={items} />
describe('within an EuiPopover', () => {
const ContextMenuInPopover: React.FC<any> = ({ children, ...rest }) => {
const [isOpen, setIsOpen] = useState(false);
const closePopover = () => setIsOpen(false);
const openPopover = () => setIsOpen(true);
return (
<EuiPopover
isOpen={isOpen}
closePopover={closePopover}
button={
<button data-test-subj="popoverToggle" onClick={openPopover}>
Toggle popover
</button>
}
{...rest}
>
<EuiContextMenuPanel>
{children}
<button onClick={closePopover}>
Closes popover from context menu
</button>
</EuiContextMenuPanel>
</EuiPopover>
);
cy.wait(400); // EuiPopover's updateFocus() takes ~350ms to run
};

const mountAndOpenPopover = (component = <ContextMenuInPopover />) => {
cy.realMount(component);
cy.get('[data-test-subj="popoverToggle"]').click();
cy.wait(350); // EuiPopover's updateFocus() takes ~350ms to run
};

it('reclaims focus from the parent popover panel', () => {
mountAndOpenPopover();
cy.focused().should('not.have.attr', 'class', 'euiPopover__panel');
cy.focused().should('have.attr', 'class', 'euiContextMenuPanel');
});

it('does not hijack focus from the EuiPopover if `initialFocus` is set', () => {
mountAndOpenPopover(
<ContextMenuInPopover initialFocus="#testInitialFocus">
<input id="testInitialFocus" />
</ContextMenuInPopover>
);
cy.focused().should('not.have.attr', 'class', 'euiContextMenuPanel');
cy.focused().should('have.attr', 'id', 'testInitialFocus');
});

it('restores focus to the toggling button on popover close', () => {
mountAndOpenPopover();
cy.realPress('Tab');
cy.realPress('Enter');
cy.focused().should('have.attr', 'data-test-subj', 'popoverToggle');
});

it('restores focus to the toggling button on popover escape key', () => {
mountAndOpenPopover();
cy.realPress('{esc}');
cy.focused().should('have.attr', 'data-test-subj', 'popoverToggle');
});
});
});

Expand Down Expand Up @@ -208,7 +292,7 @@ describe('EuiContextMenuPanel', () => {
});
});

it('does not lose focus while using left/right arrow navigation between panels', () => {
describe('panels', () => {
const panels = [
{
id: 0,
Expand Down Expand Up @@ -245,21 +329,31 @@ describe('EuiContextMenuPanel', () => {
initialFocusedItemIndex: 0,
},
];
cy.mount(<EuiContextMenu panels={panels} initialPanelId={0} />);
cy.realPress('{downarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
cy.realPress('{rightarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemB');
cy.realPress('{rightarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemC');

// Test extremely rapid left/right arrow usage
cy.repeatRealPress('{leftarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
cy.repeatRealPress('{rightarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
cy.repeatRealPress('{leftarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
it('does not lose focus while using left/right arrow navigation between panels', () => {
cy.mount(<EuiContextMenu panels={panels} initialPanelId={0} />);
cy.realPress('{downarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
cy.realPress('{rightarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemB');
cy.realPress('{rightarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
});

it('does not lose focus when inside an EuiPopover and during rapid left/right arrow usage', () => {
cy.mount(
<EuiPopover isOpen={true} button={<button />}>
<EuiContextMenu panels={panels} initialPanelId={0} />
</EuiPopover>
);
cy.wait(350); // Wait for EuiContextMenuPanel to reclaim focus from popover
cy.realPress('{downarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
cy.repeatRealPress('{rightarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
cy.repeatRealPress('{leftarrow}');
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
});
});
});

Expand Down
Loading