Skip to content

Commit 2ee5c5d

Browse files
authored
[PopoverOverlay] Escape only closes popover if event.target is a descendant (#7332)
### WHY are these changes introduced? Fixes #7329 When multiple Polaris Popovers are open and pressing `Escape`, then all Popovers close. This creates conflict in moving focus to activator of a Popover, especially if a Popover is a child of another Popover. In the case of a parent child relationship, when both Popovers close, then the child Popover no longer has the activator available within the DOM to move focus to, so focus goes to the `body`. ### WHAT is this pull request doing? PopoverOverlay has a global listener for `Escape` key presses, so all Popovers respond to the `Escape` key presses and attempt to close. This pull request will only close the Popover where the event target of the keypress is either a descendant of the Popover or the associated activator. As a result, Popovers will iteratively close based on the presence of focus. <details> <summary>Demo of fix</summary> <img src="https://user-images.githubusercontent.com/86790511/193919419-1dc57b3f-2ee6-4062-9670-d3f24a0af55a.gif" alt="Demo of fix"> </details> ### How to 🎩 Validate `Escape` Popover behavior within the following codesandbox, which has a snapshot of Polaris with the change in this PR. https://codesandbox.io/s/polaris-double-popover-escape-with-fix-0od2c3 ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
1 parent 7147693 commit 2ee5c5d

File tree

4 files changed

+105
-48
lines changed

4 files changed

+105
-48
lines changed

.changeset/violet-rice-appear.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/polaris': patch
3+
---
4+
5+
`PopoverOverlay` closes focused `Popover` when pressing `Escape`

polaris-react/src/components/Combobox/tests/Combobox.test.tsx

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -302,8 +302,14 @@ describe('<Combobox />', () => {
302302

303303
triggerFocus(combobox);
304304

305+
const target = combobox.find(TextField)!.find('input')!.domNode;
306+
const keyupEvent = new KeyboardEvent('keyup', {
307+
keyCode: Key.Escape,
308+
bubbles: true,
309+
});
310+
305311
combobox.act(() => {
306-
dispatchKeyup(Key.Escape);
312+
target?.dispatchEvent(keyupEvent);
307313
});
308314

309315
expect(combobox).toContainReactComponent(Popover, {
@@ -433,8 +439,3 @@ function triggerOptionSelected(combobox: any) {
433439
}
434440

435441
function noop() {}
436-
437-
function dispatchKeyup(key: Key) {
438-
const event: KeyboardEventInit & {keyCode: Key} = {keyCode: key};
439-
document.dispatchEvent(new KeyboardEvent('keyup', event));
440-
}

polaris-react/src/components/Popover/components/PopoverOverlay/PopoverOverlay.tsx

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,8 +299,19 @@ export class PopoverOverlay extends PureComponent<PopoverOverlayProps, State> {
299299
this.props.onClose(PopoverCloseSource.ScrollOut);
300300
};
301301

302-
private handleEscape = () => {
303-
this.props.onClose(PopoverCloseSource.EscapeKeypress);
302+
private handleEscape = (event: Event) => {
303+
const target = event.target as HTMLElement;
304+
const {
305+
contentNode,
306+
props: {activator},
307+
} = this;
308+
const composedPath = event.composedPath();
309+
const wasDescendant = wasContentNodeDescendant(composedPath, contentNode);
310+
const isActivatorDescendant = nodeContainsDescendant(activator, target);
311+
312+
if (wasDescendant || isActivatorDescendant) {
313+
this.props.onClose(PopoverCloseSource.EscapeKeypress);
314+
}
304315
};
305316

306317
private handleFocusFirstItem = () => {

polaris-react/src/components/Popover/components/PopoverOverlay/tests/PopoverOverlay.test.tsx

Lines changed: 80 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -8,29 +8,10 @@ import {PositionedOverlay} from '../../../../PositionedOverlay';
88
import {PopoverOverlay} from '../PopoverOverlay';
99
import {Popover} from '../../../Popover';
1010

11-
interface HandlerMap {
12-
[eventName: string]: (event: any) => void;
13-
}
14-
15-
const listenerMap: HandlerMap = {};
16-
1711
describe('<PopoverOverlay />', () => {
18-
let addEventListener: jest.SpyInstance;
19-
let removeEventListener: jest.SpyInstance;
20-
2112
let rafSpy: jest.SpyInstance;
2213

2314
beforeEach(() => {
24-
addEventListener = jest.spyOn(document, 'addEventListener');
25-
addEventListener.mockImplementation((event, callback) => {
26-
listenerMap[event] = callback;
27-
});
28-
29-
removeEventListener = jest.spyOn(document, 'removeEventListener');
30-
removeEventListener.mockImplementation((event) => {
31-
listenerMap[event] = noop;
32-
});
33-
3415
rafSpy = jest.spyOn(window, 'requestAnimationFrame');
3516
rafSpy.mockImplementation((callback) => callback());
3617
});
@@ -40,9 +21,6 @@ describe('<PopoverOverlay />', () => {
4021
(document.activeElement as HTMLElement).blur();
4122
}
4223

43-
addEventListener.mockRestore();
44-
removeEventListener.mockRestore();
45-
4624
rafSpy.mockRestore();
4725
});
4826

@@ -204,24 +182,6 @@ describe('<PopoverOverlay />', () => {
204182
});
205183
});
206184

207-
it('calls the onClose callback when the escape key is pressed', () => {
208-
const spy = jest.fn();
209-
210-
mountWithApp(
211-
<PopoverOverlay
212-
active
213-
id="PopoverOverlay-1"
214-
activator={activator}
215-
onClose={spy}
216-
>
217-
{children}
218-
</PopoverOverlay>,
219-
);
220-
221-
listenerMap.keyup({keyCode: Key.Escape});
222-
expect(spy).toHaveBeenCalledTimes(1);
223-
});
224-
225185
it('does not call the onClose callback when a descendent HTMLElement is clicked', () => {
226186
const spy = jest.fn();
227187

@@ -316,6 +276,77 @@ describe('<PopoverOverlay />', () => {
316276
expect(spy).not.toHaveBeenCalled();
317277
});
318278

279+
describe('when the Escape key is pressed', () => {
280+
it('calls the onClose callback if event target is descendant', () => {
281+
const spy = jest.fn();
282+
283+
const popoverOverlay = mountWithApp(
284+
<PopoverOverlay
285+
active
286+
id="PopoverOverlay-1"
287+
activator={activator}
288+
onClose={spy}
289+
>
290+
<TextField
291+
label="Store name"
292+
value="Click me"
293+
onChange={() => {}}
294+
autoComplete="off"
295+
/>
296+
</PopoverOverlay>,
297+
);
298+
299+
const target = popoverOverlay.find(TextField)!.find('input')!.domNode!;
300+
triggerEscape(target);
301+
302+
expect(spy).toHaveBeenCalledTimes(1);
303+
});
304+
305+
it('calls the onClose callback if event target is activator descendant', () => {
306+
const spy = jest.fn();
307+
308+
const popoverOverlay = mountWithApp(
309+
<PopoverOverlay
310+
active
311+
id="PopoverOverlay-1"
312+
activator={activator}
313+
onClose={spy}
314+
>
315+
<TextField
316+
label="Store name"
317+
value="Click me"
318+
onChange={() => {}}
319+
autoComplete="off"
320+
/>
321+
</PopoverOverlay>,
322+
);
323+
324+
const target = popoverOverlay.find(TextField)!.find('input')!.domNode!;
325+
triggerEscape(target);
326+
327+
expect(spy).toHaveBeenCalledTimes(1);
328+
});
329+
330+
it('does not call the onClose callback if event target is not descendant or activator descendant', () => {
331+
const spy = jest.fn();
332+
333+
mountWithApp(
334+
<PopoverOverlay
335+
active
336+
id="PopoverOverlay-1"
337+
activator={activator}
338+
onClose={spy}
339+
>
340+
{children}
341+
</PopoverOverlay>,
342+
);
343+
344+
triggerEscape();
345+
346+
expect(spy).toHaveBeenCalledTimes(0);
347+
});
348+
});
349+
319350
describe('deleting descendant elements', () => {
320351
const DeleteButton = () => {
321352
const [show, setShow] = React.useState(true);
@@ -567,3 +598,12 @@ describe('<PopoverOverlay />', () => {
567598
});
568599

569600
function noop() {}
601+
602+
function triggerEscape(target?: Element) {
603+
const keyupEvent = new KeyboardEvent('keyup', {
604+
keyCode: Key.Escape,
605+
bubbles: true,
606+
});
607+
608+
(target || document).dispatchEvent(keyupEvent);
609+
}

0 commit comments

Comments
 (0)