Skip to content

Commit 21f35a9

Browse files
sampottscursoragent
andcommitted
fix(core): support multiple submenu viewport bindings and root restore
Settings menus register two submenu panels; bindChild previously replaced the active subscription so viewport transitions never ran for the first panel. Track bindings per view, restore the root panel when returning from a submenu, and animate initial open height via syncRoot. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent bcbf6f2 commit 21f35a9

3 files changed

Lines changed: 177 additions & 17 deletions

File tree

packages/core/src/dom/ui/menu/create-menu.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,8 @@ export function createMenu(options: MenuOptions): MenuApi {
286286
openRafId = 0;
287287
// Root view children may connect after the menu host (HTML custom elements).
288288
// Re-measure on open so viewport CSS variables reflect the mounted panel.
289-
viewport?.syncRoot(getNavigationInput().current.stack.length > 0);
289+
const hasActiveSubmenu = getNavigationInput().current.stack.length > 0;
290+
viewport?.syncRoot(hasActiveSubmenu, { animate: !hasActiveSubmenu });
290291
// Guard against close() being called before the RAF fires — active
291292
// stays true during the closing animation, so also check status.
292293
if (!popover.input.current.active || popover.input.current.status === 'ending' || highlightedItem) return;

packages/core/src/dom/ui/menu/menu-viewport.ts

Lines changed: 54 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,17 @@ export interface MenuViewportAttrs {
3131
'data-menu-viewport': '';
3232
}
3333

34+
export interface MenuViewportSyncRootOptions {
35+
/** Animate viewport width/height when establishing the root panel size (e.g. first open). */
36+
animate?: boolean;
37+
}
38+
3439
export interface MenuViewportApi {
3540
/**
3641
* When no submenu is on the navigation stack, remeasure and size for the root panel.
3742
* With an active submenu, only preserves `data-transitioning` during in-flight resizes.
3843
*/
39-
syncRoot(hasActiveSubmenu: boolean): void;
44+
syncRoot(hasActiveSubmenu: boolean, options?: MenuViewportSyncRootOptions): void;
4045
/** Subscribe to a submenu view transition and drive viewport/root coordination. */
4146
bindChild(view: HTMLElement, transition: MenuViewTransitionApi): () => void;
4247
destroy(): void;
@@ -63,7 +68,7 @@ interface MenuViewportState {
6368
navigation: MenuViewportNavigationContext | null;
6469
viewportTransitionId: number;
6570
viewportTransitioning: boolean;
66-
childUnsubscribe: (() => void) | null;
71+
childUnsubscribes: Map<HTMLElement, () => void>;
6772
rootAttrsUnsubscribe: (() => void) | null;
6873
cancelViewportSettle: (() => void) | null;
6974
childPhaseKeys: WeakMap<HTMLElement, string>;
@@ -375,7 +380,11 @@ function restoreRootPanelToResting(state: MenuViewportState): void {
375380
setViewportSize(state.content, size);
376381
}
377382

378-
function syncRoot(state: MenuViewportState, hasActiveSubmenu: boolean): void {
383+
function syncRoot(
384+
state: MenuViewportState,
385+
hasActiveSubmenu: boolean,
386+
options: MenuViewportSyncRootOptions = {}
387+
): void {
379388
const { content } = state;
380389

381390
if (hasActiveSubmenu) {
@@ -394,7 +403,32 @@ function syncRoot(state: MenuViewportState, hasActiveSubmenu: boolean): void {
394403
// to root size before the back animation finishes.
395404
if (!rootView || getActiveMenuViewElement(viewport)) return;
396405

406+
connectRootView(state, rootView);
407+
408+
const rootPhase = state.rootTransition.input.current.phase;
409+
410+
if (rootPhase !== 'active' && rootPhase !== 'entering') {
411+
state.rootTransition.sync({
412+
active: true,
413+
direction: state.navigation?.direction() ?? 'back',
414+
triggerId: null,
415+
});
416+
}
417+
397418
const size = measureRootMenuView(state, rootView, resolveMeasureMinWidth(content));
419+
const current = getCurrentViewportSize(content);
420+
421+
if (options.animate && (!current || !isContentSizeValid(current))) {
422+
const fromSize: ContentSize =
423+
current && current.width > 0 ? { width: current.width, height: 0 } : { width: size.width, height: 0 };
424+
const viewportTransitionId = startViewportTransition(state);
425+
426+
setViewportSize(content, fromSize);
427+
forceLayout(content);
428+
setViewportSize(content, size);
429+
scheduleViewportTransitionAttrsClear(state, viewportTransitionId);
430+
return;
431+
}
398432

399433
clearViewportTransition(state);
400434
setViewportSize(content, size);
@@ -423,7 +457,7 @@ export function createMenuViewport(
423457
navigation: options.navigation ?? null,
424458
viewportTransitionId: 0,
425459
viewportTransitioning: false,
426-
childUnsubscribe: null,
460+
childUnsubscribes: new Map(),
427461
rootAttrsUnsubscribe: null,
428462
cancelViewportSettle: null,
429463
childPhaseKeys: new WeakMap(),
@@ -436,13 +470,11 @@ export function createMenuViewport(
436470
}
437471

438472
return {
439-
syncRoot(hasActiveSubmenu) {
440-
syncRoot(state, hasActiveSubmenu);
473+
syncRoot(hasActiveSubmenu, options) {
474+
syncRoot(state, hasActiveSubmenu, options);
441475
},
442476

443477
bindChild(view, transition) {
444-
state.childUnsubscribe?.();
445-
446478
const syncChild = (): void => {
447479
const viewState = transition.input.current;
448480
const phaseKey = `${viewState.phase}:${viewState.direction}`;
@@ -453,23 +485,31 @@ export function createMenuViewport(
453485
syncChildViewPhase(state, view, viewState);
454486
};
455487

488+
state.childUnsubscribes.get(view)?.();
489+
456490
const unsubscribe = transition.input.subscribe(syncChild);
457-
state.childUnsubscribe = unsubscribe;
491+
state.childUnsubscribes.set(view, unsubscribe);
458492
syncChild();
459493

460494
return () => {
495+
const existing = state.childUnsubscribes.get(view);
496+
497+
if (existing !== unsubscribe) return;
498+
461499
unsubscribe();
462-
if (state.childUnsubscribe === unsubscribe) {
463-
state.childUnsubscribe = null;
464-
}
500+
state.childUnsubscribes.delete(view);
465501
state.childPhaseKeys.delete(view);
466502
};
467503
},
468504

469505
destroy() {
470506
state.viewportTransitionId += 1;
471-
state.childUnsubscribe?.();
472-
state.childUnsubscribe = null;
507+
508+
for (const unsubscribe of state.childUnsubscribes.values()) {
509+
unsubscribe();
510+
}
511+
512+
state.childUnsubscribes.clear();
473513
state.rootAttrsUnsubscribe?.();
474514
state.rootAttrsUnsubscribe = null;
475515
state.cancelViewportSettle?.();

packages/core/src/dom/ui/menu/tests/menu-viewport.test.ts

Lines changed: 121 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ function createMockTransition(state: MenuViewTransitionState): MenuViewTransitio
4444

4545
interface ViewportSyncOptions {
4646
hasActiveSubmenu?: boolean;
47+
animate?: boolean;
4748
view?: HTMLElement | null;
4849
viewState?: MenuViewTransitionState | null;
4950
}
@@ -52,10 +53,10 @@ function syncMenuViewport(content: HTMLElement | null, sync: ViewportSyncOptions
5253
if (!content) return;
5354

5455
const viewport = getViewport(content);
55-
const { hasActiveSubmenu = false, view = null, viewState = null } = sync;
56+
const { hasActiveSubmenu = false, animate, view = null, viewState = null } = sync;
5657

5758
if (!view || !viewState) {
58-
viewport.syncRoot(hasActiveSubmenu);
59+
viewport.syncRoot(hasActiveSubmenu, animate ? { animate } : undefined);
5960
flush();
6061
return;
6162
}
@@ -600,6 +601,64 @@ describe('createMenuViewport', () => {
600601
expect(resetSpy).not.toHaveBeenCalled();
601602
});
602603

604+
it('keeps earlier submenu views subscribed when additional views register', () => {
605+
const content = addElement();
606+
const rootView = document.createElement('div');
607+
const firstView = document.createElement('div');
608+
const secondView = document.createElement('div');
609+
610+
applyAttrs(rootView, getMenuViewAttrs({ root: true }));
611+
firstView.setAttribute('data-menu-view', '');
612+
secondView.setAttribute('data-menu-view', '');
613+
content.append(rootView, firstView, secondView);
614+
615+
mockMenuViewSize(rootView, {
616+
currentWidth: 160,
617+
currentHeight: 100,
618+
naturalWidth: 160,
619+
naturalHeight: 100,
620+
});
621+
mockMenuViewSize(firstView, {
622+
currentWidth: 200,
623+
currentHeight: 120,
624+
naturalWidth: 200,
625+
naturalHeight: 120,
626+
});
627+
mockMenuViewSize(secondView, {
628+
currentWidth: 240,
629+
currentHeight: 140,
630+
naturalWidth: 240,
631+
naturalHeight: 140,
632+
});
633+
634+
const viewport = createMenuViewport(content);
635+
636+
const firstTransition = createMockTransition({
637+
phase: 'hidden',
638+
direction: 'forward',
639+
triggerId: 'trigger-1',
640+
});
641+
const secondTransition = createMockTransition({
642+
phase: 'hidden',
643+
direction: 'forward',
644+
triggerId: 'trigger-2',
645+
});
646+
647+
viewport.bindChild(firstView, firstTransition);
648+
viewport.bindChild(secondView, secondTransition);
649+
secondView.hidden = true;
650+
651+
(firstTransition.input as WritableState<MenuViewTransitionState>).patch({
652+
phase: 'entering',
653+
direction: 'forward',
654+
triggerId: 'trigger-1',
655+
});
656+
flush();
657+
658+
expect(content.style.getPropertyValue('--media-menu-width')).toBe('160px');
659+
expect(content.style.getPropertyValue('--media-menu-height')).toBe('100px');
660+
});
661+
603662
it('bindChild cleanup only unsubscribes the view it bound', () => {
604663
const content = addElement();
605664
const rootView = document.createElement('div');
@@ -937,4 +996,64 @@ describe('createMenuViewport', () => {
937996
expect(content.style.getPropertyValue('--media-menu-width')).toBe('160px');
938997
expect(content.style.getPropertyValue('--media-menu-height')).toBe('');
939998
});
999+
1000+
it('restores the root panel when syncRoot runs after the root was hidden for a submenu', () => {
1001+
const content = addElement();
1002+
const rootView = document.createElement('div');
1003+
const menuView = document.createElement('div');
1004+
1005+
applyAttrs(rootView, getMenuViewAttrs({ root: true }));
1006+
menuView.setAttribute('data-menu-view', '');
1007+
content.append(rootView, menuView);
1008+
1009+
mockMenuViewSize(rootView, {
1010+
currentWidth: 180,
1011+
currentHeight: 74,
1012+
naturalWidth: 180,
1013+
naturalHeight: 74,
1014+
});
1015+
1016+
const rootTransition = createMenuViewTransition({ persistent: true });
1017+
let hasActiveSubmenu = true;
1018+
const viewport = createMenuViewport(content, rootTransition, {
1019+
navigation: {
1020+
hasActiveSubmenu: () => hasActiveSubmenu,
1021+
direction: () => 'back' as const,
1022+
},
1023+
});
1024+
1025+
viewport.syncRoot(true);
1026+
flush();
1027+
1028+
expect(rootView.hasAttribute('data-open')).toBe(false);
1029+
1030+
hasActiveSubmenu = false;
1031+
menuView.hidden = true;
1032+
viewport.syncRoot(false);
1033+
flush();
1034+
1035+
expect(rootView.hasAttribute('data-open')).toBe(true);
1036+
expect(content.style.getPropertyValue('--media-menu-height')).toBe('74px');
1037+
});
1038+
1039+
it('animates the initial root viewport size when animate is requested', () => {
1040+
const content = addElement();
1041+
const rootView = document.createElement('div');
1042+
1043+
applyAttrs(rootView, getMenuViewAttrs({ root: true }));
1044+
content.append(rootView);
1045+
1046+
mockMenuViewSize(rootView, {
1047+
currentWidth: 180,
1048+
currentHeight: 74,
1049+
naturalWidth: 180,
1050+
naturalHeight: 74,
1051+
});
1052+
1053+
syncMenuViewport(content, { animate: true });
1054+
1055+
expect(content.hasAttribute('data-transitioning')).toBe(true);
1056+
expect(content.style.getPropertyValue('--media-menu-width')).toBe('180px');
1057+
expect(content.style.getPropertyValue('--media-menu-height')).toBe('74px');
1058+
});
9401059
});

0 commit comments

Comments
 (0)