Skip to content

Commit 234bad7

Browse files
michaldudakclaude
michaldudak
and
claude
committed
[popover][combobox] Commit request metadata only when the consumer accepts
`setOpen` wrote its whole result before the controlled consumer had a chance to reflect the request back through `open`. A request the consumer simply ignored — without cancelling it — therefore left its reason, its transition style and its unmount override behind in internal state that nothing ever corrected, because the reconciliation only re-ran when the prop itself changed. The consequences were live. An ignored hover request left `openReason` on `trigger-hover`, so a later programmatic open was classified as a hover session: no initial focus, no trap, no scroll lock and no internal backdrop on a modal popover. An ignored dismissal left `instantType` on `dismiss`, cancelling the next transition. In Combobox the same shape reached focus: a declined outside press latched a close reason that never expired, so the next close skipped the handoff to the external input and `inert` dropped focus to `<body>`. Both now follow one rule. A controlled change reflected back in the request's own synchronous React transaction keeps the interaction's provenance. A prop that stays put declines the request, and everything it would have committed is discarded. A prop that moves on its own is programmatic — including a deferred `startTransition` or an asynchronous store update, which cannot be told apart from a decline without a public acknowledgement mechanism. Trigger ownership, raw `open` and `dispatchOpenChange` stay immediate: raw `open` is what makes a request observable, ownership is overwritten by whichever request is accepted, and the emitter is load-bearing for close-time focus behaviour. Reclassifying a session that is already running — an impatient click promoting a hover-opened popover — commits immediately too, since there is nothing to accept. `useControlledOpenProvenance` owns the effect ordering that used to be a comment. Reconciliation has to observe the raw value the request left behind before `useControlledProp` overwrites the effective one, and a deferred commit has to land before the effective `open` transition it belongs to is processed — otherwise a popup the consumer asked to keep mounted would unmount immediately. Combobox has no raw/effective split to read, and `useControlled`'s setter is inert while controlled, so a declined request renders nothing on its own. A request counter forces exactly one pass, and the effect that publishes the effective `open` value settles the pending reason into `closeReason` in the same write. Popover's `openChangeReason` had no readers left and is removed. Menu keeps its own close/open reason asymmetry, which is deliberately untouched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 6b8f3e8 commit 234bad7

11 files changed

Lines changed: 391 additions & 97 deletions

File tree

‎packages/react/src/combobox/popup/ComboboxPopup.test.tsx‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,77 @@ describe('<Combobox.Popup />', () => {
248248
expect(document.body).toHaveFocus();
249249
});
250250

251+
it('hands focus back on a close that follows a declined outside press', async ({
252+
onTestFinished,
253+
}) => {
254+
globalThis.BASE_UI_ANIMATIONS_DISABLED = false;
255+
onTestFinished(() => {
256+
globalThis.BASE_UI_ANIMATIONS_DISABLED = true;
257+
});
258+
259+
function Test() {
260+
const [open, setOpen] = React.useState(true);
261+
return (
262+
<React.Fragment>
263+
{/* eslint-disable-next-line react/no-danger */}
264+
<style dangerouslySetInnerHTML={{ __html: style }} />
265+
<p data-testid="plain">Not focusable</p>
266+
<button type="button" data-testid="close-externally" onClick={() => setOpen(false)}>
267+
Close
268+
</button>
269+
<Combobox.Root
270+
items={['a', 'b']}
271+
open={open}
272+
onOpenChange={(nextOpen) => {
273+
// Accepts opens, declines every close request.
274+
if (nextOpen) {
275+
setOpen(true);
276+
}
277+
}}
278+
>
279+
<Combobox.Input data-testid="input" />
280+
<Combobox.Portal>
281+
<Combobox.Positioner>
282+
<Combobox.Popup data-testid="popup" className="animation-test-popup">
283+
<Combobox.List>
284+
{(item: string) => (
285+
<Combobox.Item key={item} value={item}>
286+
{item}
287+
</Combobox.Item>
288+
)}
289+
</Combobox.List>
290+
<button type="button" data-testid="inside">
291+
Create new
292+
</button>
293+
</Combobox.Popup>
294+
</Combobox.Positioner>
295+
</Combobox.Portal>
296+
</Combobox.Root>
297+
</React.Fragment>
298+
);
299+
}
300+
301+
const { user } = await render(<Test />);
302+
const inside = await screen.findByTestId('inside');
303+
inside.focus();
304+
expect(inside).toHaveFocus();
305+
306+
// The consumer declines this dismissal, so the popup stays open and the request's reason
307+
// must not outlive it.
308+
await user.click(screen.getByTestId('plain'));
309+
expect(screen.getByTestId('popup')).not.toHaveAttribute('data-ending-style');
310+
311+
inside.focus();
312+
expect(inside).toHaveFocus();
313+
314+
// A programmatic close carries no reason of its own. Reusing the declined outside press's
315+
// reason would skip the handoff and let `inert` drop focus to `<body>`.
316+
fireEvent.click(screen.getByTestId('close-externally'));
317+
318+
expect(screen.getByTestId('popup')).toHaveAttribute('data-ending-style');
319+
await waitFor(() => expect(screen.getByTestId('input')).toHaveFocus());
320+
});
321+
251322
it('starts each close with fresh return-focus state while the popup stays mounted', async ({
252323
onTestFinished,
253324
}) => {

‎packages/react/src/combobox/popup/ComboboxPopup.tsx‎

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import { StateAttributesMapping } from '../../internals/getStateAttributesProps'
2020
import { activeElement, contains, getTarget } from '../../floating-ui-react/utils';
2121
import { getDisabledMountTransitionStyles } from '../../internals/getDisabledMountTransitionStyles';
2222
import { REASONS } from '../../internals/reasons';
23-
import type { FloatingUIOpenChangeDetails } from '../../internals/types';
2423
import { ComboboxInternalDismissButton } from '../utils/ComboboxInternalDismissButton';
2524
import { getComboboxPopupId } from '../root/utils';
2625
import { useListEmpty } from '../utils/parts';
@@ -120,32 +119,14 @@ export const ComboboxPopup = React.forwardRef(function ComboboxPopup(
120119
const resolvedInitialFocus =
121120
initialFocus === undefined ? computedDefaultInitialFocus : initialFocus;
122121

123-
const closedByOutsidePressRef = React.useRef(false);
124-
useIsoLayoutEffect(() => {
125-
if (open) {
126-
closedByOutsidePressRef.current = false;
127-
}
128-
}, [open]);
129-
130-
useIsoLayoutEffect(() => {
131-
const events = floatingRootContext.context.events;
132-
133-
function onOpenChange(details: FloatingUIOpenChangeDetails) {
134-
if (!details.open) {
135-
closedByOutsidePressRef.current = details.reason === REASONS.outsidePress;
136-
}
137-
}
138-
139-
events.on('openchange', onOpenChange);
140-
return () => {
141-
events.off('openchange', onOpenChange);
142-
};
143-
}, [floatingRootContext]);
144-
122+
// Making the closing popup inert blurs whatever it held, so a control inside it that had focus
123+
// would otherwise strand it on `<body>`. An outside press is the user moving focus themselves,
124+
// and the store records the reason of the close that actually committed rather than the reason
125+
// of a request a controlled consumer may have declined.
145126
const returnFocusToExternalInput = useStableCallback(() => {
146127
const positionerElement = store.state.positionerElement;
147128
if (
148-
!closedByOutsidePressRef.current &&
129+
store.state.closeReason !== REASONS.outsidePress &&
149130
positionerElement &&
150131
contains(positionerElement, activeElement(ownerDocument(positionerElement)))
151132
) {

‎packages/react/src/combobox/root/AriaCombobox.tsx‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,15 @@ export function AriaCombobox<Value = any, Mode extends SelectionMode = 'none', I
331331
state: 'open',
332332
});
333333

334+
// The reason of a close request that has not been settled yet. A controlled consumer can decline
335+
// it, and `onOpenChange` fires before that decision is observable, so the reason only becomes the
336+
// committed close reason once `open` has actually moved.
337+
const pendingCloseReasonRef = React.useRef<AriaCombobox.ChangeEventReason | null>(null);
338+
// Declining a controlled request changes no state of the consumer's own, and `useControlled`'s
339+
// setter is inert while controlled, so nothing would re-render to settle the pending reason.
340+
// Bumping this forces exactly one pass, whether the request is accepted or declined.
341+
const [controlledRequestVersion, setControlledRequestVersion] = React.useState(0);
342+
334343
const isGrouped = isGroupedItems(items);
335344
const query = !open && closeQuery !== null ? closeQuery : String(inputValue).trim();
336345

@@ -468,6 +477,7 @@ export function AriaCombobox<Value = any, Mode extends SelectionMode = 'none', I
468477
labelId: undefined,
469478
selectedValue,
470479
open,
480+
closeReason: null,
471481
items: storeItems,
472482
selectionMode,
473483
listRef,
@@ -762,6 +772,13 @@ export function AriaCombobox<Value = any, Mode extends SelectionMode = 'none', I
762772
return;
763773
}
764774

775+
// Recorded after cancellation so a cancelled request leaves nothing behind. Settled by the
776+
// synchronization effect below, which is the only place that can see whether `open` moved.
777+
pendingCloseReasonRef.current = nextOpen ? null : eventDetails.reason;
778+
if (openProp !== undefined) {
779+
setControlledRequestVersion((version) => version + 1);
780+
}
781+
765782
if (nextOpen && closeQuery !== null) {
766783
// `ComboboxInput` calls `setInputValue` before `setOpen`, so on an input-change reopen
767784
// `inputValue` is still the pre-keystroke value and the typed filter always survives.
@@ -1426,10 +1443,19 @@ export function AriaCombobox<Value = any, Mode extends SelectionMode = 'none', I
14261443
});
14271444

14281445
useIsoLayoutEffect(() => {
1446+
// Settle the request that may have asked for this value. The popup being open means any
1447+
// pending request was declined or superseded; the popup being closed with a request in flight
1448+
// means that request is the one that committed. A close with nothing in flight moved the state
1449+
// directly and carries no reason.
1450+
const pendingCloseReason = pendingCloseReasonRef.current;
1451+
pendingCloseReasonRef.current = null;
1452+
const closeReason = open ? null : pendingCloseReason;
1453+
14291454
store.update({
14301455
id,
14311456
selectedValue,
14321457
open,
1458+
closeReason,
14331459
mounted,
14341460
transitionStatus,
14351461
items: storeItems,
@@ -1462,6 +1488,7 @@ export function AriaCombobox<Value = any, Mode extends SelectionMode = 'none', I
14621488
id,
14631489
selectedValue,
14641490
open,
1491+
controlledRequestVersion,
14651492
mounted,
14661493
transitionStatus,
14671494
storeItems,

‎packages/react/src/combobox/store.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,14 @@ export type State = {
1919
mounted: boolean;
2020
transitionStatus: TransitionStatus;
2121
forceMounted: boolean;
22+
/**
23+
* The reason of the last close that actually committed, cleared whenever the popup is open.
24+
*
25+
* A close request can be declined by a controlled consumer, and the request event fires before
26+
* that decision is visible, so the request's reason is not a safe record of how the popup
27+
* closed. This is written alongside the effective `open` value it belongs to.
28+
*/
29+
closeReason: AriaCombobox.ChangeEventReason | null;
2230

2331
inline: boolean;
2432

@@ -121,6 +129,7 @@ export const selectors = {
121129
open: (state: State) => state.open,
122130
mounted: (state: State) => state.mounted,
123131
forceMounted: (state: State) => state.forceMounted,
132+
closeReason: (state: State) => state.closeReason,
124133

125134
inline: (state: State) => state.inline,
126135

‎packages/react/src/menu/trigger/MenuTrigger.tsx‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -209,8 +209,10 @@ export const MenuTrigger = fastComponentRef(function MenuTrigger(
209209

210210
const rootTriggerProps = store.useState('triggerProps', isMountedByThisTrigger);
211211

212-
const { preFocusGuardRef, handlePreFocusGuardFocus, handleFocusTargetFocus } =
213-
useTriggerFocusGuards(store, triggerElementRef);
212+
const { handlePreFocusGuardFocus, handleFocusTargetFocus } = useTriggerFocusGuards(
213+
store,
214+
triggerElementRef,
215+
);
214216

215217
const state: MenuTriggerState = {
216218
disabled,
@@ -275,11 +277,7 @@ export const MenuTrigger = fastComponentRef(function MenuTrigger(
275277
if (isOpenedByThisTrigger) {
276278
return (
277279
<React.Fragment>
278-
<FocusGuard
279-
ref={preFocusGuardRef}
280-
onFocus={handlePreFocusGuardFocus}
281-
key={`${thisTriggerId}-pre-focus-guard`}
282-
/>
280+
<FocusGuard onFocus={handlePreFocusGuardFocus} key={`${thisTriggerId}-pre-focus-guard`} />
283281
<React.Fragment key={thisTriggerId}>{element}</React.Fragment>
284282
<FocusGuard
285283
ref={store.context.triggerFocusTargetRef}

‎packages/react/src/popover/root/PopoverRoot.test.tsx‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,89 @@ describe('<Popover.Root />', () => {
9494
});
9595

9696
describe('controlled open', () => {
97+
it('does not let an ignored request classify a later programmatic open', async () => {
98+
function App() {
99+
const [open, setOpen] = React.useState(false);
100+
101+
return (
102+
<React.Fragment>
103+
<button type="button" data-testid="open-externally" onClick={() => setOpen(true)}>
104+
Open
105+
</button>
106+
<Popover.Root
107+
open={open}
108+
onOpenChange={() => {
109+
// Deliberately ignores every request without cancelling it, which is what a
110+
// consumer filtering interactions through its own state looks like.
111+
}}
112+
>
113+
<Popover.Trigger openOnHover delay={0} data-testid="trigger">
114+
Trigger
115+
</Popover.Trigger>
116+
<Popover.Portal>
117+
<Popover.Positioner>
118+
<Popover.Popup>
119+
<button type="button" data-testid="inside">
120+
Inside
121+
</button>
122+
</Popover.Popup>
123+
</Popover.Positioner>
124+
</Popover.Portal>
125+
</Popover.Root>
126+
</React.Fragment>
127+
);
128+
}
129+
130+
const { user } = await render(<App />);
131+
132+
await user.hover(screen.getByTestId('trigger'));
133+
expect(screen.queryByTestId('inside')).toBe(null);
134+
135+
// Dispatched without moving the pointer: a real click elsewhere would fire a hover-close
136+
// on the way, and that second request would mask the stale classification.
137+
fireEvent.click(screen.getByTestId('open-externally'));
138+
139+
// A programmatic open carries no interaction reason, so the focus manager takes initial
140+
// focus. Reusing the ignored hover request's reason would classify this as a hover session
141+
// and leave focus behind on the button that opened it.
142+
await waitFor(() => expect(screen.getByTestId('inside')).toHaveFocus());
143+
});
144+
145+
it('does not let an ignored close request stamp the transition style', async () => {
146+
function App() {
147+
const [open, setOpen] = React.useState(true);
148+
149+
return (
150+
<Popover.Root
151+
open={open}
152+
onOpenChange={(nextOpen) => {
153+
// Accepts opens, ignores every close request.
154+
if (nextOpen) {
155+
setOpen(true);
156+
}
157+
}}
158+
>
159+
<Popover.Trigger data-testid="trigger">Trigger</Popover.Trigger>
160+
<Popover.Portal>
161+
<Popover.Positioner>
162+
<Popover.Popup data-testid="popup">Content</Popover.Popup>
163+
</Popover.Positioner>
164+
</Popover.Portal>
165+
</Popover.Root>
166+
);
167+
}
168+
169+
const { user } = await render(<App />);
170+
expect(screen.getByTestId('popup')).not.toHaveAttribute('data-instant');
171+
172+
// The consumer declines this dismissal, so nothing it would have committed may survive —
173+
// including the transition style, which would otherwise cancel the next animation.
174+
await user.keyboard('{Escape}');
175+
await flushMicrotasks();
176+
177+
expect(screen.getByTestId('popup')).not.toHaveAttribute('data-instant');
178+
});
179+
97180
it('should call onChange when the open state changes', async () => {
98181
const handleChange = vi.fn();
99182

‎packages/react/src/popover/root/PopoverRoot.tsx‎

Lines changed: 3 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
'use client';
22
import * as React from 'react';
33
import { fastComponent } from '@base-ui/utils/fastHooks';
4-
import { useIsoLayoutEffect } from '@base-ui/utils/useIsoLayoutEffect';
54
import { useDismiss, FloatingTree } from '../../floating-ui-react';
65
import { PopoverRootContext, usePopoverRootContext } from './PopoverRootContext';
6+
import { useControlledOpenProvenance } from './useControlledOpenProvenance';
77
import { PopoverStore, type State as PopoverStoreState } from '../store/PopoverStore';
88
import { PopoverHandle } from '../store/PopoverHandle';
99
import {
@@ -46,30 +46,7 @@ const PopoverRootComponent = fastComponent(function PopoverRootComponent<Payload
4646
triggerIdProp,
4747
});
4848

49-
// Registered before `useControlledProp` so it observes raw `open` before that effect writes
50-
// `openProp` into the store.
51-
//
52-
// The effective open state is `openProp ?? open`, and `setOpen` writes raw `open` only after the
53-
// consumer declined to cancel. So when a controlled prop changes, raw `open` already matching it
54-
// means an interaction request was accepted and the session keeps the reason that request
55-
// recorded. A mismatch means the parent moved the state itself: a programmatic session with no
56-
// interaction reason.
57-
useIsoLayoutEffect(() => {
58-
if (openProp === undefined || store.state.open === openProp) {
59-
return;
60-
}
61-
62-
if (openProp) {
63-
store.update({ open: true, openReason: null });
64-
} else {
65-
// Mirror a direct close into raw `open`, otherwise the next direct open would look like
66-
// acceptance of this one. The session reason survives until unmount: the popup is still
67-
// closing, and everything gated on it must keep behaving the way the session started.
68-
store.set('open', false);
69-
}
70-
}, [store, openProp]);
71-
72-
store.useControlledProp('openProp', openProp);
49+
useControlledOpenProvenance(store, openProp);
7350
store.useControlledProp('triggerIdProp', triggerIdProp);
7451

7552
const open = store.useState('open');
@@ -83,7 +60,7 @@ const PopoverRootComponent = fastComponent(function PopoverRootComponent<Payload
8360
useImplicitActiveTrigger(store);
8461
const { forceUnmount } = useOpenStateTransitions(open, store, () => {
8562
// Physical unmount ends the session, so the latched classification goes with it.
86-
store.update({ stickIfOpen: true, openChangeReason: null, openReason: null });
63+
store.update({ stickIfOpen: true, openReason: null });
8764
});
8865

8966
store.useSyncedValues({

0 commit comments

Comments
 (0)