Skip to content

Commit 2db0d08

Browse files
tay1orjonestay1orjones
authored
fix(modal): update preventCloseOnClickOutside behavior to match the design spec (#20266)
* fix(modal): update preventCloseOnClickOutside logic and tests * chore: remove comments * chore: remove uneeded test story * chore: remove old uneeded tests
1 parent a876c83 commit 2db0d08

File tree

7 files changed

+513
-238
lines changed

7 files changed

+513
-238
lines changed

‎packages/react/src/components/ComposedModal/ComposedModal-test.js‎

Lines changed: 161 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -144,27 +144,6 @@ describe('ComposedModal', () => {
144144
);
145145
});
146146

147-
it('should prevent close on click outside', async () => {
148-
render(
149-
<>
150-
<button type="button">Click me</button>
151-
<ComposedModal open preventCloseOnClickOutside>
152-
<ModalHeader>Modal header</ModalHeader>
153-
<ModalBody>This is the modal body content</ModalBody>
154-
</ComposedModal>
155-
</>
156-
);
157-
expect(screen.getByRole('presentation', { hidden: true })).toHaveClass(
158-
'is-visible'
159-
);
160-
161-
await userEvent.click(screen.getByText('Click me'));
162-
163-
expect(screen.getByRole('presentation', { hidden: true })).toHaveClass(
164-
'is-visible'
165-
);
166-
});
167-
168147
it('should focus selector on open', async () => {
169148
function ComposedModalExample() {
170149
const [isOpen, setIsOpen] = React.useState(false);
@@ -432,38 +411,6 @@ describe('ComposedModal', () => {
432411
expect(onClick).toHaveBeenCalled();
433412
});
434413

435-
it('should close when clicking outside passive composed modal', async () => {
436-
const onClose = jest.fn();
437-
render(
438-
<ComposedModal open onClose={onClose}>
439-
<ModalBody>This is the modal body content</ModalBody>
440-
</ComposedModal>
441-
);
442-
const backgroundLayer = screen.getByRole('presentation');
443-
await userEvent.click(backgroundLayer);
444-
expect(onClose).toHaveBeenCalled();
445-
});
446-
447-
it('should not close when clicking outside non-passive composed modal', async () => {
448-
const onClose = jest.fn();
449-
render(
450-
<>
451-
<button data-testid="outside-button">☀️</button>
452-
<ComposedModal open onClose={onClose}>
453-
<ModalHeader>Header</ModalHeader>
454-
<ModalBody>Body</ModalBody>
455-
<ModalFooter
456-
primaryButtonText="Confirm"
457-
secondaryButtonText="Cancel"
458-
/>
459-
</ComposedModal>
460-
</>
461-
);
462-
463-
await userEvent.click(screen.getByTestId('outside-button'));
464-
expect(onClose).not.toHaveBeenCalled();
465-
});
466-
467414
it('should NOT close when clicked inside dialog window, dragged outside and released mouse button', async () => {
468415
const onClose = jest.fn();
469416
render(
@@ -483,6 +430,167 @@ describe('ComposedModal', () => {
483430
expect(onClose).not.toHaveBeenCalled();
484431
});
485432

433+
describe('close behavior for clicks outside the modal', () => {
434+
describe('passive', () => {
435+
it('should close on outside click by default', async () => {
436+
const onClose = jest.fn();
437+
render(
438+
<ComposedModal open onClose={onClose}>
439+
<ModalHeader>ModalHeader content</ModalHeader>
440+
<ModalBody>ModalBody content</ModalBody>
441+
{/* ModalFooter is omitted, this is what makes it passive */}
442+
</ComposedModal>
443+
);
444+
445+
// The background layer is used here instead of a button outside the
446+
// modal because a real user cannot interact with a button. The
447+
// backround layer is in the way.
448+
const backgroundLayer = screen.getByRole('presentation', {
449+
hidden: true,
450+
});
451+
expect(backgroundLayer).toHaveClass('is-visible');
452+
453+
await userEvent.click(backgroundLayer);
454+
expect(backgroundLayer).not.toHaveClass('is-visible');
455+
expect(onClose).toHaveBeenCalled();
456+
});
457+
it('should not close on outside click when preventCloseOnClickOutside', async () => {
458+
const onClose = jest.fn();
459+
render(
460+
<ComposedModal open onClose={onClose} preventCloseOnClickOutside>
461+
<ModalHeader>ModalHeader content</ModalHeader>
462+
<ModalBody>ModalBody content</ModalBody>
463+
{/* ModalFooter is omitted, this is what makes it passive */}
464+
</ComposedModal>
465+
);
466+
467+
// The background layer is used here instead of a button outside the
468+
// modal because a real user cannot interact with a button. The
469+
// backround layer is in the way.
470+
const backgroundLayer = screen.getByRole('presentation', {
471+
hidden: true,
472+
});
473+
expect(backgroundLayer).toHaveClass('is-visible');
474+
475+
await userEvent.click(backgroundLayer);
476+
expect(backgroundLayer).toHaveClass('is-visible');
477+
expect(onClose).not.toHaveBeenCalled();
478+
});
479+
it('should close on outside click when preventCloseOnClickOutside is explicitly false', async () => {
480+
const onClose = jest.fn();
481+
render(
482+
<ComposedModal
483+
open
484+
onClose={onClose}
485+
preventCloseOnClickOutside={false}>
486+
<ModalHeader>ModalHeader content</ModalHeader>
487+
<ModalBody>ModalBody content</ModalBody>
488+
{/* ModalFooter is omitted, this is what makes it passive */}
489+
</ComposedModal>
490+
);
491+
492+
// The background layer is used here instead of a button outside the
493+
// modal because a real user cannot interact with a button. The
494+
// backround layer is in the way.
495+
const backgroundLayer = screen.getByRole('presentation', {
496+
hidden: true,
497+
});
498+
expect(backgroundLayer).toHaveClass('is-visible');
499+
500+
await userEvent.click(backgroundLayer);
501+
expect(backgroundLayer).not.toHaveClass('is-visible');
502+
expect(onClose).toHaveBeenCalled();
503+
});
504+
});
505+
describe('non-passive', () => {
506+
it('should not close on outside click by default', async () => {
507+
const onClose = jest.fn();
508+
render(
509+
<ComposedModal open onClose={onClose}>
510+
<ModalHeader>ModalHeader content</ModalHeader>
511+
<ModalBody>ModalBody content</ModalBody>
512+
<ModalFooter
513+
primaryButtonText="Confirm"
514+
secondaryButtonText="Cancel"
515+
/>
516+
</ComposedModal>
517+
);
518+
519+
// The background layer is used here instead of a button outside the
520+
// modal because a real user cannot interact with a button. The
521+
// backround layer is in the way.
522+
const backgroundLayer = screen.getByRole('presentation', {
523+
hidden: true,
524+
});
525+
expect(backgroundLayer).toHaveClass('is-visible');
526+
527+
await userEvent.click(backgroundLayer);
528+
expect(backgroundLayer).toHaveClass('is-visible');
529+
expect(onClose).not.toHaveBeenCalled();
530+
});
531+
it('should not close on outside click when preventCloseOnClickOutside', async () => {
532+
const onClose = jest.fn();
533+
render(
534+
<ComposedModal open onClose={onClose} preventCloseOnClickOutside>
535+
<ModalHeader>ModalHeader content</ModalHeader>
536+
<ModalBody>ModalBody content</ModalBody>
537+
<ModalFooter
538+
primaryButtonText="Confirm"
539+
secondaryButtonText="Cancel"
540+
/>
541+
</ComposedModal>
542+
);
543+
544+
// The background layer is used here instead of a button outside the
545+
// modal because a real user cannot interact with a button. The
546+
// backround layer is in the way.
547+
const backgroundLayer = screen.getByRole('presentation', {
548+
hidden: true,
549+
});
550+
expect(backgroundLayer).toHaveClass('is-visible');
551+
552+
await userEvent.click(backgroundLayer);
553+
expect(backgroundLayer).toHaveClass('is-visible');
554+
expect(onClose).not.toHaveBeenCalled();
555+
});
556+
it('should close on outside click when preventCloseOnClickOutside is explicitly false', async () => {
557+
const spy = jest.spyOn(console, 'warn').mockImplementation(() => {});
558+
const onClose = jest.fn();
559+
560+
render(
561+
<ComposedModal
562+
open
563+
onClose={onClose}
564+
preventCloseOnClickOutside={false}>
565+
<ModalHeader>ModalHeader content</ModalHeader>
566+
<ModalBody>ModalBody content</ModalBody>
567+
<ModalFooter
568+
primaryButtonText="Confirm"
569+
secondaryButtonText="Cancel"
570+
/>
571+
</ComposedModal>
572+
);
573+
574+
// The background layer is used here instead of a button outside the
575+
// modal because a real user cannot interact with a button. The
576+
// backround layer is in the way.
577+
const backgroundLayer = screen.getByRole('presentation', {
578+
hidden: true,
579+
});
580+
expect(backgroundLayer).toHaveClass('is-visible');
581+
582+
await userEvent.click(backgroundLayer);
583+
expect(backgroundLayer).not.toHaveClass('is-visible');
584+
expect(onClose).toHaveBeenCalled();
585+
expect(spy).toHaveBeenCalledWith(
586+
'Warning: `<ComposedModal>` prop `preventCloseOnClickOutside` should not be `false` when `<ModalFooter>` is present. Transactional, non-passive Modals should not be dissmissable by clicking outside. See: https://carbondesignsystem.com/components/modal/usage/#transactional-modal'
587+
);
588+
589+
spy.mockRestore();
590+
});
591+
});
592+
});
593+
486594
it('should focus on launcherButtonRef element on close when defined', async () => {
487595
const ComposedModalExample = () => {
488596
const [open, setOpen] = useState(true);

‎packages/react/src/components/ComposedModal/ComposedModal.mdx‎

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
import { Story, ArgTypes, Canvas, Unstyled, Meta } from '@storybook/addon-docs/blocks';
1+
import {
2+
Story,
3+
ArgTypes,
4+
Canvas,
5+
Unstyled,
6+
Meta,
7+
} from '@storybook/addon-docs/blocks';
28
import ComposedModal from '../ComposedModal';
39
import { InlineNotification } from '../Notification';
410
import CodeSnippet from '../CodeSnippet';
@@ -16,13 +22,15 @@ import { stackblitzPrefillConfig } from '../../../previewer/codePreviewer';
1622
&nbsp;|&nbsp;
1723
[Accessibility](https://www.carbondesignsystem.com/components/modal/accessibility)
1824

19-
{/* <!-- START doctoc generated TOC please keep comment here to allow auto update --> <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE --> */}
25+
{/* <!-- START doctoc generated TOC please keep comment here to allow auto update --> */}
26+
{/* <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE --> */}
2027

2128
## Table of Contents
2229

2330
- [Overview](#overview)
2431
- [Component API](#component-api)
2532
- [Opening/closing modal](#openingclosing-modal)
33+
- [`preventCloseOnClickOutside`](#preventcloseonclickoutside)
2634
- [Modal sizes](#modal-sizes)
2735
- [Alignment](#alignment)
2836
- [Overflow content](#overflow-content)
@@ -116,29 +124,35 @@ const ModalStateManager = ({
116124
kind="warning"
117125
title="Warning"
118126
className="sb-notification">
119-
<CodeSnippet type="inline" hideCopyButton>Modal</CodeSnippet>
127+
<CodeSnippet type="inline" hideCopyButton>
128+
Modal
129+
</CodeSnippet>
120130
and
121-
<CodeSnippet type="inline" hideCopyButton>ComposedModal</CodeSnippet>
131+
<CodeSnippet type="inline" hideCopyButton>
132+
ComposedModal
133+
</CodeSnippet>
122134
have to be put at the top level in a React tree. The easiest way to ensure
123135
that is using a React portal, as shown in the example above.
124136
</InlineNotification>
125137
</Unstyled>
126-
<br />
127-
<Unstyled>
128-
<InlineNotification
129-
kind="info"
130-
title="Outside click behavior"
131-
className="sb-notification">
132-
Passive modals can be closed by clicking outside by default. Non-passive
133-
modals cannot be dismissed this way, even if{' '}
134-
<CodeSnippet type="inline" hideCopyButton>preventCloseOnClickOutside</CodeSnippet>{' '}
135-
is set to{' '}
136-
<CodeSnippet type="inline" hideCopyButton>false</CodeSnippet>.
137-
</InlineNotification>
138-
</Unstyled>
139138

140139
{/* <!-- prettier-ignore-end --> */}
141140

141+
### `preventCloseOnClickOutside`
142+
143+
This prop controls what happens when a user clicks outside the bounds of the
144+
modal. When true, clicks outside the modal will not trigger close. When false,
145+
clicks outside the modal will trigger close. When left undefined, the modal type
146+
determines the default behavior.
147+
148+
Passive modals by default close on outside clicks. For transactional,
149+
non-passive modals that include `ModalFooter`, the
150+
[design spec for Modal](https://carbondesignsystem.com/components/modal/usage/#transactional-modal)
151+
calls for these to not be dissmissable via outside clicks.
152+
153+
Setting `preventCloseOnClickOutside` to `false` to override the default for
154+
non-passive modals is allowed but not recommended, and will trigger a warning.
155+
142156
## Modal sizes
143157

144158
There are four responsive
@@ -246,9 +260,13 @@ With `<ComposedModal>`, you can control the buttons with the following code.
246260
title="Warning"
247261
className="sb-notification">
248262
As the instructions for the three buttons imply,
249-
<CodeSnippet type="inline" hideCopyButton>ModalFooter</CodeSnippet>
263+
<CodeSnippet type="inline" hideCopyButton>
264+
ModalFooter
265+
</CodeSnippet>
250266
is flexible with the buttons you render using
251-
<CodeSnippet type="inline" hideCopyButton>Button</CodeSnippet>
267+
<CodeSnippet type="inline" hideCopyButton>
268+
Button
269+
</CodeSnippet>
252270
components. In this case, your application code is responsible for handling
253271
button actions, such as closing the modal.
254272
</InlineNotification>

0 commit comments

Comments
 (0)