Skip to content
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.

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

fix: ensure esc key does not close modals that are blocking #3033

Merged
merged 4 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion src/Modal/AlertModal.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/Modal/MarketingModal.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/Modal/ModalDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
/**
Expand Down
10 changes: 5 additions & 5 deletions src/Modal/ModalLayer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ function ModalLayer({
return null;
}

const onClickOutside = !isBlocking ? onClose : null;
const handleClose = isBlocking ? null : onClose;

return (
<ModalContextProvider onClose={onClose} isOpen={isOpen} isBlocking={isBlocking}>
Expand All @@ -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}` : '',
)}
>
<ModalContentContainer>
<ModalBackdrop onClick={onClickOutside} />
<ModalBackdrop onClick={handleClose} />
{children}
</ModalContentContainer>
</FocusOn>
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/Modal/ModalPopup.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
55 changes: 47 additions & 8 deletions src/Modal/tests/ModalLayer.test.jsx
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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 (
<focus-on data-testid="focus-on" {...otherProps}>{children}</focus-on>
);
},
}),
}));

function Dialog() {
Expand All @@ -32,6 +33,10 @@ function Dialog() {
}

describe('<ModalLayer />', () => {
beforeEach(() => {
jest.clearAllMocks();
});

describe('when isOpen', () => {
const isOpen = true;
const closeFn = jest.fn();
Expand Down Expand Up @@ -72,30 +77,64 @@ describe('<ModalLayer />', () => {
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(
<ModalLayer isOpen onClose={closeFn} isBlocking={false}>
<Dialog />
</ModalLayer>,
);

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(
<ModalLayer isOpen onClose={closeFn} isBlocking={false}>
<Dialog />
</ModalLayer>,
);
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(
<ModalLayer isOpen onClose={closeFn} isBlocking>
<Dialog />
</ModalLayer>,
);
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(
<ModalLayer isOpen onClose={closeFn} isBlocking>
<Dialog />
</ModalLayer>,
);
const backdrop = screen.getByTestId('modal-backdrop');
await userEvent.click(backdrop);
userEvent.click(backdrop);
expect(closeFn).not.toHaveBeenCalled();
});
});
Expand Down
Loading