From c2bac91278fd1fc8e5047dba102e6f50937d46be Mon Sep 17 00:00:00 2001 From: Eric Olkowski Date: Wed, 19 Aug 2026 13:09:14 -0400 Subject: [PATCH 1/4] fix(Modal): updated logic to set aria-hidden for tearsheets --- .../react-core/src/components/Modal/Modal.tsx | 31 +++++-- .../components/Modal/__tests__/Modal.test.tsx | 84 +++++++++++++++++++ .../components/Modal/examples/ModalBasic.tsx | 25 ++++++ 3 files changed, 133 insertions(+), 7 deletions(-) diff --git a/packages/react-core/src/components/Modal/Modal.tsx b/packages/react-core/src/components/Modal/Modal.tsx index dac6ebbd864..82ff0183fa7 100644 --- a/packages/react-core/src/components/Modal/Modal.tsx +++ b/packages/react-core/src/components/Modal/Modal.tsx @@ -69,6 +69,7 @@ interface ModalState { class Modal extends Component { static displayName = 'Modal'; static currentId = 0; + static openModalStack: string[] = []; boxId = ''; backdropId = ''; @@ -109,12 +110,24 @@ class Modal extends Component { toggleSiblingsFromScreenReaders = (hide: boolean) => { const { appendTo } = this.props; const target: HTMLElement = this.getElement(appendTo); - const bodyChildren = target.children; - for (const child of Array.from(bodyChildren)) { - const isPopperElement = child.hasAttribute('data-popper-placement'); - if (child.id !== this.backdropId && !isPopperElement) { - hide ? child.setAttribute('aria-hidden', '' + hide) : child.removeAttribute('aria-hidden'); + const idx = Modal.openModalStack.indexOf(this.backdropId); + + if (hide && idx === -1) { + Modal.openModalStack.push(this.backdropId); + } else if (!hide && idx !== -1) { + Modal.openModalStack.splice(idx, 1); + } + + const activeBackdropId = + Modal.openModalStack.length > 0 ? Modal.openModalStack[Modal.openModalStack.length - 1] : null; + + for (const child of Array.from(target.children)) { + // We need to prevent aria-hidden being applied to popper elements appended to document.body + if (child.hasAttribute('data-popper-placement')) { + continue; } + const shouldHide = activeBackdropId && child.id !== activeBackdropId; + shouldHide ? child.setAttribute('aria-hidden', 'true') : child.removeAttribute('aria-hidden'); } }; @@ -140,8 +153,10 @@ class Modal extends Component { this.toggleSiblingsFromScreenReaders(true); } else { if (prevProps.isOpen !== this.props.isOpen) { - target.classList.remove(css(styles.backdropOpen)); this.toggleSiblingsFromScreenReaders(false); + if (Modal.openModalStack.length === 0) { + target.classList.remove(css(styles.backdropOpen)); + } } } } @@ -150,8 +165,10 @@ class Modal extends Component { const { appendTo } = this.props; const target: HTMLElement = this.getElement(appendTo); target.removeEventListener('keydown', this.handleEscKeyClick, false); - target.classList.remove(css(styles.backdropOpen)); this.toggleSiblingsFromScreenReaders(false); + if (Modal.openModalStack.length === 0) { + target.classList.remove(css(styles.backdropOpen)); + } } render() { diff --git a/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx b/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx index 7c1c768008c..b173baf2382 100644 --- a/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx +++ b/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx @@ -64,7 +64,28 @@ const ModalWithAdjacentModal = () => { ); }; +const MultipleOpenModals = () => { + const [isFirstOpen, setIsFirstOpen] = useState(true); + const [isSecondOpen, setIsSecondOpen] = useState(false); + + return ( + <> + + setIsFirstOpen(false)} aria-label="First modal"> + + + setIsSecondOpen(false)} aria-label="Second modal"> + Second modal content + + + ); +}; + describe('Modal', () => { + beforeEach(() => { + Modal.openModalStack = []; + }); + test('Modal creates a container element once for div', () => { render(); expect(document.createElement).toHaveBeenCalledWith('div'); @@ -181,4 +202,67 @@ describe('Modal', () => { 'pf-v6-l-bullseye' ); }); + + test('backdropOpen class remains when closing one of multiple open modals', async () => { + const user = userEvent.setup(); + + render(, { container: document.body.appendChild(target) }); + + await user.click(screen.getByRole('button', { name: 'Open second modal' })); + + expect(target).toHaveClass(css(styles.backdropOpen)); + + const closeButtons = screen.getAllByRole('button', { name: 'Close', hidden: true }); + await user.click(closeButtons[closeButtons.length - 1]); + + expect(target).toHaveClass(css(styles.backdropOpen)); + }); + + test('backdropOpen class is removed when all modals are closed', async () => { + const user = userEvent.setup(); + + render(, { container: document.body.appendChild(target) }); + + await user.click(screen.getByRole('button', { name: 'Open second modal' })); + + const closeButtons = screen.getAllByRole('button', { name: 'Close', hidden: true }); + await user.click(closeButtons[closeButtons.length - 1]); + await user.click(screen.getByRole('button', { name: 'Close' })); + + expect(target).not.toHaveClass(css(styles.backdropOpen)); + }); + + test('only the most recent modal does not have aria-hidden when multiple modals are open', async () => { + const user = userEvent.setup(); + + render(, { container: document.body.appendChild(target) }); + + const firstBackdrop = screen.getByLabelText('First modal').closest('[class*="backdrop"]'); + + await user.click(screen.getByRole('button', { name: 'Open second modal' })); + + const secondBackdrop = screen.getByLabelText('Second modal').closest('[class*="backdrop"]'); + + expect(firstBackdrop).toHaveAttribute('aria-hidden', 'true'); + expect(secondBackdrop).not.toHaveAttribute('aria-hidden'); + }); + + test('closing the active modal reveals the previous modal', async () => { + const user = userEvent.setup(); + + render(, { container: document.body.appendChild(target) }); + + await user.click(screen.getByRole('button', { name: 'Open second modal' })); + + const firstBackdrop = screen + .getByLabelText('First modal', { selector: '[role="dialog"]' }) + .closest('[class*="backdrop"]'); + + expect(firstBackdrop).toHaveAttribute('aria-hidden', 'true'); + + const closeButtons = screen.getAllByRole('button', { name: 'Close', hidden: true }); + await user.click(closeButtons[closeButtons.length - 1]); + + expect(firstBackdrop).not.toHaveAttribute('aria-hidden'); + }); }); diff --git a/packages/react-core/src/components/Modal/examples/ModalBasic.tsx b/packages/react-core/src/components/Modal/examples/ModalBasic.tsx index 7ebec3da305..a55f1231ac1 100644 --- a/packages/react-core/src/components/Modal/examples/ModalBasic.tsx +++ b/packages/react-core/src/components/Modal/examples/ModalBasic.tsx @@ -3,10 +3,14 @@ import { Button, Modal, ModalBody, ModalFooter, ModalHeader } from '@patternfly/ export const ModalBasic: React.FunctionComponent = () => { const [isModalOpen, setIsModalOpen] = useState(false); + const [isModal2Open, setIsModal2Open] = useState(false); const handleModalToggle = (_event: KeyboardEvent | React.MouseEvent) => { setIsModalOpen(!isModalOpen); }; + const handleModal2Toggle = (_event: KeyboardEvent | React.MouseEvent) => { + setIsModal2Open(!isModal2Open); + }; return ( @@ -27,6 +31,9 @@ export const ModalBasic: React.FunctionComponent = () => { consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. + + + + Nested modal + + + + + ); }; From 6106b3d94b59efcfc53b0b02d181da6e0ebadf05 Mon Sep 17 00:00:00 2001 From: Eric Olkowski Date: Wed, 19 Aug 2026 13:30:26 -0400 Subject: [PATCH 2/4] Coderabbit suggestion --- .../react-core/src/components/Modal/Modal.tsx | 27 ++++++---- .../components/Modal/__tests__/Modal.test.tsx | 53 ++++++++++++++++++- .../components/Modal/examples/ModalBasic.tsx | 1 + 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/packages/react-core/src/components/Modal/Modal.tsx b/packages/react-core/src/components/Modal/Modal.tsx index 82ff0183fa7..602ffbe8f8c 100644 --- a/packages/react-core/src/components/Modal/Modal.tsx +++ b/packages/react-core/src/components/Modal/Modal.tsx @@ -69,7 +69,7 @@ interface ModalState { class Modal extends Component { static displayName = 'Modal'; static currentId = 0; - static openModalStack: string[] = []; + static openModalStacks: Map = new Map(); boxId = ''; backdropId = ''; @@ -107,22 +107,31 @@ class Modal extends Component { return appendTo || document.body; }; + static getStackForTarget(target: HTMLElement): string[] { + if (!Modal.openModalStacks.has(target)) { + Modal.openModalStacks.set(target, []); + } + return Modal.openModalStacks.get(target)!; + } + toggleSiblingsFromScreenReaders = (hide: boolean) => { const { appendTo } = this.props; const target: HTMLElement = this.getElement(appendTo); - const idx = Modal.openModalStack.indexOf(this.backdropId); + const stack = Modal.getStackForTarget(target); + const idx = stack.indexOf(this.backdropId); if (hide && idx === -1) { - Modal.openModalStack.push(this.backdropId); + stack.push(this.backdropId); } else if (!hide && idx !== -1) { - Modal.openModalStack.splice(idx, 1); + stack.splice(idx, 1); + if (stack.length === 0) { + Modal.openModalStacks.delete(target); + } } - const activeBackdropId = - Modal.openModalStack.length > 0 ? Modal.openModalStack[Modal.openModalStack.length - 1] : null; + const activeBackdropId = stack.length > 0 ? stack[stack.length - 1] : null; for (const child of Array.from(target.children)) { - // We need to prevent aria-hidden being applied to popper elements appended to document.body if (child.hasAttribute('data-popper-placement')) { continue; } @@ -154,7 +163,7 @@ class Modal extends Component { } else { if (prevProps.isOpen !== this.props.isOpen) { this.toggleSiblingsFromScreenReaders(false); - if (Modal.openModalStack.length === 0) { + if (!Modal.openModalStacks.has(target)) { target.classList.remove(css(styles.backdropOpen)); } } @@ -166,7 +175,7 @@ class Modal extends Component { const target: HTMLElement = this.getElement(appendTo); target.removeEventListener('keydown', this.handleEscKeyClick, false); this.toggleSiblingsFromScreenReaders(false); - if (Modal.openModalStack.length === 0) { + if (!Modal.openModalStacks.has(target)) { target.classList.remove(css(styles.backdropOpen)); } } diff --git a/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx b/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx index b173baf2382..c72960ed4b6 100644 --- a/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx +++ b/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx @@ -83,7 +83,7 @@ const MultipleOpenModals = () => { describe('Modal', () => { beforeEach(() => { - Modal.openModalStack = []; + Modal.openModalStacks = new Map(); }); test('Modal creates a container element once for div', () => { @@ -265,4 +265,55 @@ describe('Modal', () => { expect(firstBackdrop).not.toHaveAttribute('aria-hidden'); }); + + test('modals with different appendTo targets have independent stacks', async () => { + const user = userEvent.setup(); + const targetA = document.createElement('div'); + const targetB = document.createElement('div'); + document.body.appendChild(targetA); + document.body.appendChild(targetB); + + const siblingA = document.createElement('aside'); + siblingA.textContent = 'Sibling A'; + targetA.appendChild(siblingA); + + const siblingB = document.createElement('aside'); + siblingB.textContent = 'Sibling B'; + targetB.appendChild(siblingB); + + const DistinctTargetModals = () => { + const [isAOpen, setIsAOpen] = useState(true); + const [isBOpen, setIsBOpen] = useState(true); + + return ( + <> + setIsAOpen(false)} aria-label="Modal A"> + Modal A content + + setIsBOpen(false)} aria-label="Modal B"> + Modal B content + + + ); + }; + + render(); + + expect(siblingA).toHaveAttribute('aria-hidden', 'true'); + expect(siblingB).toHaveAttribute('aria-hidden', 'true'); + expect(targetA).toHaveClass(css(styles.backdropOpen)); + expect(targetB).toHaveClass(css(styles.backdropOpen)); + + const closeButtons = screen.getAllByRole('button', { name: 'Close', hidden: true }); + await user.click(closeButtons[1]); + + expect(targetB).not.toHaveClass(css(styles.backdropOpen)); + expect(siblingB).not.toHaveAttribute('aria-hidden'); + + expect(targetA).toHaveClass(css(styles.backdropOpen)); + expect(siblingA).toHaveAttribute('aria-hidden', 'true'); + + document.body.removeChild(targetA); + document.body.removeChild(targetB); + }); }); diff --git a/packages/react-core/src/components/Modal/examples/ModalBasic.tsx b/packages/react-core/src/components/Modal/examples/ModalBasic.tsx index a55f1231ac1..fd75bf7b214 100644 --- a/packages/react-core/src/components/Modal/examples/ModalBasic.tsx +++ b/packages/react-core/src/components/Modal/examples/ModalBasic.tsx @@ -18,6 +18,7 @@ export const ModalBasic: React.FunctionComponent = () => { Show basic modal document.querySelector('#root') as HTMLElement} isOpen={isModalOpen} onClose={handleModalToggle} ouiaId="BasicModal" From f3bbba5754c657d7338349d7592cb2f0e45555be Mon Sep 17 00:00:00 2001 From: Eric Olkowski Date: Thu, 20 Aug 2026 13:29:32 -0400 Subject: [PATCH 3/4] Added close cleanup tweaks --- .../react-core/src/components/Modal/Modal.tsx | 23 +++++++++++++------ .../components/Modal/__tests__/Modal.test.tsx | 17 ++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/packages/react-core/src/components/Modal/Modal.tsx b/packages/react-core/src/components/Modal/Modal.tsx index 602ffbe8f8c..6dfe09a3ce2 100644 --- a/packages/react-core/src/components/Modal/Modal.tsx +++ b/packages/react-core/src/components/Modal/Modal.tsx @@ -117,19 +117,28 @@ class Modal extends Component { toggleSiblingsFromScreenReaders = (hide: boolean) => { const { appendTo } = this.props; const target: HTMLElement = this.getElement(appendTo); - const stack = Modal.getStackForTarget(target); - const idx = stack.indexOf(this.backdropId); - if (hide && idx === -1) { - stack.push(this.backdropId); - } else if (!hide && idx !== -1) { - stack.splice(idx, 1); + if (hide) { + const stack = Modal.getStackForTarget(target); + if (stack.indexOf(this.backdropId) === -1) { + stack.push(this.backdropId); + } + } else { + const stack = Modal.openModalStacks.get(target); + if (!stack) { + return; + } + const idx = stack.indexOf(this.backdropId); + if (idx !== -1) { + stack.splice(idx, 1); + } if (stack.length === 0) { Modal.openModalStacks.delete(target); } } - const activeBackdropId = stack.length > 0 ? stack[stack.length - 1] : null; + const stack = Modal.openModalStacks.get(target); + const activeBackdropId = stack?.length ? stack[stack.length - 1] : null; for (const child of Array.from(target.children)) { if (child.hasAttribute('data-popper-placement')) { diff --git a/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx b/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx index c72960ed4b6..f4c4ea87feb 100644 --- a/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx +++ b/packages/react-core/src/components/Modal/__tests__/Modal.test.tsx @@ -316,4 +316,21 @@ describe('Modal', () => { document.body.removeChild(targetA); document.body.removeChild(targetB); }); + + test('unmounting a never-opened modal with a custom target does not leak a stack entry', () => { + const customTarget = document.createElement('div'); + document.body.appendChild(customTarget); + + const { unmount } = render( + {}}> + Never opened + + ); + + unmount(); + + expect(Modal.openModalStacks.has(customTarget)).toBe(false); + + document.body.removeChild(customTarget); + }); }); From ada9a3c5f20c5c1611077fd86b5910a41cfa7cd2 Mon Sep 17 00:00:00 2001 From: Eric Olkowski Date: Thu, 20 Aug 2026 13:41:31 -0400 Subject: [PATCH 4/4] Reverted basic example --- .../components/Modal/examples/ModalBasic.tsx | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/packages/react-core/src/components/Modal/examples/ModalBasic.tsx b/packages/react-core/src/components/Modal/examples/ModalBasic.tsx index fd75bf7b214..7ebec3da305 100644 --- a/packages/react-core/src/components/Modal/examples/ModalBasic.tsx +++ b/packages/react-core/src/components/Modal/examples/ModalBasic.tsx @@ -3,14 +3,10 @@ import { Button, Modal, ModalBody, ModalFooter, ModalHeader } from '@patternfly/ export const ModalBasic: React.FunctionComponent = () => { const [isModalOpen, setIsModalOpen] = useState(false); - const [isModal2Open, setIsModal2Open] = useState(false); const handleModalToggle = (_event: KeyboardEvent | React.MouseEvent) => { setIsModalOpen(!isModalOpen); }; - const handleModal2Toggle = (_event: KeyboardEvent | React.MouseEvent) => { - setIsModal2Open(!isModal2Open); - }; return ( @@ -18,7 +14,6 @@ export const ModalBasic: React.FunctionComponent = () => { Show basic modal document.querySelector('#root') as HTMLElement} isOpen={isModalOpen} onClose={handleModalToggle} ouiaId="BasicModal" @@ -32,9 +27,6 @@ export const ModalBasic: React.FunctionComponent = () => { consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. - - - - Nested modal - - - - - ); };