Skip to content

Commit a98297b

Browse files
committed
[menu] Fix detached-trigger store sync and context-menu navigation
Bugs: MenuStore's parent observer no longer detaches itself on the first parent change (the synchronous observe-fire was clobbering the per-parent subscription); the hover-open mouseup listener is now cleaned up; the touch-to-close guard runs before onOpenChange/dispatchOpenChange so controlled consumers aren't force-closed; ArrowLeft no longer closes a root context menu (it's not treated as nested). Cleanup: remove the never-constructed nested-context-menu parent variant; drop the no-op render-time 'delete rootTriggerProps.id'; use the deduping warn util for the modal-on-child-menu warning (was console.warn every render) with corrected wording; use REASONS.triggerHover instead of the string literal; comment fix for stickIfOpen. Keep the duplicated trigger drag-release bounds check inline for now (shared helper is a follow-up).
1 parent 3c5e55f commit a98297b

6 files changed

Lines changed: 53 additions & 38 deletions

File tree

docs/src/app/(docs)/react/components/menu/types.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1078,7 +1078,6 @@ type MenuParent =
10781078
| { type: 'menu'; store: MenuStore<unknown> }
10791079
| { type: 'menubar'; context: MenubarContext }
10801080
| { type: 'context-menu'; context: ContextMenuRootContext }
1081-
| { type: 'nested-context-menu'; context: ContextMenuRootContext; menuContext: MenuRootContext }
10821081
| { type: undefined };
10831082
```
10841083

packages/react/src/menu/positioner/MenuPositioner.tsx

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -298,11 +298,7 @@ export const MenuPositioner = React.forwardRef(function MenuPositioner(
298298
<MenuPositionerContext.Provider value={positioner}>
299299
{shouldRenderBackdrop && (
300300
<InternalBackdrop
301-
ref={
302-
parent.type === 'context-menu' || parent.type === 'nested-context-menu'
303-
? parent.context.internalBackdropRef
304-
: null
305-
}
301+
ref={parent.type === 'context-menu' ? parent.context.internalBackdropRef : null}
306302
inert={inertValue(!open)}
307303
cutout={backdropCutout}
308304
/>

packages/react/src/menu/root/MenuRoot.tsx

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { useId } from '@base-ui/utils/useId';
66
import { useIsoLayoutEffect } from '@base-ui/utils/useIsoLayoutEffect';
77
import { EMPTY_ARRAY, EMPTY_OBJECT } from '@base-ui/utils/empty';
88
import { fastComponent } from '@base-ui/utils/fastHooks';
9+
import { warn } from '@base-ui/utils/warn';
910
import {
1011
FloatingTree,
1112
useDismiss,
@@ -147,8 +148,10 @@ export const MenuRoot = fastComponent(function MenuRoot<Payload>(props: MenuRoot
147148

148149
if (process.env.NODE_ENV !== 'production') {
149150
if (parent.type !== undefined && modalProp !== undefined) {
150-
console.warn(
151-
'Base UI: The `modal` prop is not supported on nested menus. It will be ignored.',
151+
// `warn` dedupes, so this won't spam on every render. `parent.type !== undefined` also
152+
// covers menubar and context menus, not just submenus.
153+
warn(
154+
'The `modal` prop is not supported on submenus, menubar menus, or context menus. It will be ignored.',
152155
);
153156
}
154157
}
@@ -238,6 +241,22 @@ export const MenuRoot = fastComponent(function MenuRoot<Payload>(props: MenuRoot
238241
return;
239242
}
240243

244+
const nativeEvent = eventDetails.event as Event;
245+
246+
// Prevent the menu from closing on mobile devices that have a delayed click event.
247+
// In some cases the menu, when tapped, will fire the focus event first and then the click
248+
// event. Without this guard, the menu will close immediately after opening.
249+
// This must bail before notifying `onOpenChange` and dispatching the open change, otherwise
250+
// controlled consumers (and floating-ui's own state) would close the menu regardless.
251+
if (
252+
nextOpen === false &&
253+
nativeEvent?.type === 'click' &&
254+
(nativeEvent as PointerEvent).pointerType === 'touch' &&
255+
!allowTouchToCloseRef.current
256+
) {
257+
return;
258+
}
259+
241260
const shouldPreventUnmountOnClose = attachPreventUnmountOnClose(
242261
eventDetails as MenuRoot.ChangeEventDetails,
243262
);
@@ -256,19 +275,8 @@ export const MenuRoot = fastComponent(function MenuRoot<Payload>(props: MenuRoot
256275

257276
store.state.floatingRootContext.dispatchOpenChange(nextOpen, eventDetails);
258277

259-
const nativeEvent = eventDetails.event as Event;
260-
if (
261-
nextOpen === false &&
262-
nativeEvent?.type === 'click' &&
263-
(nativeEvent as PointerEvent).pointerType === 'touch' &&
264-
!allowTouchToCloseRef.current
265-
) {
266-
return;
267-
}
268-
269-
// Prevent the menu from closing on mobile devices that have a delayed click event.
270-
// In some cases the menu, when tapped, will fire the focus event first and then the click event.
271-
// Without this guard, the menu will close immediately after opening.
278+
// Reset the touch-to-close guard so a delayed click after a focus-driven open is ignored
279+
// (see the early return above).
272280
if (nextOpen && reason === REASONS.triggerFocus) {
273281
allowTouchToCloseRef.current = false;
274282
allowTouchToCloseTimeout.start(300, () => {
@@ -394,7 +402,10 @@ export const MenuRoot = fastComponent(function MenuRoot<Payload>(props: MenuRoot
394402
enabled: !disabled,
395403
listRef: store.context.itemDomElements,
396404
activeIndex,
397-
nested: parent.type !== undefined,
405+
// A root context menu has no parent menu to return to, so it must not be treated as nested:
406+
// otherwise the cross-orientation close key (e.g. ArrowLeft) would close it, which native
407+
// context menus never do. Its submenus are regular menus with `parent.type === 'menu'`.
408+
nested: parent.type !== undefined && parent.type !== 'context-menu',
398409
loopFocus,
399410
orientation,
400411
parentOrientation: parent.type === 'menubar' ? parent.context.orientation : undefined,
@@ -666,11 +677,6 @@ export type MenuParent =
666677
type: 'context-menu';
667678
context: ContextMenuRootContext;
668679
}
669-
| {
670-
type: 'nested-context-menu';
671-
context: ContextMenuRootContext;
672-
menuContext: MenuRootContext;
673-
}
674680
| {
675681
type: undefined;
676682
};

packages/react/src/menu/store/MenuStore.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,16 +129,24 @@ export class MenuStore<Payload> extends ReactStore<
129129
selectors,
130130
);
131131

132+
// This menu's own mouse-up gate, used when it has no parent to borrow from.
133+
const ownAllowMouseUpTriggerRef = this.context.allowMouseUpTriggerRef;
134+
132135
// Set up propagation of state from parent menu if applicable.
133-
this.unsubscribeParentListener = this.observe('parent', (parent) => {
134-
this.unsubscribeParentListener?.();
136+
// `observe` fires the listener synchronously here and again on every `parent` change, so the
137+
// observer's own unsubscriber and the per-parent store subscription must live in separate
138+
// fields — otherwise the synchronous first call clobbers one with the other and the first
139+
// parent change tears down the observer itself.
140+
this.observe('parent', (parent) => {
141+
this.unsubscribeParentStore?.();
142+
this.unsubscribeParentStore = null;
135143

136144
if (parent.type === 'menu') {
137145
let rootId = parent.store.select('rootId');
138146
let floatingTreeRoot = parent.store.select('floatingTreeRoot');
139147
let keyboardEventRelay = parent.store.select('keyboardEventRelay');
140148

141-
this.unsubscribeParentListener = parent.store.subscribe(() => {
149+
this.unsubscribeParentStore = parent.store.subscribe(() => {
142150
const nextRootId = parent.store.select('rootId');
143151
const nextFloatingTreeRoot = parent.store.select('floatingTreeRoot');
144152
const nextKeyboardEventRelay = parent.store.select('keyboardEventRelay');
@@ -163,9 +171,11 @@ export class MenuStore<Payload> extends ReactStore<
163171

164172
if (parent.type !== undefined) {
165173
this.context.allowMouseUpTriggerRef = parent.context.allowMouseUpTriggerRef;
174+
return;
166175
}
167176

168-
this.unsubscribeParentListener = null;
177+
// Back at the root: stop borrowing a parent's ref so mouse-up state stays isolated.
178+
this.context.allowMouseUpTriggerRef = ownAllowMouseUpTriggerRef;
169179
});
170180
}
171181

@@ -185,7 +195,7 @@ export class MenuStore<Payload> extends ReactStore<
185195
return externalStore ?? internalStore;
186196
}
187197

188-
private unsubscribeParentListener: (() => void) | null = null;
198+
private unsubscribeParentStore: (() => void) | null = null;
189199
}
190200

191201
function createInitialState<Payload>(): State<Payload> {

packages/react/src/menu/submenu-trigger/MenuSubmenuTrigger.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,6 @@ export const MenuSubmenuTrigger = React.forwardRef(function MenuSubmenuTrigger(
157157
const localInteractionProps = click.reference ?? EMPTY_OBJECT;
158158

159159
const rootTriggerProps = store.useState('triggerProps', true);
160-
delete rootTriggerProps.id;
161160

162161
const state: MenuSubmenuTriggerState = { disabled, highlighted, open };
163162

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use client';
22
import * as React from 'react';
33
import { useTimeout } from '@base-ui/utils/useTimeout';
4+
import { addEventListener } from '@base-ui/utils/addEventListener';
45
import { ownerDocument } from '@base-ui/utils/owner';
56
import { fastComponentRef } from '@base-ui/utils/fastHooks';
67
import { useStableCallback } from '@base-ui/utils/useStableCallback';
@@ -138,8 +139,7 @@ export const MenuTrigger = fastComponentRef(function MenuTrigger(
138139

139140
if (
140141
contains(triggerRef.current, mouseUpTarget) ||
141-
contains(store.select('positionerElement'), mouseUpTarget) ||
142-
mouseUpTarget === triggerRef.current
142+
contains(store.select('positionerElement'), mouseUpTarget)
143143
) {
144144
return;
145145
}
@@ -165,8 +165,12 @@ export const MenuTrigger = fastComponentRef(function MenuTrigger(
165165
React.useEffect(() => {
166166
if (isOpenedByThisTrigger && store.select('lastOpenChangeReason') === REASONS.triggerHover) {
167167
const doc = ownerDocument(triggerRef.current);
168-
doc.addEventListener('mouseup', handleDocumentMouseUp, { once: true });
168+
// A hover-open can close again (hover out) without ever seeing a mouseup, leaving the
169+
// `{ once: true }` listener armed to fire on an unrelated later interaction. Returning the
170+
// cleanup removes it.
171+
return addEventListener(doc, 'mouseup', handleDocumentMouseUp, { once: true });
169172
}
173+
return undefined;
170174
}, [isOpenedByThisTrigger, handleDocumentMouseUp, store]);
171175

172176
const parentMenubarHasSubmenuOpen = isInMenubar && parent.context.hasSubmenuOpen;
@@ -189,7 +193,8 @@ export const MenuTrigger = fastComponentRef(function MenuTrigger(
189193
isClosing: () => store.select('transitionStatus') === 'ending',
190194
});
191195

192-
// Whether to ignore clicks to open the menu.
196+
// Whether to keep the menu open when the trigger is clicked shortly after a hover-open
197+
// (i.e. ignore the click that would otherwise close it).
193198
// `lastOpenChangeReason` doesn't need to be reactive here, as we need to run this
194199
// only when `isOpenedByThisTrigger` changes.
195200
const stickIfOpen = useStickIfOpen(isOpenedByThisTrigger, store.select('lastOpenChangeReason'));
@@ -369,7 +374,7 @@ function useStickIfOpen(open: boolean, openReason: string | null) {
369374
const stickIfOpenTimeout = useTimeout();
370375
const [stickIfOpen, setStickIfOpen] = React.useState(false);
371376
useIsoLayoutEffect(() => {
372-
if (open && openReason === 'trigger-hover') {
377+
if (open && openReason === REASONS.triggerHover) {
373378
// Only allow "patient" clicks to close the menu if it's open.
374379
// If they clicked within 500ms of the menu opening, keep it open.
375380
setStickIfOpen(true);

0 commit comments

Comments
 (0)