From e3c88d836de2ce7460a53d1af7a8b308a2046fec Mon Sep 17 00:00:00 2001 From: Rafael Anachoreta Date: Tue, 12 May 2026 14:00:33 +0100 Subject: [PATCH 1/2] [ER-49165] Fix focus recursion between Drawer and Modal --- .changeset/drawer-modal-focus-recursion.md | 8 + packages/base/Drawer/package.json | 4 +- packages/base/Drawer/src/Drawer/Drawer.tsx | 19 ++- packages/base/Drawer/src/Drawer/test.tsx | 147 ++++++++++++++++++ packages/base/Modal/src/Modal/Modal.tsx | 19 +-- packages/base/Utils/src/utils/Modal/index.ts | 6 +- .../Utils/src/utils/Modal/modal-manager.ts | 14 ++ pnpm-lock.yaml | 6 + 8 files changed, 207 insertions(+), 16 deletions(-) create mode 100644 .changeset/drawer-modal-focus-recursion.md create mode 100644 packages/base/Drawer/src/Drawer/test.tsx diff --git a/.changeset/drawer-modal-focus-recursion.md b/.changeset/drawer-modal-focus-recursion.md new file mode 100644 index 0000000000..ee255d53f6 --- /dev/null +++ b/.changeset/drawer-modal-focus-recursion.md @@ -0,0 +1,8 @@ +--- +'@toptal/picasso-drawer': patch +'@toptal/picasso-modal': patch +'@toptal/picasso-utils': minor +'@toptal/picasso': patch +--- + +- fix `Maximum call stack size exceeded` when a `Drawer` is opened from inside an open `Modal` and then the `Modal` is closed (ER-49165). `Modal` uses a custom document-level focus listener (with `disableEnforceFocus` on MUI's Modal) while `Drawer` relies on MUI's built-in `FocusTrap`; with both active the two focus mechanisms recursed on `Element.focus()`. `Drawer` now registers with the same shared `ModalManager` singleton that `Modal` uses, so when a `Drawer` is on top the underlying `Modal` correctly identifies itself as no longer the topmost overlay and bails out of its focus listener. diff --git a/packages/base/Drawer/package.json b/packages/base/Drawer/package.json index 075a45c054..e096f40cae 100644 --- a/packages/base/Drawer/package.json +++ b/packages/base/Drawer/package.json @@ -47,8 +47,10 @@ ".": "./dist-package/src/index.js" }, "devDependencies": { + "@toptal/picasso-modal": "4.0.0", "@toptal/picasso-provider": "5.0.2", - "@toptal/picasso-tailwind-merge": "2.0.4" + "@toptal/picasso-tailwind-merge": "2.0.4", + "@toptal/picasso-test-utils": "2.0.0" }, "files": [ "dist-package/**", diff --git a/packages/base/Drawer/src/Drawer/Drawer.tsx b/packages/base/Drawer/src/Drawer/Drawer.tsx index c92676fedd..7a15fbca38 100644 --- a/packages/base/Drawer/src/Drawer/Drawer.tsx +++ b/packages/base/Drawer/src/Drawer/Drawer.tsx @@ -1,4 +1,4 @@ -import React, { useRef } from 'react' +import React, { useEffect, useRef } from 'react' import { Modal } from '@mui/base/Modal' import type { BaseProps, TransitionProps } from '@toptal/picasso-shared' import { useDrawer, usePicassoRoot } from '@toptal/picasso-provider' @@ -9,6 +9,8 @@ import { Slide } from '@toptal/picasso-slide' import { Backdrop } from '@toptal/picasso-backdrop' import { Container } from '@toptal/picasso-container' import { + defaultModalManager, + generateModalId, useIsomorphicLayoutEffect, usePageScrollLock, } from '@toptal/picasso-utils' @@ -81,6 +83,7 @@ export const Drawer = ({ const { setHasDrawer } = useDrawer() const container = usePicassoRoot() const ref = useRef(null) + const drawerId = useRef(generateModalId()) usePageScrollLock(Boolean(maintainBodyScrollLock && open)) @@ -94,6 +97,20 @@ export const Drawer = ({ return cleanup }, [open, setHasDrawer]) + // Register with the shared overlay manager so a Picasso Modal below + // yields focus enforcement to this Drawer (see ER-49165). + useEffect(() => { + const id = drawerId.current + + if (open) { + defaultModalManager.add(id) + } + + return () => { + defaultModalManager.remove(id) + } + }, [open]) + const handleOnClose = () => { if (onClose) { onClose() diff --git a/packages/base/Drawer/src/Drawer/test.tsx b/packages/base/Drawer/src/Drawer/test.tsx new file mode 100644 index 0000000000..0ae08db5b4 --- /dev/null +++ b/packages/base/Drawer/src/Drawer/test.tsx @@ -0,0 +1,147 @@ +import React from 'react' +import { afterEach, describe, expect, it, jest } from '@jest/globals' +import { render, cleanup } from '@toptal/picasso-test-utils' +import { Modal } from '@toptal/picasso-modal' +import { defaultModalManager } from '@toptal/picasso-utils' + +import Drawer from './Drawer' + +describe('Drawer', () => { + afterEach(() => { + cleanup() + defaultModalManager.reset() + }) + + it('renders without crashing', () => { + const { baseElement } = render(content) + + expect(baseElement).toBeInTheDocument() + }) + + describe('overlay registration', () => { + it('registers with defaultModalManager when open', () => { + render(content) + expect(defaultModalManager.modals).toHaveLength(1) + }) + + it('does not register when closed', () => { + render(content) + expect(defaultModalManager.modals).toHaveLength(0) + }) + + it('removes itself from defaultModalManager on unmount', () => { + const { unmount } = render(content) + + expect(defaultModalManager.modals).toHaveLength(1) + unmount() + expect(defaultModalManager.modals).toHaveLength(0) + }) + + it('updates registration when open prop toggles', () => { + const { rerender } = render(content) + + expect(defaultModalManager.modals).toHaveLength(1) + + rerender(content) + expect(defaultModalManager.modals).toHaveLength(0) + + rerender(content) + expect(defaultModalManager.modals).toHaveLength(1) + }) + }) + + // ER-49165: Picasso Modal uses a custom document focus listener (because it + // sets disableEnforceFocus on MUI's Modal) while Drawer relies on MUI's + // FocusTrap. With both active, the two focus mechanisms recursed on + // Element.focus() and raised "Maximum call stack size exceeded". + describe('when nested inside a Picasso Modal (ER-49165)', () => { + it('takes over as topmost overlay in the shared manager', () => { + // Mount Modal first so its id is unambiguous in the manager — relying + // on the stack position after a simultaneous mount of both would be + // fragile against MUI's internal mount lifecycle. + const { rerender } = render(modal content) + const modalId = defaultModalManager.modals[0] + + expect(defaultModalManager.isTopModal(modalId)).toBe(true) + + rerender( + + drawer content + + ) + + // Modal stayed mounted, only Drawer fired its add, so the stack is + // [modalId, drawerId] with the Drawer on top. + expect(defaultModalManager.modals).toHaveLength(2) + expect(defaultModalManager.modals[0]).toBe(modalId) + const drawerId = defaultModalManager.modals[1] + + expect(defaultModalManager.isTopModal(drawerId)).toBe(true) + expect(defaultModalManager.isTopModal(modalId)).toBe(false) + }) + + it('makes the Modal listener bail out on document focus events', () => { + const focusSpy = jest.spyOn(HTMLElement.prototype, 'focus') + // Spying on isTopModal lets us prove the Modal listener actually ran — + // without this, a harness regression (e.g. the listener not attached or + // the synthetic event not reaching it) would silently pass the + // negative .focus() assertion below. + const isTopModalSpy = jest.spyOn(defaultModalManager, 'isTopModal') + + render( + + + + + + + ) + + // Discard focus calls fired during mount (auto-focus, MUI FocusTrap). + focusSpy.mockClear() + isTopModalSpy.mockClear() + + // Re-fire the document focus listener path that previously recursed. + document.dispatchEvent(new FocusEvent('focus', { bubbles: true })) + + // Proof the Modal listener actually fired and queried the manager. + // isTopModal is only called from inside Modal's focus handler, so a + // call here means the handler ran end-to-end up to the bailout check. + expect(isTopModalSpy).toHaveBeenCalled() + // And at least one of those calls returned false (the Modal's own id, + // since Drawer is on top), exercising the bailout branch. + expect(isTopModalSpy.mock.results.map(result => result.value)).toContain( + false + ) + + // With the bailout taken, Modal's handler does not invoke + // focusFirstFocusableElement, so no .focus() calls originate from it. + expect(focusSpy).not.toHaveBeenCalled() + + focusSpy.mockRestore() + isTopModalSpy.mockRestore() + }) + + it('restores Modal as topmost when the Drawer closes', () => { + const { rerender } = render(modal content) + const modalId = defaultModalManager.modals[0] + + rerender( + + drawer content + + ) + + expect(defaultModalManager.isTopModal(modalId)).toBe(false) + + rerender( + + drawer content + + ) + + expect(defaultModalManager.modals).toHaveLength(1) + expect(defaultModalManager.isTopModal(modalId)).toBe(true) + }) + }) +}) diff --git a/packages/base/Modal/src/Modal/Modal.tsx b/packages/base/Modal/src/Modal/Modal.tsx index d961718022..7073170090 100644 --- a/packages/base/Modal/src/Modal/Modal.tsx +++ b/packages/base/Modal/src/Modal/Modal.tsx @@ -16,7 +16,8 @@ import { usePicassoRoot, RootContext } from '@toptal/picasso-provider' import { CloseMinor16 } from '@toptal/picasso-icons' import { useCombinedRefs, - ModalManager, + defaultModalManager, + generateModalId, usePageScrollLock, } from '@toptal/picasso-utils' import { ButtonCircular } from '@toptal/picasso-button' @@ -66,8 +67,6 @@ export interface Props extends BaseProps, HTMLAttributes { } } -const defaultManager = new ModalManager() - // https://github.com/udacity/ud891/blob/gh-pages/lesson2-focus/07-modals-and-keyboard-traps/solution/modal.js#L25 // found in https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex const focusableElementsString = @@ -112,12 +111,6 @@ const isFocusInsideTooltip = () => { return false } -const generateKey = (() => { - let count = 0 - - return () => ++count -})() - // eslint-disable-next-line react/display-name export const Modal = forwardRef(function Modal( { @@ -150,7 +143,7 @@ export const Modal = forwardRef(function Modal( ref, useRef(null) ) - const modalId = useRef(generateKey()) + const modalId = useRef(generateModalId()) const { rootRef } = useContext(RootContext) useEffect(() => { @@ -161,7 +154,7 @@ export const Modal = forwardRef(function Modal( ) } - if (!defaultManager.isTopModal(modalId.current)) { + if (!defaultModalManager.isTopModal(modalId.current)) { return } @@ -195,11 +188,11 @@ export const Modal = forwardRef(function Modal( const currentModalId = modalId.current if (open) { - defaultManager.add(currentModalId) + defaultModalManager.add(currentModalId) } return () => { - defaultManager.remove(currentModalId) + defaultModalManager.remove(currentModalId) } }, [open]) diff --git a/packages/base/Utils/src/utils/Modal/index.ts b/packages/base/Utils/src/utils/Modal/index.ts index a385579c38..50a955f854 100644 --- a/packages/base/Utils/src/utils/Modal/index.ts +++ b/packages/base/Utils/src/utils/Modal/index.ts @@ -1,2 +1,6 @@ -export { ModalManager } from './modal-manager' +export { + ModalManager, + defaultModalManager, + generateModalId, +} from './modal-manager' export { useModal } from './use-modal' diff --git a/packages/base/Utils/src/utils/Modal/modal-manager.ts b/packages/base/Utils/src/utils/Modal/modal-manager.ts index b60a832ded..19d3d81e1f 100644 --- a/packages/base/Utils/src/utils/Modal/modal-manager.ts +++ b/packages/base/Utils/src/utils/Modal/modal-manager.ts @@ -24,4 +24,18 @@ export class ModalManager { this.modals.length > 0 && this.modals[this.modals.length - 1] === modalId ) } + + /** @internal Test-only helper to reset state between specs. */ + reset() { + this.modals.length = 0 + } } + +// Shared across Picasso overlay components (Modal, Drawer) so the topmost +// overlay's focus mechanism wins and nested overlays don't fight each other. +// See: https://toptal-core.atlassian.net/browse/ER-49165 +export const defaultModalManager = new ModalManager() + +let modalIdCounter = 0 + +export const generateModalId = () => ++modalIdCounter diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9fdf6e4358..58ce311539 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1269,12 +1269,18 @@ importers: specifier: ^18.2.0 version: 18.2.0 devDependencies: + '@toptal/picasso-modal': + specifier: 4.0.0 + version: link:../Modal '@toptal/picasso-provider': specifier: 5.0.2 version: link:../../picasso-provider '@toptal/picasso-tailwind-merge': specifier: 2.0.4 version: link:../../picasso-tailwind-merge + '@toptal/picasso-test-utils': + specifier: 2.0.0 + version: link:../Test-Utils packages/base/Dropdown: dependencies: From 5d6d6f99cdcd1b64c042abde887edcff8b991699 Mon Sep 17 00:00:00 2001 From: Rafael Anachoreta Date: Tue, 12 May 2026 14:42:01 +0100 Subject: [PATCH 2/2] [ER-49165] Tighten changeset wording --- .changeset/drawer-modal-focus-recursion.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/drawer-modal-focus-recursion.md b/.changeset/drawer-modal-focus-recursion.md index ee255d53f6..89f05caeb4 100644 --- a/.changeset/drawer-modal-focus-recursion.md +++ b/.changeset/drawer-modal-focus-recursion.md @@ -5,4 +5,4 @@ '@toptal/picasso': patch --- -- fix `Maximum call stack size exceeded` when a `Drawer` is opened from inside an open `Modal` and then the `Modal` is closed (ER-49165). `Modal` uses a custom document-level focus listener (with `disableEnforceFocus` on MUI's Modal) while `Drawer` relies on MUI's built-in `FocusTrap`; with both active the two focus mechanisms recursed on `Element.focus()`. `Drawer` now registers with the same shared `ModalManager` singleton that `Modal` uses, so when a `Drawer` is on top the underlying `Modal` correctly identifies itself as no longer the topmost overlay and bails out of its focus listener. +- fix `Maximum call stack size exceeded` when a `Drawer` is opened inside an open `Modal`. `Modal` opts out of MUI's `FocusTrap` and uses its own document-level focus listener while `Drawer` keeps MUI's, so the two mechanisms recurse on `Element.focus()`. `Drawer` now registers with the shared `ModalManager` singleton, letting the underlying `Modal` bail out of its focus listener when a `Drawer` is on top.