Menu: stabilize popover animation against Ariakit's shared animated flag#78452
Closed
arthur791004 wants to merge 2 commits into
Closed
Menu: stabilize popover animation against Ariakit's shared animated flag#78452arthur791004 wants to merge 2 commits into
animated flag#78452arthur791004 wants to merge 2 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
arthur791004
force-pushed
the
fix/menu-popover-ancestor-rerender
branch
from
b1e9ff3 to
319bcc0
Compare
|
Size Change: +56 B (0%) Total Size: 7.97 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 6a1ec36. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/26147493699
|
arthur791004
force-pushed
the
fix/menu-popover-ancestor-rerender
branch
2 times, most recently
from
ec5aa9e to
fd80646
Compare
arthur791004
changed the title
animated flag
arthur791004
force-pushed
the
fix/menu-popover-ancestor-rerender
branch
2 times, most recently
from
2c34da5 to
64e9508
Compare
… flag After the 33.0.0 `Menu.Popover` refactor (#77460), Ariakit's props (ref, `data-enter`) are spread on the inner `MenuSurface` while the real opacity/transform motion lives on the `MenuMotionRoot` wrapper. The surface therefore has no transition of its own and reports `getComputedStyle( … ).transitionDuration === "0s"`. Ariakit's `useDisclosureContent` reads that `0s` and responds with `store.setState( "animated", false )`. A modal menu shares a single Ariakit store between its dialog backdrop and the popover surface, so that flag flip can starve the surface's own enter transition: it never receives `data-enter`, the `MenuMotionRoot` `:has( … [data-enter] )` opacity override never matches, and the popover renders stuck at `opacity: 0` while still in the DOM and keyboard-reachable. This only surfaces under React 17 update timing (no automatic batching), so it does not affect the Gutenberg plugin itself but does affect host applications still rendering these components under React 17. Give `MenuSurface` a no-op `transition: opacity 1ms` so it reports a non-zero duration and stays out of Ariakit's `animated`-disable path. The real fade/slide motion still comes from `MenuMotionRoot`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arthur791004
force-pushed
the
fix/menu-popover-ancestor-rerender
branch
from
64e9508 to
049be52
Compare
Adds a `Popover Visible In Legacy Root` story that deterministically reproduces `Menu.Popover` rendering stuck at `opacity: 0`. The bug needs React 17 update timing (no automatic batching of state updates dispatched from `requestAnimationFrame` callbacks). React 18's automatic batching masks it, so the story mounts the menu in a legacy `ReactDOM.render` root to restore that timing. The `play` function opens the menu and asserts the popover actually becomes visible (`data-enter` stamped, `MenuMotionRoot` at `opacity: 1`). Without the `MenuSurface` fix the story fails — the popover is in the DOM and focused but never painted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arthur791004
force-pushed
the
fix/menu-popover-ancestor-rerender
branch
from
049be52 to
6a1ec36
Compare
arthur791004
added
the
[Type] Bug
Contributor
|
Hey @arthur791004 , given that the issue is only reproducible with React 17, my preference would be that the issue gets fixed in the consumer project by updating to a more recent version of React. Gutenberg is currently on React 18 and will very soon update to React 19; it would be quite difficult for us to guarantee that our APIs work as expected in earlier (5 year old) versions. |
Contributor
Author
|
Agreed. Closing this issue. Any consumer encountered this issue can follow the workaround on this PR to fix it 🙂 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Menu.Popovercan open fully invisible — theMenuMotionRootwrapper stays atopacity: 0. The popover is in the DOM and focused, just never painted. Also affectsDropdownMenu, the DataViews "Add filter" button, and other consumers.Why it happens
Menu.Popoverputs Ariakit's props (ref,data-enter) on the innerMenuSurface, but the opacity/transform motion lives on the outerMenuMotionRoot. SoMenuSurfacehas no transition of its own and reportstransitionDuration: 0s— and Ariakit reacts to that0sby disabling its sharedanimatedflag.flowchart TD A[Modal menu opens] --> B[Dialog backdrop and popover surface<br/>share one Ariakit animated flag] B --> C[Both schedule the enter transition<br/>via a double requestAnimationFrame] C --> D[Backdrop rAF fires first<br/>transitionDuration is 0s<br/>so animated becomes false] D --> E{How is that<br/>update flushed} E -->|React 18 batched| F[Surface rAF still fires<br/>and stamps data-enter] E -->|React 17 synchronous| G[Surface pending rAF is<br/>cancelled before it fires] F --> H[Popover visible] G --> I[data-enter never set<br/>popover stuck at opacity 0]React 18 batches the update, so the bug stays hidden in the Gutenberg plugin — it only surfaces in host apps still running React 17.
The fix
A no-op
transition: opacity 1msonMenuSurface, so itstransitionDurationis no longer0sand Ariakit keepsanimatedalive. Imperceptible, no DOM/ARIA change — the real fade/slide motion still comes fromMenuMotionRoot.Testing
New Storybook story
Menu → Popover Visible In Legacy Rootmounts the menu in a legacyReactDOM.renderroot (React 17 timing) and asserts the popover becomes visible. It passes with this fix and fails (opacity: 0) without it.Menu / DefaultandMenu / Inside Modalare unchanged.🤖 Generated with Claude Code