From a381182abd82d1ecf9dd2b8d3f327dc88347860b Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 17 Apr 2024 15:33:13 -0400 Subject: [PATCH] fix: ensure esc key does not close modals that are blocking (#3033) --- .github/pull_request_template.md | 2 +- src/Modal/AlertModal.jsx | 2 +- src/Modal/MarketingModal.jsx | 2 +- src/Modal/ModalDialog.jsx | 2 +- src/Modal/ModalLayer.jsx | 10 +++--- src/Modal/ModalPopup.jsx | 2 +- src/Modal/tests/ModalLayer.test.jsx | 55 ++++++++++++++++++++++++----- 7 files changed, 57 insertions(+), 18 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 4a8d9d1edf..620e8d9213 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -14,7 +14,7 @@ Include a direct link to your changes in this PR's deploy preview here (e.g., a * [ ] Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)? * [ ] Were your changes tested in the `example` app? * [ ] Is there adequate test coverage for your changes? -* [ ] Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add `wittjeff` and `adamstankiewicz` as reviewers on this PR. +* [ ] Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please request an a11y review for the PR in the [#wg-paragon](https://openedx.slack.com/archives/C02NR285KV4) Open edX Slack channel. ## Post-merge Checklist diff --git a/src/Modal/AlertModal.jsx b/src/Modal/AlertModal.jsx index b0940a886b..9d7902a606 100644 --- a/src/Modal/AlertModal.jsx +++ b/src/Modal/AlertModal.jsx @@ -41,7 +41,7 @@ AlertModal.propTypes = { title: PropTypes.string.isRequired, /** Is the modal dialog open or closed */ isOpen: PropTypes.bool, - /** Prevent clicking on the backdrop to close the modal */ + /** Prevent clicking on the backdrop or pressing Esc to close the modal */ isBlocking: PropTypes.bool, /** Specifies whether the dialog box should contain 'x' icon button in the top right */ hasCloseButton: PropTypes.bool, diff --git a/src/Modal/MarketingModal.jsx b/src/Modal/MarketingModal.jsx index 59594cc208..e93aa54ab5 100644 --- a/src/Modal/MarketingModal.jsx +++ b/src/Modal/MarketingModal.jsx @@ -36,7 +36,7 @@ MarketingModal.propTypes = { title: PropTypes.string.isRequired, /** Is the modal dialog open or closed */ isOpen: PropTypes.bool, - /** Prevent clicking on the backdrop to close the modal */ + /** Prevent clicking on the backdrop or pressing Esc to close the modal */ isBlocking: PropTypes.bool, /** The close 'x' icon button in the top right corner */ hasCloseButton: PropTypes.bool, diff --git a/src/Modal/ModalDialog.jsx b/src/Modal/ModalDialog.jsx index a63442b598..6814b3c22e 100644 --- a/src/Modal/ModalDialog.jsx +++ b/src/Modal/ModalDialog.jsx @@ -115,7 +115,7 @@ ModalDialog.propTypes = { */ isFullscreenOnMobile: PropTypes.bool, /** - * Prevent clicking on the backdrop to close the modal + * Prevent clicking on the backdrop or pressing Esc to close the modal */ isBlocking: PropTypes.bool, /** diff --git a/src/Modal/ModalLayer.jsx b/src/Modal/ModalLayer.jsx index 61990a41f4..1cc38cf0f4 100644 --- a/src/Modal/ModalLayer.jsx +++ b/src/Modal/ModalLayer.jsx @@ -63,7 +63,7 @@ function ModalLayer({ return null; } - const onClickOutside = !isBlocking ? onClose : null; + const handleClose = isBlocking ? null : onClose; return ( @@ -72,15 +72,15 @@ function ModalLayer({ allowPinchZoom scrollLock enabled={isOpen} - onEscapeKey={onClose} - onClickOutside={onClickOutside} + onEscapeKey={handleClose} + onClickOutside={handleClose} className={classNames( 'pgn__modal-layer', zIndex ? `zindex-${zIndex}` : '', )} > - + {children} @@ -96,7 +96,7 @@ ModalLayer.propTypes = { onClose: PropTypes.func.isRequired, /** Is the modal dialog open or closed */ isOpen: PropTypes.bool.isRequired, - /** Prevent clicking on the backdrop to close the modal */ + /** Prevent clicking on the backdrop or pressing Esc to close the modal */ isBlocking: PropTypes.bool, /** Specifies the z-index of the modal */ zIndex: PropTypes.number, diff --git a/src/Modal/ModalPopup.jsx b/src/Modal/ModalPopup.jsx index ada4f0db22..52bc3adb04 100644 --- a/src/Modal/ModalPopup.jsx +++ b/src/Modal/ModalPopup.jsx @@ -76,7 +76,7 @@ ModalPopup.propTypes = { onClose: PropTypes.func.isRequired, /** Is the modal dialog open or closed */ isOpen: PropTypes.bool.isRequired, - /** Prevent clicking on the backdrop to close the modal */ + /** Prevent clicking on the backdrop or pressing Esc to close the modal */ isBlocking: PropTypes.bool, /** Insert modal into a different location in the DOM */ withPortal: PropTypes.bool, diff --git a/src/Modal/tests/ModalLayer.test.jsx b/src/Modal/tests/ModalLayer.test.jsx index 22cc96f59d..bc93b66013 100644 --- a/src/Modal/tests/ModalLayer.test.jsx +++ b/src/Modal/tests/ModalLayer.test.jsx @@ -1,4 +1,5 @@ import React from 'react'; +import { FocusOn } from 'react-focus-on'; import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; @@ -15,12 +16,12 @@ jest.mock('../Portal', () => function PortalMock(props) { }); jest.mock('react-focus-on', () => ({ - FocusOn: (props) => { + FocusOn: jest.fn().mockImplementation((props) => { const { children, ...otherProps } = props; return ( {children} ); - }, + }), })); function Dialog() { @@ -32,6 +33,10 @@ function Dialog() { } describe('', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + describe('when isOpen', () => { const isOpen = true; const closeFn = jest.fn(); @@ -72,30 +77,64 @@ describe('', () => { expect(dialog).not.toBeInTheDocument(); }); - describe('Backdrop', () => { - it('closes a non-blocking modal layer when clicked', async () => { + describe('Dismiss modal', () => { + it('closes a non-blocking modal layer when backdrop is clicked', () => { const closeFn = jest.fn(); render( , ); - const backdrop = screen.getByTestId('modal-backdrop'); - await userEvent.click(backdrop); + userEvent.click(backdrop); expect(closeFn).toHaveBeenCalled(); }); - it('does not close a blocking modal layer when clicked', async () => { + it('should configure FocusOn to close a non-blocking modal layer when Esc key is pressed', () => { + const closeFn = jest.fn(); + render( + + + , + ); + expect(FocusOn).toHaveBeenCalledWith( + expect.objectContaining({ + onEscapeKey: closeFn, + }), + // note: this 2nd function argument represents the + // `refOrContext` (in this case, the context value + // provided by `ModalContextProvider`). + {}, + ); + }); + + it('should not configure FocusOn to close a blocking modal layer when Esc key is pressed', () => { const closeFn = jest.fn(); render( , ); + expect(FocusOn).toHaveBeenCalledWith( + expect.objectContaining({ + onEscapeKey: null, + }), + // note: this 2nd function argument represents the + // `refOrContext` (in this case, the context value + // provided by `ModalContextProvider`). + {}, + ); + }); + it('does not close a blocking modal layer when backdrop is clicked', () => { + const closeFn = jest.fn(); + render( + + + , + ); const backdrop = screen.getByTestId('modal-backdrop'); - await userEvent.click(backdrop); + userEvent.click(backdrop); expect(closeFn).not.toHaveBeenCalled(); }); });