From b38becd6ea8ef1e7bcfdc070ab751cbbe7e7fad4 Mon Sep 17 00:00:00 2001 From: atomiks Date: Mon, 27 Jul 2026 11:39:39 +1000 Subject: [PATCH] [all components] Correct effect timing (#5337) --- .../src/accordion/panel/AccordionPanel.tsx | 2 +- .../src/accordion/root/AccordionRoot.tsx | 3 +- .../collapsible/panel/CollapsiblePanel.tsx | 2 +- .../react/src/dialog/root/DialogRoot.test.tsx | 72 ++++ .../react/src/dialog/root/useDialogRoot.ts | 3 +- .../src/drawer/popup/DrawerPopup.test.tsx | 108 ++++++ .../react/src/drawer/popup/DrawerPopup.tsx | 4 +- .../drawer/provider/DrawerProvider.test.tsx | 29 +- .../src/drawer/provider/DrawerProvider.tsx | 46 +-- .../drawer/provider/DrawerProviderContext.ts | 4 +- .../react/src/drawer/root/DrawerRoot.test.tsx | 112 +++++- packages/react/src/drawer/root/DrawerRoot.tsx | 24 +- .../swipe-area/DrawerSwipeArea.test.tsx | 118 +++++-- .../src/drawer/swipe-area/DrawerSwipeArea.tsx | 14 +- .../drawer/viewport/DrawerViewport.test.tsx | 318 ++++++++++++++++++ .../src/drawer/viewport/DrawerViewport.tsx | 9 +- .../submenu-trigger/MenuSubmenuTrigger.tsx | 3 +- packages/react/src/toggle/Toggle.tsx | 3 +- 18 files changed, 753 insertions(+), 121 deletions(-) diff --git a/packages/react/src/accordion/panel/AccordionPanel.tsx b/packages/react/src/accordion/panel/AccordionPanel.tsx index 5cc5faa7a..420ce3365 100644 --- a/packages/react/src/accordion/panel/AccordionPanel.tsx +++ b/packages/react/src/accordion/panel/AccordionPanel.tsx @@ -54,7 +54,7 @@ export const AccordionPanel = React.forwardRef(function AccordionPanel( if (process.env.NODE_ENV !== 'production') { // eslint-disable-next-line react-hooks/rules-of-hooks - useIsoLayoutEffect(() => { + React.useEffect(() => { if (keepMountedProp === false && hiddenUntilFound) { warn( 'The `keepMounted={false}` prop on an `Accordion.Panel` is ignored when `hiddenUntilFound` is enabled on the panel or root, since the panel must remain mounted while closed.', diff --git a/packages/react/src/accordion/root/AccordionRoot.tsx b/packages/react/src/accordion/root/AccordionRoot.tsx index 0e845382f..cbec3483f 100644 --- a/packages/react/src/accordion/root/AccordionRoot.tsx +++ b/packages/react/src/accordion/root/AccordionRoot.tsx @@ -2,7 +2,6 @@ import * as React from 'react'; import { useControlled } from '@base-ui/utils/useControlled'; import { useStableCallback } from '@base-ui/utils/useStableCallback'; -import { useIsoLayoutEffect } from '@base-ui/utils/useIsoLayoutEffect'; import { warn } from '@base-ui/utils/warn'; import { BaseUIComponentProps, Orientation } from '../../internals/types'; import { CompositeList } from '../../internals/composite/list/CompositeList'; @@ -43,7 +42,7 @@ export const AccordionRoot = React.forwardRef(function AccordionRoot { + React.useEffect(() => { if (hiddenUntilFoundProp && keepMountedProp === false) { warn( 'The `keepMounted={false}` prop on `Accordion.Root` is ignored when `hiddenUntilFound` is enabled, since panels must remain mounted while closed.', diff --git a/packages/react/src/collapsible/panel/CollapsiblePanel.tsx b/packages/react/src/collapsible/panel/CollapsiblePanel.tsx index e117e3ea4..e601bb9d7 100644 --- a/packages/react/src/collapsible/panel/CollapsiblePanel.tsx +++ b/packages/react/src/collapsible/panel/CollapsiblePanel.tsx @@ -34,7 +34,7 @@ export const CollapsiblePanel = React.forwardRef(function CollapsiblePanel( if (process.env.NODE_ENV !== 'production') { // eslint-disable-next-line react-hooks/rules-of-hooks - useIsoLayoutEffect(() => { + React.useEffect(() => { if (hiddenUntilFoundProp && keepMountedProp === false) { warn( 'The `keepMounted={false}` prop on `Collapsible.Panel` is ignored when `hiddenUntilFound` is enabled, since the panel must remain mounted while closed.', diff --git a/packages/react/src/dialog/root/DialogRoot.test.tsx b/packages/react/src/dialog/root/DialogRoot.test.tsx index fe323e6f5..3566f8401 100644 --- a/packages/react/src/dialog/root/DialogRoot.test.tsx +++ b/packages/react/src/dialog/root/DialogRoot.test.tsx @@ -9,9 +9,12 @@ import { Select } from '@base-ui/react/select'; import { NumberField } from '@base-ui/react/number-field'; import { ScrollArea } from '@base-ui/react/scroll-area'; import { useRefWithInit } from '@base-ui/utils/useRefWithInit'; +import { useIsoLayoutEffect } from '@base-ui/utils/useIsoLayoutEffect'; import { useTimeout } from '@base-ui/utils/useTimeout'; import { REASONS } from '../../internals/reasons'; import { useDialogRootContext } from './DialogRootContext'; +import { DialogStore } from '../store/DialogStore'; +import { DialogInteractions } from './useDialogRoot'; describe('', () => { const { render } = createRenderer(); @@ -34,6 +37,75 @@ describe('', () => { expectedPopupRole: 'dialog', }); + it('reports nested drawer counts before passive effects', async () => { + const childStore = new DialogStore( + { + open: true, + mounted: true, + modal: false, + disablePointerDismissal: false, + }, + undefined, + true, + ); + const parentStore = new DialogStore(undefined, undefined, false); + let mountPassiveEffectFlushed = false; + let teardownPassiveEffectFlushed = false; + let mountedBeforePassiveEffect: boolean | null = null; + let unmountedBeforePassiveEffect: boolean | null = null; + let phase: 'mount' | 'teardown' = 'mount'; + + Object.assign(parentStore.context, { + onNestedDialogOpen(dialogCount: number, drawerCount: number) { + if (drawerCount > 0 && mountedBeforePassiveEffect === null) { + mountedBeforePassiveEffect = !mountPassiveEffectFlushed; + } + if ( + phase === 'teardown' && + dialogCount === 0 && + drawerCount === 0 && + unmountedBeforePassiveEffect === null + ) { + unmountedBeforePassiveEffect = !teardownPassiveEffectFlushed; + } + }, + }); + + function PassiveEffectBoundary() { + useIsoLayoutEffect( + () => () => { + teardownPassiveEffectFlushed = false; + }, + [], + ); + + React.useEffect(() => { + mountPassiveEffectFlushed = true; + return () => { + teardownPassiveEffectFlushed = true; + }; + }, []); + + return null; + } + + const { unmount } = await render( + + + + , + ); + + expect(mountedBeforePassiveEffect).toBe(true); + + phase = 'teardown'; + await act(async () => { + unmount(); + }); + + expect(unmountedBeforePassiveEffect).toBe(true); + }); + it('keeps trigger ownership when another trigger mounts while open', async () => { function Test() { const [showSecondTrigger, setShowSecondTrigger] = React.useState(false); diff --git a/packages/react/src/dialog/root/useDialogRoot.ts b/packages/react/src/dialog/root/useDialogRoot.ts index 8beccefd0..74ed59fba 100644 --- a/packages/react/src/dialog/root/useDialogRoot.ts +++ b/packages/react/src/dialog/root/useDialogRoot.ts @@ -1,5 +1,6 @@ 'use client'; import * as React from 'react'; +import { useIsoLayoutEffect } from '@base-ui/utils/useIsoLayoutEffect'; import { useScrollLock } from '@base-ui/utils/useScrollLock'; import { useDismiss } from '../../floating-ui-react'; import { contains, getTarget } from '../../floating-ui-react/utils'; @@ -92,7 +93,7 @@ export function DialogInteractions({ }); // Notify parent of our open/close state using parent callbacks, if any - React.useEffect(() => { + useIsoLayoutEffect(() => { if (parentContext?.onNestedDialogOpen) { if (open) { parentContext.onNestedDialogOpen( diff --git a/packages/react/src/drawer/popup/DrawerPopup.test.tsx b/packages/react/src/drawer/popup/DrawerPopup.test.tsx index 647998eba..5e2095e0a 100644 --- a/packages/react/src/drawer/popup/DrawerPopup.test.tsx +++ b/packages/react/src/drawer/popup/DrawerPopup.test.tsx @@ -4,8 +4,10 @@ import { AlertDialog } from '@base-ui/react/alert-dialog'; import { Dialog } from '@base-ui/react/dialog'; import { Drawer } from '@base-ui/react/drawer'; import { SafeReact } from '@base-ui/utils/safeReact'; +import { useIsoLayoutEffect } from '@base-ui/utils/useIsoLayoutEffect'; import { act, fireEvent, screen, waitFor } from '@mui/internal-test-utils'; import { createRenderer, describeConformance, isJSDOM } from '#test-utils'; +import { useDialogRootContext } from '../../dialog/root/DialogRootContext'; import { useDrawerRootContext } from '../root/DrawerRootContext'; function setHeight(element: HTMLElement | null, getValue: () => number) { @@ -338,6 +340,112 @@ describe('', () => { }, ); + it('reports nested presence and frontmost height before passive effects', async () => { + let passiveEffectFlushed = false; + let heightReportedBeforePassiveEffect: boolean | null = null; + let presenceReportedBeforePassiveEffect: boolean | null = null; + const patchedDrawerContexts = new WeakSet(); + + function PassiveEffectBoundary({ childOpen }: { childOpen: boolean }) { + React.useEffect(() => { + if (childOpen) { + passiveEffectFlushed = true; + } + }, [childOpen]); + + return null; + } + + function ChildDrawerMethodProbe() { + const context = useDrawerRootContext(); + + if (!patchedDrawerContexts.has(context)) { + patchedDrawerContexts.add(context); + const notifyParentFrontmostHeight = context.notifyParentFrontmostHeight; + const notifyParentHasNestedDrawer = context.notifyParentHasNestedDrawer; + + context.notifyParentFrontmostHeight = (height) => { + if (height > 0 && heightReportedBeforePassiveEffect === null) { + heightReportedBeforePassiveEffect = !passiveEffectFlushed; + } + notifyParentFrontmostHeight?.(height); + }; + context.notifyParentHasNestedDrawer = (present) => { + if (present && presenceReportedBeforePassiveEffect === null) { + presenceReportedBeforePassiveEffect = !passiveEffectFlushed; + } + notifyParentHasNestedDrawer?.(present); + }; + } + + return null; + } + + function PopupPhaseBoundary() { + useDialogRootContext().useState('open'); + useDrawerRootContext(); + passiveEffectFlushed = false; + + useIsoLayoutEffect(() => { + passiveEffectFlushed = false; + }); + + React.useEffect(() => { + passiveEffectFlushed = true; + }); + + return null; + } + + function TestCase({ childOpen }: { childOpen: boolean }) { + return ( + + + + + + + + + + + { + if (element) { + Object.defineProperty(element, 'offsetHeight', { + configurable: true, + value: 100, + }); + } + }} + > + + Child drawer + + + + + + + + + + ); + } + + const { setProps } = await render(); + + await setProps({ childOpen: true }); + + expect(heightReportedBeforePassiveEffect).toBe(true); + expect(presenceReportedBeforePassiveEffect).toBe(true); + const parentPopup = screen.getByTestId('parent-popup'); + expect(parentPopup.style.getPropertyValue('--nested-drawers')).toBe('1'); + expect(parentPopup).toHaveAttribute('data-nested-drawer-open', ''); + expect(parentPopup.style.getPropertyValue('--drawer-frontmost-height')).toBe('100px'); + }); + it('does not treat dialogs inside nested drawers as nested drawers', async () => { await render( diff --git a/packages/react/src/drawer/popup/DrawerPopup.tsx b/packages/react/src/drawer/popup/DrawerPopup.tsx index ac75c45d7..9f706f94a 100644 --- a/packages/react/src/drawer/popup/DrawerPopup.tsx +++ b/packages/react/src/drawer/popup/DrawerPopup.tsx @@ -267,7 +267,7 @@ export const DrawerPopup = React.forwardRef(function DrawerPopup( }; }, [nestedSwipeProgressStore, popupRef]); - React.useEffect(() => { + useIsoLayoutEffect(() => { if (!open) { return undefined; } @@ -279,7 +279,7 @@ export const DrawerPopup = React.forwardRef(function DrawerPopup( }; }, [frontmostHeight, open, notifyParentFrontmostHeight]); - React.useEffect(() => { + useIsoLayoutEffect(() => { if (!notifyParentHasNestedDrawer) { return undefined; } diff --git a/packages/react/src/drawer/provider/DrawerProvider.test.tsx b/packages/react/src/drawer/provider/DrawerProvider.test.tsx index 83e3bbbe1..c6fefe661 100644 --- a/packages/react/src/drawer/provider/DrawerProvider.test.tsx +++ b/packages/react/src/drawer/provider/DrawerProvider.test.tsx @@ -1,10 +1,13 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import * as React from 'react'; import { Drawer } from '@base-ui/react/drawer'; import { screen } from '@mui/internal-test-utils'; import { createRenderer } from '#test-utils'; import { useDrawerProviderContext } from './DrawerProviderContext'; +const manualDrawer = {}; +const missingDrawer = {}; + function ProviderControls() { const context = useDrawerProviderContext(); @@ -14,10 +17,10 @@ function ProviderControls() { return ( - - - - + + + +