[menu] Play the enter transition for an initially open submenu - #4383
Conversation
commit: |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
PR: out-of-date
A submenu with `defaultOpen` appeared fully formed while its parent menu was still animating in, so it looked like a detached panel floating next to an invisible menu. `useTransitionStatus` initialises `mounted` to `open`, which means the `open && !mounted` branch that produces the `'starting'` phase never runs for a popup that mounts already open. No `[data-starting-style]`, no enter transition. That is deliberate for page-load content, but a submenu's subtree only mounts once its parent popup opens, so its appearance is something the user watches happen. `useTransitionStatus` gains an opt-in `animateInitialOpen`, threaded through `useOpenStateTransitions`, that starts `mounted` at `false` so the existing branch runs on the first render. React re-renders before committing, so this costs no extra frame and no synthesized transition status. Only `MenuRoot` passes it, and only for submenus; every other popup keeps today's behaviour. Known gap: with a `keepMounted` parent the submenu subtree already exists at page load, so it still skips the transition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
41cf1aa to
e52263e
Compare
github-actions
Bot
removed
the
PR: out-of-date
michaldudak
changed the title
Bundle size
PerformanceTotal duration: 1,453.74 ms -132.22 ms(-8.3%) | Renders: 76 (+0) | Paint: 2,236.13 ms -200.10 ms(-8.2%)
…and 1 more (+9 within noise) — details Check out the code infra dashboard for more information about this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e52263ecb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review follow-up: - Gate `animateInitialOpen` on the parent popup being mid enter transition (`transitionStatus === 'starting'`) instead of on being a submenu, so a `defaultOpen` submenu inside a menu that is open on the first page render no longer animates at page load. - Seed the submenu store's `instantType` from the parent when it mounts already open, so `[data-instant]` styling suppresses the enter transition on both popups or neither (e.g. keyboard opens). - Harden the test frame sampler: use the managed AnimationFrame scheduler, expose a non-destructive `seen()` accessor so `waitFor` retries observe a live value, and stop trackers in `afterEach` so a failing test cannot leak a per-frame DOM polling loop into later tests. - Add tests for the page-load, keepMounted (pins the known limitation), and instant-parent scenarios. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Gate `animateInitialOpen` on the submenu being open at mount so a closed submenu doesn't seed an `instantType` that a later controlled open would never clear - Write `instantType` in the same store update that mounts the popup subtree so a React 17 legacy-mode sync flush can't mount the submenu in between and read the previous cycle's value - Wrap the negative tests' waits in `act` so pending transition-status updates don't trip `vitest-fail-on-console` - Correct the gating comment: a `keepMounted` parent does pass through `'starting'`; its submenu doesn't animate because these initializers already ran at page load Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1411888bfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ransition
A controlled open={false} must win over defaultOpen when deciding whether an
initially open submenu animates in, so a closed submenu cannot seed an
instantType it would never clear.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 667f0c61ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The seeded value must not outlive the initial reveal: controlled open flips bypass setOpen, so a lingering value would render [data-instant] on every subsequent open and suppress transitions that should play. Cleared once the enter phase settles, following the MenuPositioner trigger-change pattern. Also replaces the raw setTimeout waits in the enter-transition tests with the managed Timeout scheduler, cleared in afterEach, so an interrupted test cannot leak a pending callback into a later one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 171eb1afe9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
With the popup subtree omitted at mount (suspended or waiting on data), useAnimationsFinished returns without invoking its callback and a later ref assignment never reruns the clearing effect, so the seeded instantType would stick and mark a late-rendered popup as instant. Clear immediately in that case: a popup appearing after the reveal settles is page-load-like content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d643428d09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A controlled close during the initial enter aborts the pending animations-finished cleanup, and a closed popup schedules no replacement, so the seeded instantType survived and suppressed the next controlled reopen's transition. Clear the seed as soon as closing starts: the exit's suppression is already decided at its trigger commit, so nothing is left to protect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
A submenu with
defaultOpenappears fully formed while its parent menu is still animating in, so it reads as a detached panel floating next to an invisible menu.Fixes #4224.
Note
This PR was repurposed. It previously deferred the submenu's reveal until the parent's open animation finished, to fix the submenu "teleporting" into place. That symptom no longer reproduces on
master— positioning now tracks a moving anchor to within ~1px (measured in Chromium: median 0.7px, max 1.3px deviation from the submenu trigger across a parent scale animation). What remains of #4224 is the missing enter transition, which is what this PR now fixes. The previous approach has been dropped entirely.Root cause
useTransitionStatusinitialisesmountedtoopen:A popup that mounts already open has
mounted === trueon its first render, so the branch that produces the'starting'phase never runs.[data-starting-style]is never applied and no enter transition plays.That is deliberate for page-load content — a
defaultOpenpopup or SSR'd markup shouldn't animate in on every load. But a submenu's subtree only mounts once its parent popup opens, so its appearance is something the user watches happen, and it should animate like everything else in that popup.Measured on
master(frames carrying[data-starting-style]):defaultOpendefaultOpenSolution
useTransitionStatusgains an opt-inanimateInitialOpenthat startsmountedatfalse, so the existingopen && !mountedbranch runs on the first render. React re-renders before committing, so this costs no extra frame and needs no synthesized transition status — the popup goes through the normal'starting'phase.It is threaded through
useOpenStateTransitions, and onlyMenuRootpasses it — only when the parent menu popup is itself mid enter transition at the submenu's first render (parent.type === 'menu'and the parent store'stransitionStatusis'starting'). Keying off the parent's live'starting'phase rather than off being a submenu means a submenu animates exactly when its mount is part of a reveal the user is watching: a parent popup that was already open on the first page render (defaultOpenat page load,keepMounted) never passes through'starting', so its submenu remains page-load content and doesn't animate. Every other popup, and adefaultOpenroot menu, keeps today's behaviour.An initially open submenu also seeds its store's
instantTypefrom the parent, so when the parent opened instantly (e.g. a keyboard click),[data-instant]is present on both popups and consumer styling suppresses both enter transitions or neither.Not covered
With a
keepMountedparent the submenu's subtree already exists at page load, so it mounts open during the initial render and appears without a transition when the parent later opens. Closing that gap needs a visibility-based signal rather than a mount-based one, which is a larger change to shared transition machinery. A test pins the current behaviour to document the gap.Tests
Six Chromium tests in
MenuPopupEnterTransition.test.tsx, verified to discriminate — onmasterthe first fails, and with the parent-transition gating loosened back to plainparent.type === 'menu'the two page-load tests and the instant test fail:defaultOpenroot menu still does not play itdefaultOpensubmenu inside adefaultOpenroot menu does not play it at page loaddefaultOpensubmenu under akeepMountedparent does not play it (pins the gap above)data-instantfrom an instantly-opened parentThe per-frame
[data-starting-style]sampler uses the managedAnimationFramescheduler, exposes a non-destructiveseen()accessor forwaitForretries, and is stopped inafterEach, so a failing test can't leak a polling loop into later tests.Full suites: Menu 1048 passed (chromium) / 877 passed (jsdom), Popover 473 passed, popupStoreUtils 31 passed.
docs/src/app/(private)/experiments/menu/default-open-submenu.tsxis included as a repro harness, with the transition slowed to 2s so the enter animation is easy to see. Happy to drop it if experiments aren't wanted here.