Skip to content

Commit 85f04bf

Browse files
tlabajtlabaj
committed
fix(Modal): Fixes various bugs in the Modal and deprecated Modal
1 parent a382cd6 commit 85f04bf

File tree

6 files changed

+107
-38
lines changed

6 files changed

+107
-38
lines changed

‎packages/react-core/src/components/Modal/Modal.tsx‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -60,14 +60,14 @@ export enum ModalVariant {
6060
}
6161

6262
interface ModalState {
63-
container: HTMLElement;
6463
ouiaStateId: string;
6564
}
6665

6766
class Modal extends React.Component<ModalProps, ModalState> {
6867
static displayName = 'Modal';
6968
static currentId = 0;
7069
boxId = '';
70+
backdropId = '';
7171

7272
static defaultProps: PickOptional<ModalProps> = {
7373
isOpen: false,
@@ -80,10 +80,11 @@ class Modal extends React.Component<ModalProps, ModalState> {
8080
constructor(props: ModalProps) {
8181
super(props);
8282
const boxIdNum = Modal.currentId++;
83+
const backdropId = boxIdNum + 1;
8384
this.boxId = props.id || `pf-modal-part-${boxIdNum}`;
85+
this.backdropId = `pf-modal-part-${backdropId}`;
8486

8587
this.state = {
86-
container: undefined,
8788
ouiaStateId: getDefaultOUIAId(Modal.displayName, props.variant)
8889
};
8990
}
@@ -107,7 +108,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
107108
const target: HTMLElement = this.getElement(appendTo);
108109
const bodyChildren = target.children;
109110
for (const child of Array.from(bodyChildren)) {
110-
if (child !== this.state.container) {
111+
if (child.id !== this.backdropId) {
111112
hide ? child.setAttribute('aria-hidden', '' + hide) : child.removeAttribute('aria-hidden');
112113
}
113114
}
@@ -118,44 +119,38 @@ class Modal extends React.Component<ModalProps, ModalState> {
118119
componentDidMount() {
119120
const { appendTo } = this.props;
120121
const target: HTMLElement = this.getElement(appendTo);
121-
const container = document.createElement('div');
122-
this.setState({ container });
123-
target.appendChild(container);
124122
target.addEventListener('keydown', this.handleEscKeyClick, false);
125123

126124
if (this.props.isOpen) {
127125
target.classList.add(css(styles.backdropOpen));
128-
} else {
129-
target.classList.remove(css(styles.backdropOpen));
126+
this.toggleSiblingsFromScreenReaders(true);
130127
}
131128
}
132129

133-
componentDidUpdate() {
130+
componentDidUpdate(prevProps: ModalProps) {
134131
const { appendTo } = this.props;
135132
const target: HTMLElement = this.getElement(appendTo);
136133
if (this.props.isOpen) {
137134
target.classList.add(css(styles.backdropOpen));
138135
this.toggleSiblingsFromScreenReaders(true);
139136
} else {
140-
target.classList.remove(css(styles.backdropOpen));
141-
this.toggleSiblingsFromScreenReaders(false);
137+
if (prevProps.isOpen !== this.props.isOpen) {
138+
target.classList.remove(css(styles.backdropOpen));
139+
this.toggleSiblingsFromScreenReaders(false);
140+
}
142141
}
143142
}
144143

145144
componentWillUnmount() {
146145
const { appendTo } = this.props;
147146
const target: HTMLElement = this.getElement(appendTo);
148-
if (this.state.container) {
149-
target.removeChild(this.state.container);
150-
}
151147
target.removeEventListener('keydown', this.handleEscKeyClick, false);
152148
target.classList.remove(css(styles.backdropOpen));
153149
this.toggleSiblingsFromScreenReaders(false);
154150
}
155151

156152
render() {
157153
const {
158-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
159154
appendTo,
160155
// eslint-disable-next-line @typescript-eslint/no-unused-vars
161156
onEscapePress,
@@ -168,15 +163,15 @@ class Modal extends React.Component<ModalProps, ModalState> {
168163
elementToFocus,
169164
...props
170165
} = this.props;
171-
const { container } = this.state;
172166

173-
if (!canUseDOM || !container) {
167+
if (!canUseDOM || !this.getElement(appendTo)) {
174168
return null;
175169
}
176170

177171
return ReactDOM.createPortal(
178172
<ModalContent
179173
boxId={this.boxId}
174+
backdropId={this.backdropId}
180175
aria-label={ariaLabel}
181176
aria-describedby={ariaDescribedby}
182177
aria-labelledby={ariaLabelledby}
@@ -187,7 +182,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
187182
backdropClassName={props.backdropClassName}
188183
{...props}
189184
/>,
190-
container
185+
this.getElement(appendTo)
191186
) as React.ReactElement;
192187
}
193188
}

‎packages/react-core/src/components/Modal/ModalContent.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ export interface ModalContentProps extends OUIAProps {
1616
'aria-labelledby'?: string;
1717
/** Id of the modal box container. */
1818
boxId: string;
19+
/** Id of the backdrop. */
20+
backdropId?: string;
1921
/** Content rendered inside the modal. */
2022
children: React.ReactNode;
2123
/** Additional classes added to the modal box. */
@@ -63,6 +65,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
6365
width,
6466
maxWidth,
6567
boxId,
68+
backdropId,
6669
disableFocusTrap = false,
6770
ouiaId,
6871
ouiaSafe = true,
@@ -109,7 +112,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
109112
</ModalBox>
110113
);
111114
return (
112-
<Backdrop className={css(backdropClassName)}>
115+
<Backdrop className={css(backdropClassName)} id={backdropId}>
113116
<FocusTrap
114117
active={!disableFocusTrap}
115118
focusTrapOptions={{

‎packages/react-core/src/components/Modal/__tests__/Modal.test.tsx‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import * as React from 'react';
22

33
import { render, screen } from '@testing-library/react';
44
import userEvent from '@testing-library/user-event';
5+
import '@testing-library/jest-dom';
56

67
import { css } from '../../../../../react-styles/dist/js';
78
import styles from '@patternfly/react-styles/css/components/Backdrop/backdrop';
@@ -38,6 +39,32 @@ const ModalWithSiblings = () => {
3839
);
3940
};
4041

42+
const ModalWithAdjacentModal = () => {
43+
const [isOpen, setIsOpen] = React.useState(true);
44+
const [isModalMounted, setIsModalMounted] = React.useState(true);
45+
const modalProps = { ...props, isOpen, appendTo: target, onClose: () => setIsOpen(false) };
46+
47+
return (
48+
<>
49+
<aside>Aside sibling</aside>
50+
<article>Section sibling</article>
51+
{isModalMounted && (
52+
<>
53+
<Modal {...modalProps}>
54+
<button onClick={() => setIsModalMounted(false)}>Unmount Modal</button>
55+
</Modal>
56+
<Modal isOpen={false} onClose={() => {}}>
57+
Modal closed for test
58+
</Modal>
59+
<Modal isOpen={false} onClose={() => {}}>
60+
modal closed for test
61+
</Modal>
62+
</>
63+
)}
64+
</>
65+
);
66+
};
67+
4168
describe('Modal', () => {
4269
test('Modal creates a container element once for div', () => {
4370
render(<Modal {...props} />);
@@ -128,4 +155,22 @@ describe('Modal', () => {
128155
expect(asideSibling).not.toHaveAttribute('aria-hidden');
129156
expect(articleSibling).not.toHaveAttribute('aria-hidden');
130157
});
158+
159+
test('modal siblings have the aria-hidden attribute when it has adjacent modals', async () => {
160+
const user = userEvent.setup();
161+
162+
render(<ModalWithAdjacentModal />, { container: document.body.appendChild(target) });
163+
164+
const asideSibling = screen.getByRole('complementary', { hidden: true });
165+
const articleSibling = screen.getByRole('article', { hidden: true });
166+
const unmountButton = screen.getByRole('button', { name: 'Unmount Modal' });
167+
168+
expect(asideSibling).toHaveAttribute('aria-hidden');
169+
expect(articleSibling).toHaveAttribute('aria-hidden');
170+
171+
await user.click(unmountButton);
172+
173+
expect(asideSibling).not.toHaveAttribute('aria-hidden');
174+
expect(articleSibling).not.toHaveAttribute('aria-hidden');
175+
});
131176
});

‎packages/react-core/src/deprecated/components/Modal/Modal.tsx‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@ export enum ModalVariant {
9292
}
9393

9494
interface ModalState {
95-
container: HTMLElement;
9695
ouiaStateId: string;
9796
}
9897

@@ -102,6 +101,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
102101
boxId = '';
103102
labelId = '';
104103
descriptorId = '';
104+
backdropId = '';
105105

106106
static defaultProps: PickOptional<ModalProps> = {
107107
className: '',
@@ -128,12 +128,13 @@ class Modal extends React.Component<ModalProps, ModalState> {
128128
const boxIdNum = Modal.currentId++;
129129
const labelIdNum = boxIdNum + 1;
130130
const descriptorIdNum = boxIdNum + 2;
131+
const backdropIdNum = boxIdNum + 3;
131132
this.boxId = props.id || `pf-modal-part-${boxIdNum}`;
132133
this.labelId = `pf-modal-part-${labelIdNum}`;
133134
this.descriptorId = `pf-modal-part-${descriptorIdNum}`;
135+
this.backdropId = `pf-modal-part-${backdropIdNum}`;
134136

135137
this.state = {
136-
container: undefined,
137138
ouiaStateId: getDefaultOUIAId(Modal.displayName, props.variant)
138139
};
139140
}
@@ -157,7 +158,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
157158
const target: HTMLElement = this.getElement(appendTo);
158159
const bodyChildren = target.children;
159160
for (const child of Array.from(bodyChildren)) {
160-
if (child !== this.state.container) {
161+
if (child.id !== this.backdropId) {
161162
hide ? child.setAttribute('aria-hidden', '' + hide) : child.removeAttribute('aria-hidden');
162163
}
163164
}
@@ -175,15 +176,11 @@ class Modal extends React.Component<ModalProps, ModalState> {
175176
header
176177
} = this.props;
177178
const target: HTMLElement = this.getElement(appendTo);
178-
const container = document.createElement('div');
179-
this.setState({ container });
180-
target.appendChild(container);
181179
target.addEventListener('keydown', this.handleEscKeyClick, false);
182180

183181
if (this.props.isOpen) {
184182
target.classList.add(css(styles.backdropOpen));
185-
} else {
186-
target.classList.remove(css(styles.backdropOpen));
183+
this.toggleSiblingsFromScreenReaders(true);
187184
}
188185

189186
if (!title && this.isEmpty(ariaLabel) && this.isEmpty(ariaLabelledby)) {
@@ -199,32 +196,30 @@ class Modal extends React.Component<ModalProps, ModalState> {
199196
}
200197
}
201198

202-
componentDidUpdate() {
199+
componentDidUpdate(prevProps: ModalProps) {
203200
const { appendTo } = this.props;
204201
const target: HTMLElement = this.getElement(appendTo);
205202
if (this.props.isOpen) {
206203
target.classList.add(css(styles.backdropOpen));
207204
this.toggleSiblingsFromScreenReaders(true);
208205
} else {
209-
target.classList.remove(css(styles.backdropOpen));
210-
this.toggleSiblingsFromScreenReaders(false);
206+
if (prevProps.isOpen !== this.props.isOpen) {
207+
target.classList.remove(css(styles.backdropOpen));
208+
this.toggleSiblingsFromScreenReaders(false);
209+
}
211210
}
212211
}
213212

214213
componentWillUnmount() {
215214
const { appendTo } = this.props;
216215
const target: HTMLElement = this.getElement(appendTo);
217-
if (this.state.container) {
218-
target.removeChild(this.state.container);
219-
}
220216
target.removeEventListener('keydown', this.handleEscKeyClick, false);
221217
target.classList.remove(css(styles.backdropOpen));
222218
this.toggleSiblingsFromScreenReaders(false);
223219
}
224220

225221
render() {
226222
const {
227-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
228223
appendTo,
229224
// eslint-disable-next-line @typescript-eslint/no-unused-vars
230225
onEscapePress,
@@ -242,9 +237,8 @@ class Modal extends React.Component<ModalProps, ModalState> {
242237
elementToFocus,
243238
...props
244239
} = this.props;
245-
const { container } = this.state;
246240

247-
if (!canUseDOM || !container) {
241+
if (!canUseDOM || !this.getElement(appendTo)) {
248242
return null;
249243
}
250244

@@ -254,6 +248,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
254248
boxId={this.boxId}
255249
labelId={this.labelId}
256250
descriptorId={this.descriptorId}
251+
backdropId={this.backdropId}
257252
title={title}
258253
titleIconVariant={titleIconVariant}
259254
titleLabel={titleLabel}
@@ -267,7 +262,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
267262
position={position}
268263
elementToFocus={elementToFocus}
269264
/>,
270-
container
265+
this.getElement(appendTo)
271266
) as React.ReactElement;
272267
}
273268
}

‎packages/react-core/src/deprecated/components/Modal/ModalContent.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ export interface ModalContentProps extends OUIAProps {
2525
'aria-label'?: string;
2626
/** Id to use for the modal box label. */
2727
'aria-labelledby'?: string | null;
28+
/** Id of the backdrop. */
29+
backdropId?: string;
2830
/** Accessible label applied to the modal box body. This should be used to communicate
2931
* important information about the modal box body div element if needed, such as that it
3032
* is scrollable.
@@ -117,6 +119,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
117119
maxWidth,
118120
boxId,
119121
labelId,
122+
backdropId,
120123
descriptorId,
121124
disableFocusTrap = false,
122125
hasNoBodyWrapper = false,
@@ -202,7 +205,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
202205
</ModalBox>
203206
);
204207
return (
205-
<Backdrop>
208+
<Backdrop id={backdropId}>
206209
<FocusTrap
207210
active={!disableFocusTrap}
208211
focusTrapOptions={{

0 commit comments

Comments
 (0)