fix(Modal): Various bug fixes V5 (#11168)

* fix(Modal): Various bug fixes * update modal next * update test example * remove test examples
patternfly · Nov 19, 2024 · ec7624d · ec7624d
1 parent 9d53914
commit ec7624d
Show file tree

Hide file tree

Showing 7 changed files with 112 additions and 46 deletions.
diff --git a/eslint.config.mjs b/eslint.config.mjs
@@ -16,7 +16,8 @@ export default [
       'packages/react-core/src/helpers/Popper/thirdparty',
       'packages/react-docs/patternfly-docs/generated',
       'packages/react-docs/static',
-      '**/.cache'
+      '**/.cache',
+      '.history'
     ]
   },
   js.configs.recommended,

diff --git a/packages/react-core/src/components/Modal/Modal.tsx b/packages/react-core/src/components/Modal/Modal.tsx
@@ -92,7 +92,6 @@ export enum ModalVariant {
 }
 
 interface ModalState {
-  container: HTMLElement;
   ouiaStateId: string;
 }
 
@@ -102,6 +101,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
   boxId = '';
   labelId = '';
   descriptorId = '';
+  backdropId = '';
 
   static defaultProps: PickOptional<ModalProps> = {
     className: '',
@@ -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)
     };
   }
@@ -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');
       }
     }
@@ -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)) {
@@ -199,32 +196,31 @@ 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,
@@ -242,9 +238,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;
     }
 
@@ -254,6 +249,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}
@@ -267,7 +263,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
         position={position}
         elementToFocus={elementToFocus}
       />,
-      container
+      this.getElement(appendTo)
     ) as React.ReactElement;
   }
 }

diff --git a/packages/react-core/src/components/Modal/ModalContent.tsx b/packages/react-core/src/components/Modal/ModalContent.tsx
@@ -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.
@@ -117,6 +119,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
   maxWidth,
   boxId,
   labelId,
+  backdropId,
   descriptorId,
   disableFocusTrap = false,
   hasNoBodyWrapper = false,
@@ -202,7 +205,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
     </ModalBox>
   );
   return (
-    <Backdrop>
+    <Backdrop id={backdropId}>
       <FocusTrap
         active={!disableFocusTrap}
         focusTrapOptions={{

diff --git a/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx b/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx
@@ -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';
@@ -39,12 +40,33 @@ const ModalWithSiblings = () => {
   );
 };
 
-describe('Modal', () => {
-  test('Modal creates a container element once for div', () => {
-    render(<Modal {...props} />);
-    expect(document.createElement).toHaveBeenCalledWith('div');
-  });
+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 closes with escape', async () => {
     const user = userEvent.setup();
 
@@ -137,10 +159,10 @@ describe('Modal', () => {
     expect(asideSibling).not.toHaveAttribute('aria-hidden');
   });
 
-  test('modal removes the aria-hidden attribute from its siblings when unmounted', async () => {
+  test('modal siblings have the aria-hidden attribute when it has adjacent modals', async () => {
     const user = userEvent.setup();
 
-    render(<ModalWithSiblings />, { container: document.body.appendChild(target) });
+    render(<ModalWithAdjacentModal />, { container: document.body.appendChild(target) });
 
     const asideSibling = screen.getByRole('complementary', { hidden: true });
     const articleSibling = screen.getByRole('article', { hidden: true });
@@ -154,6 +176,7 @@ describe('Modal', () => {
     expect(asideSibling).not.toHaveAttribute('aria-hidden');
     expect(articleSibling).not.toHaveAttribute('aria-hidden');
   });
+
   test('The modalBoxBody has no aria-label when bodyAriaLabel is not passed', () => {
     const props = {
       isOpen: true

diff --git a/packages/react-core/src/next/components/Modal/Modal.tsx b/packages/react-core/src/next/components/Modal/Modal.tsx
@@ -58,14 +58,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,
@@ -78,10 +78,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)
     };
   }
@@ -105,7 +106,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');
       }
     }
@@ -116,44 +117,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,
@@ -166,15 +161,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}
@@ -184,7 +179,7 @@ class Modal extends React.Component<ModalProps, ModalState> {
         elementToFocus={elementToFocus}
         {...props}
       />,
-      container
+      this.getElement(appendTo)
     ) as React.ReactElement;
   }
 }

diff --git a/packages/react-core/src/next/components/Modal/ModalContent.tsx b/packages/react-core/src/next/components/Modal/ModalContent.tsx
@@ -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. */
@@ -60,6 +62,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
   width,
   maxWidth,
   boxId,
+  backdropId,
   disableFocusTrap = false,
   ouiaId,
   ouiaSafe = true,
@@ -106,7 +109,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
     </ModalBox>
   );
   return (
-    <Backdrop>
+    <Backdrop id={backdropId}>
       <FocusTrap
         active={!disableFocusTrap}
         focusTrapOptions={{