fix(Modal): Various bug fixes V5

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,

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;
   }
 }

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={{

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

packages/react-core/src/components/Modal/examples/Modal.md

@@ -15,6 +15,14 @@ import formStyles from '@patternfly/react-styles/css/components/Form/form';
 
 ## Examples
 
+### TEST PR
+
+this example is for testing the fix testing and will be deleted before PR merges.
+
+```ts file="./Test.tsx"
+
+```
+
 ### Basic modals
 
 Basic modals give users the option to either confirm or cancel an action. To flag an open modal, use the `isOpen` property. To execute a callback when a modal is closed, use the `onClose` property.

packages/react-core/src/components/Modal/examples/Test.tsx

@@ -0,0 +1,90 @@
+// this is for testing purposed only.  This file will be deleted before PR merges.
+
+import React from 'react';
+import { Modal, Button } from '@patternfly/react-core';
+
+export const Test: React.FunctionComponent = () => {
+  const YourIssue1 = ({ strict }) => {
+    const renderCount = React.useRef(0);
+    renderCount.current++;
+
+    const [isOpen, setIsOpen] = React.useState(false);
+
+    React.useEffect(() => {
+      // this use effect causes an extra re-render in strict mode
+    }, [isOpen]);
+
+    return (
+      <div>
+        <Button onClick={() => setIsOpen(true)}>{strict ? 'Strict ' : ''}Open Modal</Button>
+        {isOpen && (
+          <>
+            <Modal isOpen={isOpen} onClose={() => setIsOpen(false)}>
+              <s>
+                Inspect the div containing the modal to see the value of aria-hidden.
+                <div>Render count: {renderCount.current}</div>
+              </s>
+              <p>FIXED: Root div for portal has been removed. Only siblings of modal should have aria-hidden</p>
+            </Modal>
+          </>
+        )}
+      </div>
+    );
+  };
+
+  const YourIssue2 = () => {
+    const [isOpen, setIsOpen] = React.useState(false);
+    return (
+      <div>
+        <Button onClick={() => setIsOpen(true)}>Open Modal</Button>
+        <Modal isOpen={isOpen} onClose={() => setIsOpen(false)}>
+          <s>
+            Inspect the div containing the modal to see the value of all root siblings having no aria-hidden attribute.
+          </s>
+          <p>FIXED: Root siblings now have aria-hidden applied.</p>
+        </Modal>
+        <Modal isOpen={false} onClose={() => {}}>
+          This modal remains closed for this demo.
+        </Modal>
+        <Modal isOpen={false} onClose={() => {}}>
+          This modal remains closed for this demo.
+        </Modal>
+      </div>
+    );
+  };
+
+  return (
+    <div>
+      <p>
+        When a modal is open, the root and all siblings should have aria-hidden="true" except for the modal container.
+      </p>
+      <br />
+      <p>
+        <s>
+          When the strict mode modal opens, inspect the DOM to see that the open modal has aria-hidden="true" when it
+          should be "false".
+        </s>
+      </p>
+      <p>FIXED: root no longer has aria-hidden</p>
+      <p>
+        <s>Also, every time the modal opens a new div is appened and is never removed until the page is refreshed.</s>
+      </p>
+      <p>FIXED: the div was removed as it is not needed.</p>
+      <React.StrictMode>
+        <YourIssue1 strict />
+      </React.StrictMode>
+      <p>When the non-strict mode modal opens, inspect the DOM to see the open modal has no aria-hidden attribute.</p>
+      <YourIssue1 strict={undefined} />
+      <br />
+      <br />
+      <p>
+        <s>
+          Opening a modal while another adjacent modal is closed causes the aria-hidden attribute to be removed from the
+          root and all siblings.
+        </s>
+      </p>
+      <p>FIXED: aria hidden is no longer removed.</p>
+      <YourIssue2 />
+    </div>
+  );
+};