Skip to content

fix(Modal): Fixes various bugs in the Modal and deprecated Modal #11207

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.

Sign up for GitHub

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

Merged
kmcfaul merged 1 commit into from
Nov 19, 2024
Merged

fix(Modal): Fixes various bugs in the Modal and deprecated Modal #11207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 13 additions & 18 deletions packages/react-core/src/components/Modal/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,14 @@ export enum ModalVariant {
}

interface ModalState {
container: HTMLElement;
ouiaStateId: string;
}

class Modal extends React.Component<ModalProps, ModalState> {
static displayName = 'Modal';
static currentId = 0;
boxId = '';
backdropId = '';

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

this.state = {
container: undefined,
ouiaStateId: getDefaultOUIAId(Modal.displayName, props.variant)
};
}
Expand All @@ -107,7 +108,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
const target: HTMLElement = this.getElement(appendTo);
const bodyChildren = target.children;
for (const child of Array.from(bodyChildren)) {
if (child !== this.state.container) {
if (child.id !== this.backdropId) {
hide ? child.setAttribute('aria-hidden', '' + hide) : child.removeAttribute('aria-hidden');
}
}
Expand All @@ -118,44 +119,38 @@ class Modal extends React.Component<ModalProps, ModalState> {
componentDidMount() {
const { appendTo } = this.props;
const target: HTMLElement = this.getElement(appendTo);
const container = document.createElement('div');
this.setState({ container });
target.appendChild(container);
target.addEventListener('keydown', this.handleEscKeyClick, false);

if (this.props.isOpen) {
target.classList.add(css(styles.backdropOpen));
} else {
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(true);
}
}

componentDidUpdate() {
componentDidUpdate(prevProps: ModalProps) {
const { appendTo } = this.props;
const target: HTMLElement = this.getElement(appendTo);
if (this.props.isOpen) {
target.classList.add(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(true);
} else {
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(false);
if (prevProps.isOpen !== this.props.isOpen) {
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(false);
}
}
}

componentWillUnmount() {
const { appendTo } = this.props;
const target: HTMLElement = this.getElement(appendTo);
if (this.state.container) {
target.removeChild(this.state.container);
}
target.removeEventListener('keydown', this.handleEscKeyClick, false);
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(false);
}

render() {
const {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
appendTo,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
onEscapePress,
Expand All @@ -168,15 +163,15 @@ class Modal extends React.Component<ModalProps, ModalState> {
elementToFocus,
...props
} = this.props;
const { container } = this.state;

if (!canUseDOM || !container) {
if (!canUseDOM || !this.getElement(appendTo)) {
return null;
}

return ReactDOM.createPortal(
<ModalContent
boxId={this.boxId}
backdropId={this.backdropId}
aria-label={ariaLabel}
aria-describedby={ariaDescribedby}
aria-labelledby={ariaLabelledby}
Expand All @@ -187,7 +182,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
backdropClassName={props.backdropClassName}
{...props}
/>,
container
this.getElement(appendTo)
) as React.ReactElement;
}
}
Expand Down
5 changes: 4 additions & 1 deletion packages/react-core/src/components/Modal/ModalContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ export interface ModalContentProps extends OUIAProps {
'aria-labelledby'?: string;
/** Id of the modal box container. */
boxId: string;
/** Id of the backdrop. */
backdropId?: string;
/** Content rendered inside the modal. */
children: React.ReactNode;
/** Additional classes added to the modal box. */
Expand Down Expand Up @@ -63,6 +65,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
width,
maxWidth,
boxId,
backdropId,
disableFocusTrap = false,
ouiaId,
ouiaSafe = true,
Expand Down Expand Up @@ -109,7 +112,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
</ModalBox>
);
return (
<Backdrop className={css(backdropClassName)}>
<Backdrop className={css(backdropClassName)} id={backdropId}>
<FocusTrap
active={!disableFocusTrap}
focusTrapOptions={{
Expand Down
45 changes: 45 additions & 0 deletions packages/react-core/src/components/Modal/__tests__/Modal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as React from 'react';

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

import { css } from '../../../../../react-styles/dist/js';
import styles from '@patternfly/react-styles/css/components/Backdrop/backdrop';
Expand Down Expand Up @@ -38,6 +39,32 @@ const ModalWithSiblings = () => {
);
};

const ModalWithAdjacentModal = () => {
const [isOpen, setIsOpen] = React.useState(true);
const [isModalMounted, setIsModalMounted] = React.useState(true);
const modalProps = { ...props, isOpen, appendTo: target, onClose: () => setIsOpen(false) };

return (
<>
<aside>Aside sibling</aside>
<article>Section sibling</article>
{isModalMounted && (
<>
<Modal {...modalProps}>
<button onClick={() => setIsModalMounted(false)}>Unmount Modal</button>
</Modal>
<Modal isOpen={false} onClose={() => {}}>
Modal closed for test
</Modal>
<Modal isOpen={false} onClose={() => {}}>
modal closed for test
</Modal>
</>
)}
</>
);
};

describe('Modal', () => {
test('Modal creates a container element once for div', () => {
render(<Modal {...props} />);
Expand Down Expand Up @@ -128,4 +155,22 @@ describe('Modal', () => {
expect(asideSibling).not.toHaveAttribute('aria-hidden');
expect(articleSibling).not.toHaveAttribute('aria-hidden');
});

test('modal siblings have the aria-hidden attribute when it has adjacent modals', async () => {
const user = userEvent.setup();

render(<ModalWithAdjacentModal />, { container: document.body.appendChild(target) });

const asideSibling = screen.getByRole('complementary', { hidden: true });
const articleSibling = screen.getByRole('article', { hidden: true });
const unmountButton = screen.getByRole('button', { name: 'Unmount Modal' });

expect(asideSibling).toHaveAttribute('aria-hidden');
expect(articleSibling).toHaveAttribute('aria-hidden');

await user.click(unmountButton);

expect(asideSibling).not.toHaveAttribute('aria-hidden');
expect(articleSibling).not.toHaveAttribute('aria-hidden');
});
});
31 changes: 13 additions & 18 deletions packages/react-core/src/deprecated/components/Modal/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ export enum ModalVariant {
}

interface ModalState {
container: HTMLElement;
ouiaStateId: string;
}

Expand All @@ -102,6 +101,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
boxId = '';
labelId = '';
descriptorId = '';
backdropId = '';

static defaultProps: PickOptional<ModalProps> = {
className: '',
Expand All @@ -128,12 +128,13 @@ class Modal extends React.Component<ModalProps, ModalState> {
const boxIdNum = Modal.currentId++;
const labelIdNum = boxIdNum + 1;
const descriptorIdNum = boxIdNum + 2;
const backdropIdNum = boxIdNum + 3;
this.boxId = props.id || `pf-modal-part-${boxIdNum}`;
this.labelId = `pf-modal-part-${labelIdNum}`;
this.descriptorId = `pf-modal-part-${descriptorIdNum}`;
this.backdropId = `pf-modal-part-${backdropIdNum}`;

this.state = {
container: undefined,
ouiaStateId: getDefaultOUIAId(Modal.displayName, props.variant)
};
}
Expand All @@ -157,7 +158,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
const target: HTMLElement = this.getElement(appendTo);
const bodyChildren = target.children;
for (const child of Array.from(bodyChildren)) {
if (child !== this.state.container) {
if (child.id !== this.backdropId) {
hide ? child.setAttribute('aria-hidden', '' + hide) : child.removeAttribute('aria-hidden');
}
}
Expand All @@ -175,15 +176,11 @@ class Modal extends React.Component<ModalProps, ModalState> {
header
} = this.props;
const target: HTMLElement = this.getElement(appendTo);
const container = document.createElement('div');
this.setState({ container });
target.appendChild(container);
target.addEventListener('keydown', this.handleEscKeyClick, false);

if (this.props.isOpen) {
target.classList.add(css(styles.backdropOpen));
} else {
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(true);
}

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

componentDidUpdate() {
componentDidUpdate(prevProps: ModalProps) {
const { appendTo } = this.props;
const target: HTMLElement = this.getElement(appendTo);
if (this.props.isOpen) {
target.classList.add(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(true);
} else {
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(false);
if (prevProps.isOpen !== this.props.isOpen) {
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(false);
}
}
}

componentWillUnmount() {
const { appendTo } = this.props;
const target: HTMLElement = this.getElement(appendTo);
if (this.state.container) {
target.removeChild(this.state.container);
}
target.removeEventListener('keydown', this.handleEscKeyClick, false);
target.classList.remove(css(styles.backdropOpen));
this.toggleSiblingsFromScreenReaders(false);
}

render() {
const {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
appendTo,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
onEscapePress,
Expand All @@ -242,9 +237,8 @@ class Modal extends React.Component<ModalProps, ModalState> {
elementToFocus,
...props
} = this.props;
const { container } = this.state;

if (!canUseDOM || !container) {
if (!canUseDOM || !this.getElement(appendTo)) {
return null;
}

Expand All @@ -254,6 +248,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
boxId={this.boxId}
labelId={this.labelId}
descriptorId={this.descriptorId}
backdropId={this.backdropId}
title={title}
titleIconVariant={titleIconVariant}
titleLabel={titleLabel}
Expand All @@ -267,7 +262,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
position={position}
elementToFocus={elementToFocus}
/>,
container
this.getElement(appendTo)
) as React.ReactElement;
}
}
Expand Down
5 changes: 4 additions & 1 deletion packages/react-core/src/deprecated/components/Modal/ModalContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ export interface ModalContentProps extends OUIAProps {
'aria-label'?: string;
/** Id to use for the modal box label. */
'aria-labelledby'?: string | null;
/** Id of the backdrop. */
backdropId?: string;
/** Accessible label applied to the modal box body. This should be used to communicate
* important information about the modal box body div element if needed, such as that it
* is scrollable.
Expand Down Expand Up @@ -117,6 +119,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
maxWidth,
boxId,
labelId,
backdropId,
descriptorId,
disableFocusTrap = false,
hasNoBodyWrapper = false,
Expand Down Expand Up @@ -202,7 +205,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
</ModalBox>
);
return (
<Backdrop>
<Backdrop id={backdropId}>
<FocusTrap
active={!disableFocusTrap}
focusTrapOptions={{
Expand Down
Loading
Loading