Skip to content

Commit 6558238

Browse files
committed
[dialog] Fix nested-dialog counts and dismissal edge cases
- Aggregate nested dialog/drawer counts per child (keyed by a stable id, via Math.max) so closing one sibling no longer zeroes the parent while another stays open, which also fixes Escape/outside-press topmost gating (B4). - Accept touchend outside-press by counting changedTouches instead of the now-empty touches list (B6). - Seed store state directly on first render instead of store.update() so detached trigger subscribers aren't notified mid-render (B7). - Seed a reused handle store's openProp on first render so an initially-open dialog doesn't play an enter transition (B8). - Report the trigger on close using the triggerIdProp-aware selector (B9). - Add a nested sibling-close regression test.
1 parent 7d62b35 commit 6558238

4 files changed

Lines changed: 139 additions & 24 deletions

File tree

packages/react/src/dialog/root/DialogRoot.test.tsx

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1445,6 +1445,66 @@ describe('<Dialog.Root />', () => {
14451445
expect(outsideAfter).not.toHaveFocus();
14461446
},
14471447
);
1448+
1449+
it.skipIf(isJSDOM)(
1450+
'keeps the parent nested-dialog state when one of two sibling nested dialogs closes',
1451+
async () => {
1452+
function NestedSiblings() {
1453+
return (
1454+
<Dialog.Root modal={false}>
1455+
<Dialog.Trigger>Open parent</Dialog.Trigger>
1456+
<Dialog.Portal>
1457+
<Dialog.Popup data-testid="parent-popup">
1458+
<Dialog.Root modal={false} disablePointerDismissal>
1459+
<Dialog.Trigger>Open A</Dialog.Trigger>
1460+
<Dialog.Portal>
1461+
<Dialog.Popup data-testid="popup-a">Dialog A</Dialog.Popup>
1462+
</Dialog.Portal>
1463+
</Dialog.Root>
1464+
<Dialog.Root modal={false} disablePointerDismissal>
1465+
<Dialog.Trigger>Open B</Dialog.Trigger>
1466+
<Dialog.Portal>
1467+
<Dialog.Popup data-testid="popup-b">
1468+
<Dialog.Close>Close B</Dialog.Close>
1469+
</Dialog.Popup>
1470+
</Dialog.Portal>
1471+
</Dialog.Root>
1472+
</Dialog.Popup>
1473+
</Dialog.Portal>
1474+
</Dialog.Root>
1475+
);
1476+
}
1477+
1478+
const { user } = await render(<NestedSiblings />);
1479+
1480+
await user.click(screen.getByRole('button', { name: 'Open parent' }));
1481+
const parentPopup = await screen.findByTestId('parent-popup');
1482+
1483+
await user.click(screen.getByRole('button', { name: 'Open A' }));
1484+
await screen.findByTestId('popup-a');
1485+
1486+
await user.click(screen.getByRole('button', { name: 'Open B' }));
1487+
await screen.findByTestId('popup-b');
1488+
1489+
// Both siblings are open, so the parent reflects an open nested dialog.
1490+
expect(parentPopup).toHaveAttribute('data-nested-dialog-open');
1491+
1492+
// Closing sibling B must not zero the parent's count while sibling A stays open.
1493+
await user.click(screen.getByRole('button', { name: 'Close B' }));
1494+
await waitFor(() => {
1495+
expect(screen.queryByTestId('popup-b')).toBe(null);
1496+
});
1497+
1498+
expect(parentPopup).toHaveAttribute('data-nested-dialog-open');
1499+
1500+
// The parent is not topmost (A is still open), so Escape closes A, not the parent.
1501+
await user.keyboard('[Escape]');
1502+
await waitFor(() => {
1503+
expect(screen.queryByTestId('popup-a')).toBe(null);
1504+
});
1505+
expect(screen.queryByTestId('parent-popup')).not.toBe(null);
1506+
},
1507+
);
14481508
});
14491509

14501510
function DialogOpenChangeSpy(props: {

packages/react/src/dialog/root/useDialogRoot.ts

Lines changed: 46 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import * as React from 'react';
33
import { useScrollLock } from '@base-ui/utils/useScrollLock';
44
import { EMPTY_OBJECT } from '@base-ui/utils/empty';
5+
import { useRefWithInit } from '@base-ui/utils/useRefWithInit';
56
import { mergeProps } from '../../merge-props';
67
import { useDismiss } from '../../floating-ui-react';
78
import { contains, getTarget } from '../../floating-ui-react/utils';
@@ -17,6 +18,14 @@ import {
1718
usePopupRootSync,
1819
} from '../../utils/popups';
1920

21+
// Process-local counter for identifying nested dialogs when reporting counts to a parent.
22+
// This id is never rendered to the DOM, so it doesn't need to be SSR-stable.
23+
let nestedDialogIdCounter = 0;
24+
function getNextNestedDialogId() {
25+
nestedDialogIdCounter += 1;
26+
return `nested-dialog-${nestedDialogIdCounter}`;
27+
}
28+
2029
export function useDialogRoot(params: UseDialogRootParameters): UseDialogRootReturnValue {
2130
const { store, parentContext, actionsRef, isDrawer } = params;
2231

@@ -95,7 +104,9 @@ export function DialogInteractions({
95104
if ('button' in event && event.button !== 0) {
96105
return false;
97106
}
98-
if ('touches' in event && event.touches.length !== 1) {
107+
// On `touchend`, `touches` is empty because the finger has lifted, so count
108+
// the touch points from `changedTouches` instead.
109+
if ('changedTouches' in event && event.changedTouches.length !== 1) {
99110
return false;
100111
}
101112

@@ -122,34 +133,59 @@ export function DialogInteractions({
122133

123134
useScrollLock(open && modal === true, popupElement);
124135

136+
// Each direct child dialog reports the deepest open count in its own subtree,
137+
// keyed by a stable id. Aggregating with `Math.max` (rather than letting the
138+
// last reporter overwrite) keeps the counts correct when sibling dialogs open
139+
// and close independently: a closing sibling no longer zeroes the parent's
140+
// count while another sibling stays open.
141+
const nestedDialogId = useRefWithInit(getNextNestedDialogId).current;
142+
const nestedChildContributionsRef = useRefWithInit(
143+
() => new Map<string, { dialogs: number; drawers: number }>(),
144+
);
145+
146+
function recomputeNestedCounts() {
147+
let dialogs = 0;
148+
let drawers = 0;
149+
nestedChildContributionsRef.current.forEach((contribution) => {
150+
dialogs = Math.max(dialogs, contribution.dialogs);
151+
drawers = Math.max(drawers, contribution.drawers);
152+
});
153+
setOwnNestedOpenDialogs(dialogs);
154+
setOwnNestedOpenDrawers(drawers);
155+
}
156+
125157
// Listen for nested open/close events on this store to maintain the counts.
126-
store.useContextCallback('onNestedDialogOpen', (dialogCount, drawerCount) => {
127-
setOwnNestedOpenDialogs(dialogCount);
128-
setOwnNestedOpenDrawers(drawerCount);
158+
store.useContextCallback('onNestedDialogOpen', (childId, dialogCount, drawerCount) => {
159+
nestedChildContributionsRef.current.set(childId, {
160+
dialogs: dialogCount,
161+
drawers: drawerCount,
162+
});
163+
recomputeNestedCounts();
129164
});
130165

131-
store.useContextCallback('onNestedDialogClose', () => {
132-
setOwnNestedOpenDialogs(0);
133-
setOwnNestedOpenDrawers(0);
166+
store.useContextCallback('onNestedDialogClose', (childId) => {
167+
nestedChildContributionsRef.current.delete(childId);
168+
recomputeNestedCounts();
134169
});
135170

136171
// Notify parent of our open/close state using parent callbacks, if any
137172
React.useEffect(() => {
138173
if (parentContext?.onNestedDialogOpen && open) {
139174
parentContext.onNestedDialogOpen(
175+
nestedDialogId,
140176
ownNestedOpenDialogs + 1,
141177
ownNestedOpenDrawers + (isDrawer ? 1 : 0),
142178
);
143179
}
144180
if (parentContext?.onNestedDialogClose && !open) {
145-
parentContext.onNestedDialogClose();
181+
parentContext.onNestedDialogClose(nestedDialogId);
146182
}
147183
return () => {
148184
if (parentContext?.onNestedDialogClose && open) {
149-
parentContext.onNestedDialogClose();
185+
parentContext.onNestedDialogClose(nestedDialogId);
150186
}
151187
};
152-
}, [isDrawer, open, ownNestedOpenDialogs, ownNestedOpenDrawers, parentContext]);
188+
}, [isDrawer, open, ownNestedOpenDialogs, ownNestedOpenDrawers, parentContext, nestedDialogId]);
153189

154190
const activeTriggerProps = dismiss.reference ?? EMPTY_OBJECT;
155191
const inactiveTriggerProps = dismiss.trigger ?? EMPTY_OBJECT;

packages/react/src/dialog/root/useRenderDialogRoot.tsx

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,19 +42,30 @@ export function useRenderDialogRoot<Payload>(
4242
...rootState,
4343
});
4444

45-
// Support initially open state when uncontrolled
4645
useOnFirstRender(() => {
47-
const nextState =
48-
openProp === undefined && store.state.open === false && defaultOpen === true
49-
? { open: true, activeTriggerId: defaultTriggerIdProp }
50-
: null;
46+
// Support initially open state when uncontrolled.
47+
const shouldOpen = openProp === undefined && store.state.open === false && defaultOpen === true;
48+
// A reused handle store isn't seeded with `openProp` from `initialState` (the handle
49+
// is created before the Root knows the prop). Seed it during the first render so the
50+
// open selector reflects the controlled value immediately; otherwise an initially-open
51+
// dialog would flip from closed to open after the first paint and play an unwanted
52+
// enter transition (`useTransitionStatus`).
53+
const shouldSeedOpenProp = openProp !== undefined && store.state.openProp !== openProp;
5154

52-
if (isAlertDialog) {
53-
// Handles can reuse plain Dialog stores; alert dialog invariants must exist immediately.
54-
store.update(nextState ? { ...rootState, ...nextState } : rootState);
55-
} else if (nextState) {
56-
store.update(nextState);
55+
if (!isAlertDialog && !shouldOpen && !shouldSeedOpenProp) {
56+
return;
5757
}
58+
59+
// Assign `store.state` directly instead of calling `store.update` (which notifies
60+
// synchronously). This runs during render, so notifying detached-trigger subscribers
61+
// here would update other components mid-render.
62+
store.state = {
63+
...store.state,
64+
// Handles can reuse plain Dialog stores; alert dialog invariants must exist immediately.
65+
...(isAlertDialog ? rootState : null),
66+
...(shouldSeedOpenProp ? { openProp } : null),
67+
...(shouldOpen ? { open: true, activeTriggerId: defaultTriggerIdProp } : null),
68+
};
5869
});
5970

6071
store.useControlledProp('openProp', openProp);

packages/react/src/dialog/store/DialogStore.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,10 @@ type Context = PopupStoreContext<DialogRoot.ChangeEventDetails> & {
3131
readonly backdropRef: React.RefObject<HTMLDivElement | null>;
3232
readonly internalBackdropRef: React.RefObject<HTMLDivElement | null>;
3333
readonly outsidePressEnabledRef: React.MutableRefObject<boolean>;
34-
readonly onNestedDialogOpen?: ((dialogCount: number, drawerCount: number) => void) | undefined;
35-
readonly onNestedDialogClose?: (() => void) | undefined;
34+
readonly onNestedDialogOpen?:
35+
| ((nestedId: string, dialogCount: number, drawerCount: number) => void)
36+
| undefined;
37+
readonly onNestedDialogClose?: ((nestedId: string) => void) | undefined;
3638
};
3739

3840
const selectors = {
@@ -87,10 +89,16 @@ export class DialogStore<Payload> extends ReactStore<
8789
this.set('preventUnmountingOnClose', true);
8890
};
8991

90-
if (!nextOpen && eventDetails.trigger == null && this.state.activeTriggerId != null) {
92+
// Use the `triggerIdProp`-aware selector so fully controlled mode (`open` + `triggerId`
93+
// props) reports the trigger on close, matching the uncontrolled flow.
94+
const activeTriggerId = this.select('activeTriggerId');
95+
if (!nextOpen && eventDetails.trigger == null && activeTriggerId != null) {
9196
// When closing the dialog, pass the old trigger to the onOpenChange event
9297
// so it's not reset too early (potentially causing focus issues in controlled scenarios).
93-
eventDetails.trigger = this.state.activeTriggerElement ?? undefined;
98+
eventDetails.trigger =
99+
this.state.activeTriggerElement ??
100+
(this.context.triggerElements.getById(activeTriggerId) as Element | undefined) ??
101+
undefined;
94102
}
95103

96104
this.context.onOpenChange?.(nextOpen, eventDetails as DialogRoot.ChangeEventDetails);

0 commit comments

Comments
 (0)