Skip to content

Commit 723d0ec

Browse files
author
Constance
authored
[EuiContextMenuPanel] Fix popover toggle focus restoration issues and 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
1 parent 1ff1798 commit 723d0ec

12 files changed

Lines changed: 269 additions & 394 deletions

File tree

‎src/components/context_menu/__snapshots__/context_menu_panel.test.tsx.snap‎

Lines changed: 0 additions & 140 deletions
Original file line numberDiff line numberDiff line change
@@ -130,143 +130,3 @@ exports[`EuiContextMenuPanel props transitionDirection previous with transitionT
130130
<div />
131131
</div>
132132
`;
133-
134-
exports[`EuiContextMenuPanel updating items and content updates to items should not re-render if any items's watchedItemProps did not change 1`] = `
135-
"<EuiContextMenuPanel items={{...}}>
136-
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
137-
<EuiResizeObserver onResize={[Function: onResize]}>
138-
<div>
139-
<EuiContextMenuItem data-counter={0}>
140-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={0}>
141-
<span className=\\"euiContextMenu__itemLayout\\">
142-
<span className=\\"euiContextMenuItem__text\\">
143-
Option A
144-
</span>
145-
</span>
146-
</button>
147-
</EuiContextMenuItem>
148-
<EuiContextMenuItem data-counter={1}>
149-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={1}>
150-
<span className=\\"euiContextMenu__itemLayout\\">
151-
<span className=\\"euiContextMenuItem__text\\">
152-
Option B
153-
</span>
154-
</span>
155-
</button>
156-
</EuiContextMenuItem>
157-
</div>
158-
</EuiResizeObserver>
159-
</div>
160-
</EuiContextMenuPanel>"
161-
`;
162-
163-
exports[`EuiContextMenuPanel updating items and content updates to items should not re-render if any items's watchedItemProps did not change 2`] = `
164-
"<EuiContextMenuPanel items={{...}}>
165-
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
166-
<EuiResizeObserver onResize={[Function: onResize]}>
167-
<div>
168-
<EuiContextMenuItem data-counter={0}>
169-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={0}>
170-
<span className=\\"euiContextMenu__itemLayout\\">
171-
<span className=\\"euiContextMenuItem__text\\">
172-
Option A
173-
</span>
174-
</span>
175-
</button>
176-
</EuiContextMenuItem>
177-
<EuiContextMenuItem data-counter={1}>
178-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={1}>
179-
<span className=\\"euiContextMenu__itemLayout\\">
180-
<span className=\\"euiContextMenuItem__text\\">
181-
Option B
182-
</span>
183-
</span>
184-
</button>
185-
</EuiContextMenuItem>
186-
</div>
187-
</EuiResizeObserver>
188-
</div>
189-
</EuiContextMenuPanel>"
190-
`;
191-
192-
exports[`EuiContextMenuPanel updating items and content updates to items should re-render at all times when children exists 1`] = `
193-
"<EuiContextMenuPanel items={{...}}>
194-
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
195-
<EuiResizeObserver onResize={[Function: onResize]}>
196-
<div>
197-
Hello World
198-
</div>
199-
</EuiResizeObserver>
200-
</div>
201-
</EuiContextMenuPanel>"
202-
`;
203-
204-
exports[`EuiContextMenuPanel updating items and content updates to items should re-render at all times when children exists 2`] = `
205-
"<EuiContextMenuPanel items={{...}}>
206-
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
207-
<EuiResizeObserver onResize={[Function: onResize]}>
208-
<div>
209-
More Salutations
210-
</div>
211-
</EuiResizeObserver>
212-
</div>
213-
</EuiContextMenuPanel>"
214-
`;
215-
216-
exports[`EuiContextMenuPanel updating items and content updates to items should re-render if any items's watchedItemProps did change 1`] = `
217-
"<EuiContextMenuPanel watchedItemProps={{...}} items={{...}}>
218-
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
219-
<EuiResizeObserver onResize={[Function: onResize]}>
220-
<div>
221-
<EuiContextMenuItem data-counter={0}>
222-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={0}>
223-
<span className=\\"euiContextMenu__itemLayout\\">
224-
<span className=\\"euiContextMenuItem__text\\">
225-
Option A
226-
</span>
227-
</span>
228-
</button>
229-
</EuiContextMenuItem>
230-
<EuiContextMenuItem data-counter={1}>
231-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={1}>
232-
<span className=\\"euiContextMenu__itemLayout\\">
233-
<span className=\\"euiContextMenuItem__text\\">
234-
Option B
235-
</span>
236-
</span>
237-
</button>
238-
</EuiContextMenuItem>
239-
</div>
240-
</EuiResizeObserver>
241-
</div>
242-
</EuiContextMenuPanel>"
243-
`;
244-
245-
exports[`EuiContextMenuPanel updating items and content updates to items should re-render if any items's watchedItemProps did change 2`] = `
246-
"<EuiContextMenuPanel watchedItemProps={{...}} items={{...}}>
247-
<div className=\\"euiContextMenuPanel\\" onKeyDown={[Function]} tabIndex={-1} onAnimationEnd={[Function]}>
248-
<EuiResizeObserver onResize={[Function: onResize]}>
249-
<div>
250-
<EuiContextMenuItem data-counter={2}>
251-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={2}>
252-
<span className=\\"euiContextMenu__itemLayout\\">
253-
<span className=\\"euiContextMenuItem__text\\">
254-
Option A
255-
</span>
256-
</span>
257-
</button>
258-
</EuiContextMenuItem>
259-
<EuiContextMenuItem data-counter={3}>
260-
<button disabled={false} className=\\"euiContextMenuItem\\" type=\\"button\\" data-counter={3}>
261-
<span className=\\"euiContextMenu__itemLayout\\">
262-
<span className=\\"euiContextMenuItem__text\\">
263-
Option B
264-
</span>
265-
</span>
266-
</button>
267-
</EuiContextMenuItem>
268-
</div>
269-
</EuiResizeObserver>
270-
</div>
271-
</EuiContextMenuPanel>"
272-
`;

‎src/components/context_menu/context_menu_panel.spec.tsx‎

Lines changed: 125 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

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

11-
import React from 'react';
11+
import React, { useState } from 'react';
1212

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

45-
it('sets initial focus from `initialFocusedItemIndex`', () => {
46-
cy.mount(
47-
<EuiContextMenuPanel initialFocusedItemIndex={2}>
48-
{children}
49-
</EuiContextMenuPanel>
50-
);
51-
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
52-
});
53-
5445
describe('with `children`', () => {
5546
it('ignores arrow key navigation, which only toggles for `items`', () => {
5647
cy.mount(<EuiContextMenuPanel>{children}</EuiContextMenuPanel>);
@@ -60,6 +51,20 @@ describe('EuiContextMenuPanel', () => {
6051
});
6152

6253
describe('with `items`', () => {
54+
it('sets initial focus from `initialFocusedItemIndex`', () => {
55+
cy.mount(
56+
<EuiContextMenuPanel items={items} initialFocusedItemIndex={2} />
57+
);
58+
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
59+
});
60+
61+
it('falls back to the panel if given an invalid `focusedItemIndex`', () => {
62+
cy.mount(
63+
<EuiContextMenuPanel items={items} initialFocusedItemIndex={99} />
64+
);
65+
cy.focused().should('have.attr', 'class', 'euiContextMenuPanel');
66+
});
67+
6368
it('focuses and registers any tabbable child as navigable menu items', () => {
6469
cy.mount(
6570
<EuiContextMenuPanel
@@ -75,6 +80,36 @@ describe('EuiContextMenuPanel', () => {
7580
cy.realPress('{downarrow}');
7681
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
7782
});
83+
84+
it('correctly re-finds navigable menu items if `items` changes', () => {
85+
const DynanicItemsTest = () => {
86+
const [dynamicItems, setDynamicItems] = useState([
87+
items[0],
88+
items[1],
89+
]);
90+
const appendItems = () => setDynamicItems(items);
91+
return (
92+
<>
93+
<EuiContextMenuPanel items={dynamicItems} />
94+
<button data-test-subj="appendItems" onClick={appendItems}>
95+
Append more items
96+
</button>
97+
</>
98+
);
99+
};
100+
cy.mount(<DynanicItemsTest />);
101+
cy.realPress('{downarrow}');
102+
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
103+
cy.realPress('{downarrow}');
104+
cy.focused().should('have.attr', 'data-test-subj', 'itemB');
105+
cy.realPress('{downarrow}');
106+
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
107+
108+
cy.get('[data-test-subj="appendItems"]').click();
109+
cy.get('[data-test-subj="itemA"]').click();
110+
cy.realPress('{uparrow}');
111+
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
112+
});
78113
});
79114

80115
describe('with `panels`', () => {
@@ -123,17 +158,66 @@ describe('EuiContextMenuPanel', () => {
123158
});
124159
});
125160

126-
describe('when inside an EuiPopover', () => {
127-
it('reclaims focus from the parent popover panel', () => {
128-
cy.mount(
129-
<EuiPopover isOpen={true} button={<button />}>
130-
<EuiContextMenuPanel items={items} />
161+
describe('within an EuiPopover', () => {
162+
const ContextMenuInPopover: React.FC<any> = ({ children, ...rest }) => {
163+
const [isOpen, setIsOpen] = useState(false);
164+
const closePopover = () => setIsOpen(false);
165+
const openPopover = () => setIsOpen(true);
166+
return (
167+
<EuiPopover
168+
isOpen={isOpen}
169+
closePopover={closePopover}
170+
button={
171+
<button data-test-subj="popoverToggle" onClick={openPopover}>
172+
Toggle popover
173+
</button>
174+
}
175+
{...rest}
176+
>
177+
<EuiContextMenuPanel>
178+
{children}
179+
<button onClick={closePopover}>
180+
Closes popover from context menu
181+
</button>
182+
</EuiContextMenuPanel>
131183
</EuiPopover>
132184
);
133-
cy.wait(400); // EuiPopover's updateFocus() takes ~350ms to run
185+
};
186+
187+
const mountAndOpenPopover = (component = <ContextMenuInPopover />) => {
188+
cy.realMount(component);
189+
cy.get('[data-test-subj="popoverToggle"]').click();
190+
cy.wait(350); // EuiPopover's updateFocus() takes ~350ms to run
191+
};
192+
193+
it('reclaims focus from the parent popover panel', () => {
194+
mountAndOpenPopover();
134195
cy.focused().should('not.have.attr', 'class', 'euiPopover__panel');
135196
cy.focused().should('have.attr', 'class', 'euiContextMenuPanel');
136197
});
198+
199+
it('does not hijack focus from the EuiPopover if `initialFocus` is set', () => {
200+
mountAndOpenPopover(
201+
<ContextMenuInPopover initialFocus="#testInitialFocus">
202+
<input id="testInitialFocus" />
203+
</ContextMenuInPopover>
204+
);
205+
cy.focused().should('not.have.attr', 'class', 'euiContextMenuPanel');
206+
cy.focused().should('have.attr', 'id', 'testInitialFocus');
207+
});
208+
209+
it('restores focus to the toggling button on popover close', () => {
210+
mountAndOpenPopover();
211+
cy.realPress('Tab');
212+
cy.realPress('Enter');
213+
cy.focused().should('have.attr', 'data-test-subj', 'popoverToggle');
214+
});
215+
216+
it('restores focus to the toggling button on popover escape key', () => {
217+
mountAndOpenPopover();
218+
cy.realPress('{esc}');
219+
cy.focused().should('have.attr', 'data-test-subj', 'popoverToggle');
220+
});
137221
});
138222
});
139223

@@ -208,7 +292,7 @@ describe('EuiContextMenuPanel', () => {
208292
});
209293
});
210294

211-
it('does not lose focus while using left/right arrow navigation between panels', () => {
295+
describe('panels', () => {
212296
const panels = [
213297
{
214298
id: 0,
@@ -245,21 +329,31 @@ describe('EuiContextMenuPanel', () => {
245329
initialFocusedItemIndex: 0,
246330
},
247331
];
248-
cy.mount(<EuiContextMenu panels={panels} initialPanelId={0} />);
249-
cy.realPress('{downarrow}');
250-
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
251-
cy.realPress('{rightarrow}');
252-
cy.focused().should('have.attr', 'data-test-subj', 'itemB');
253-
cy.realPress('{rightarrow}');
254-
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
255332

256-
// Test extremely rapid left/right arrow usage
257-
cy.repeatRealPress('{leftarrow}');
258-
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
259-
cy.repeatRealPress('{rightarrow}');
260-
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
261-
cy.repeatRealPress('{leftarrow}');
262-
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
333+
it('does not lose focus while using left/right arrow navigation between panels', () => {
334+
cy.mount(<EuiContextMenu panels={panels} initialPanelId={0} />);
335+
cy.realPress('{downarrow}');
336+
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
337+
cy.realPress('{rightarrow}');
338+
cy.focused().should('have.attr', 'data-test-subj', 'itemB');
339+
cy.realPress('{rightarrow}');
340+
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
341+
});
342+
343+
it('does not lose focus when inside an EuiPopover and during rapid left/right arrow usage', () => {
344+
cy.mount(
345+
<EuiPopover isOpen={true} button={<button />}>
346+
<EuiContextMenu panels={panels} initialPanelId={0} />
347+
</EuiPopover>
348+
);
349+
cy.wait(350); // Wait for EuiContextMenuPanel to reclaim focus from popover
350+
cy.realPress('{downarrow}');
351+
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
352+
cy.repeatRealPress('{rightarrow}');
353+
cy.focused().should('have.attr', 'data-test-subj', 'itemC');
354+
cy.repeatRealPress('{leftarrow}');
355+
cy.focused().should('have.attr', 'data-test-subj', 'itemA');
356+
});
263357
});
264358
});
265359

0 commit comments

Comments
 (0)