Skip to content

Commit

Permalink
[EuiContextMenuPanel] Fix popover toggle focus restoration issues and…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
Constance authored May 10, 2022
1 parent 1ff1798 commit 723d0ec
Show file tree
Hide file tree
Showing 12 changed files with 269 additions and 394 deletions.
140 changes: 0 additions & 140 deletions src/components/context_menu/__snapshots__/context_menu_panel.test.tsx.snap
View file
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
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

0 comments on commit 723d0ec

Please sign in to comment.