Skip to content

[popups] Avoid re-rendering inactive triggers on open and close - #5865

Merged
atomiks merged 4 commits into
mui:masterfrom
atomiks:claude/charming-mendeleev-a942e9
Oct 7, 2026
Merged

atomiks merged 4 commits into
mui:masterfrom
atomiks:claude/charming-mendeleev-a942e9

Conversation

@atomiks

@atomiks atomiks commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #5895

With a shared handle, every inactive Popover, Tooltip, PreviewCard, and Dialog trigger re-rendered on each open and close. This PR mounts a handle-backed Popover, PreviewCard, or Dialog Root's interactions eagerly so trigger props stay stable, has inactive Tooltip triggers stop subscribing to root props they discard, scopes Popover trigger subscriptions per trigger, and resets the state the retained interactions keep between open sessions (including shared useDismiss state).

Performance

Main-thread time in ms with 1,000 triggers, open / close (production build, headless Chromium, M4 Pro; median of 3 interleaved page loads × 10 cycles):

Detached triggers (shared handle) Contained triggers (no handle) Mount: 1,000 Roots, each with its own handle
Popover 32 / 26 → 12 / 11 32 / 31 → 30 / 29 97 → 98
Tooltip 42 / 33 → 13 / 11 39 / 35 → 10 / 9 97 → 102
PreviewCard 23 / 21 → 8 / 6 22 / 21 → 19 / 20 95 → 103
Dialog 22 / 20 → 9 / 8 22 / 20 → 25 / 20 82 → 96

Tooltip gets the contained-trigger win too, since its fix is in the trigger rather than the Root. The trade-off is the last column: a Popover, PreviewCard, or Dialog Root with a handle now mounts its interactions up front, about 10 µs per Root when every row gets its own handle (these rows vary by about ±10 ms between runs; Tooltip no longer mounts anything extra). A shared handle, the case this targets, mounts no slower.

@atomiks atomiks added type: bug It doesn't behave as expected. performance scope: all components Widespread work has an impact on almost all components. labels Oct 2, 2026
@pkg-pr-new

pkg-pr-new Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
  • base-ui-tanstack-start

  • vite-css-base-ui-example

    pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@5865
    
    pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@5865
    

commit: 700a73d

@code-infra-dashboard

code-infra-dashboard Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+422B(+0.08%) 🔺+114B(+0.07%)

Details of bundle changes

Performance

Total duration: 515.75 ms ▼-424.33 ms(-45.1%) | Renders: 76 (+0) | Paint: 915.04 ms ▼-718.56 ms(-44.0%)

Test Duration Renders
Select mount (200 instances) 53.41 ms ▼-56.95 ms(-51.6%) 3 (+0)
Slider mount (300 instances) 45.38 ms ▼-52.28 ms(-53.5%) 2 (+0)
Mixed surface mount (app-like density) 35.07 ms ▼-46.56 ms(-57.0%) 5 (+0)
Scroll Area mount (300 instances) 32.89 ms ▼-45.37 ms(-58.0%) 3 (+0)
Checkbox mount (500 instances) 30.36 ms ▼-41.07 ms(-57.5%) 1 (+0)

…and 7 more (+3 within noise) — details


Check out the code infra dashboard for more information about this PR.

@netlify

netlify Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 700a73d
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6ac5f7dbda3ba00008eef909
😎 Deploy Preview https://deploy-preview-5865--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@atomiks
atomiks marked this pull request as ready for review October 2, 2026 05:25
@atomiks
atomiks force-pushed the claude/charming-mendeleev-a942e9 branch 2 times, most recently from bb8bb4f to cf4be11 Compare October 5, 2026 07:49
@michaldudak

Copy link
Copy Markdown
Member

PR review

Nothing blocks merge. On every component I traced, inactive handle-backed triggers stop re-rendering. The three new PopoverTrigger selectors give the same results as the logic they replace for every input, including triggers without an id. Removing onNestedDialogOpen(0, 0) is correct: the cleanup still sends (0, 0) on every true→false change.

The one real regression is in Tooltip. Its interactions now stay mounted, and that leaves a stale cursor-point reference behind between open sessions. A smaller change to the Tooltip trigger would avoid both the bug and the retained state. I traced everything by reading the code; I did not run any tests.

Bugs (1)

1. 🟠 Handle-backed Tooltip keeps a stale positionReference between sessions

Location: packages/react/src/tooltip/root/TooltipRoot.tsx:140 and packages/react/src/floating-ui-react/hooks/useClientPoint.ts:168

const shouldRenderInteractions =
  open || mounted || (!disabled && (handle != null || trackCursorAxis !== 'none'));
// useClientPoint
if (!enabled) {
  resetReference(domReference);
  return undefined;
}

TooltipInteractions now stays mounted while closed, even when trackCursorAxis="none", so useClientPoint stays mounted with enabled: false. After a close finishes:

  • activeTriggerElement resolves to null once mounted is false. The sync step in useSyncedFloatingRootContext therefore sets both referenceElement and positionReference to null.
  • domReferenceElement is only written when the value is an element, so it still points at trigger A.
  • The positioner unmounts, which changes floating. The useClientPoint effect runs again and calls resetReference(A).
  • The result is positionReference = A while referenceElement = null. From then on the positionReference === referenceElement check in the sync never passes.

On the next open from trigger B, the referenceElement selector (positionReference ?? referenceElement) still returns A. The first layout-phase position is computed against A, which may even be detached by then. It only corrects itself when the useClientPoint passive effect sees the new domReference.

Before this PR, the interactions unmounted after close, and their cleanup (store.set('positionReference', null)) cleared the value.

Failure scenario: a toolbar where several detached Tooltip.Triggers share one handle. Open on trigger 1 and let it close. Then open trigger 2 from a non-sync update, for example handle.open('trigger-2') or a controlled open/triggerId change from a timer or transition. The passive effects run after paint, so the tooltip can paint for one frame at trigger 1's position, or at 0,0 if trigger 1 was removed from the DOM. Hover and focus opens flush synchronously, so they probably hide it.

Fix: any of these works:

  • In the !enabled branch, call resetReference(null); the selector already falls back to referenceElement.
  • Only call resetReference(domReference) while open.
  • Drop Tooltip's eager mount entirely (Simplification 1), which also removes this state.

Tests (3)

1. 🟡 Tooltip and PreviewCard re-render tests only open and close imperatively

Location: packages/react/src/tooltip/root/TooltipRoot.detached-triggers.test.tsx:178, packages/react/src/preview-card/root/PreviewCardRoot.detached-triggers.test.tsx:30

Both tests use handle.open('trigger-1') / handle.close() only. Hover, focus, delay groups and close on mouseleave write extra store state (openChangeReason, lastOpenChangeReason, instantType, openMethod) that these tests never exercise.

Failure scenario: a later change makes inactive triggers subscribe to a value written only on the hover path. The tests still pass.

Fix: add a hover-based open/close variant using delay={0} and fake timers, as the Popover hover test does, plus a focus/blur variant.

2. 🟡 Re-render tests never switch triggers and have no positive control

Location: packages/react/src/popover/root/PopoverRoot.detached-triggers.test.tsx, plus the Dialog, Tooltip and PreviewCard equivalents

Every test renders two triggers, only ever opens trigger 1, and asserts the other's render count is 0. Two cases go untested:

  • Switching to trigger 2 while open, which writes activeTriggerId, openChangeReason and openMethod: exactly the state the new selectors scope.
  • A check that the active trigger still updates. If a regression stopped every trigger from re-rendering, aria-expanded and data-popup-open would go stale and these tests would still pass.

Fix:

  • Add a third "bystander" trigger and assert it renders 0 times across a trigger-1 → trigger-2 switch.
  • Assert trigger 1's aria-expanded toggles inside openAndClose.

3. 🟡 AlertDialog and Drawer get the eager mount and the (0, 0) removal with no tests

Location: packages/react/src/dialog/root/useRenderDialogRoot.tsx:92, packages/react/src/dialog/root/useDialogRoot.ts:96

useRenderDialogRoot is shared by AlertDialogRoot and DrawerRoot, but only Dialog is tested. The removed branch also zeroed drawerCount, and nested-drawer state comes from that count.

Failure scenario: a closed, handle-backed nested Drawer next to an open nested Drawer. If the parent's nested-drawer state were cleared, nothing would catch it.

Fix:

  • Add a Drawer version of "keeps the parent nested state when a closed handle-backed nested dialog mounts" that asserts the parent's nested-drawer attribute.
  • Add an AlertDialog re-render-count test.

Simplifications (4)

1. 🟡 Tooltip can skip the eager mount by not subscribing to props it throws away

Location: packages/react/src/tooltip/trigger/TooltipTrigger.tsx:216

const rootTriggerProps = store.useState('triggerProps', isMountedByThisTrigger);
const shouldApplyRootTriggerProps = isMountedByThisTrigger || trackCursorAxis !== 'none';

Inactive Tooltip triggers already discard the root props, yet they still subscribe to triggerProps(false). That subscription is the only reason they re-rendered when inactiveTriggerProps flipped between EMPTY_OBJECT and dismiss.reference.

Failure scenario: today each closed, handle-backed Tooltip Root keeps a full TooltipInteractions mounted (useDismiss plus useClientPoint). That retained state caused Bug 1, and it is also why the useClientPoint !open reset and the Tooltip composition test were needed.

Fix:

  • Drop handle != null from TooltipRoot.
  • Have TooltipTrigger select a constant when it won't apply the props, for example (state, apply) => apply ? state.activeTriggerProps : undefined called with shouldApplyRootTriggerProps. Tooltip's active and inactive props are the same object.
  • This also stops re-renders for inactive Tooltip triggers inside a handle-less Root.

2. 🟡 stickIfOpen no longer needs to be reactive store state

Location: packages/react/src/popover/store/PopoverStore.ts:163

this.set('stickIfOpen', true);
this.context.stickIfOpenTimeout.start(PATIENT_CLICK_THRESHOLD, () => {
  this.set('stickIfOpen', false);
});

No selector reads it any more; only the getter reads store.state.stickIfOpen when a press is handled.

Failure scenario: each hover-open still makes two store.set calls. Each one notifies every subscriber, which is about 9 selectors per Popover trigger × N triggers, all for a value that nothing renders. That's the per-trigger work this PR is trying to remove.

Fix: move it to context as a plain mutable field next to stickIfOpenTimeout, and keep the reset to true on unmount, which the "prop-only reopen" test relies on.

3. ℹ️ cancelDismissOnEndTimeout.clear() in the close reset does nothing observable

Location: packages/react/src/floating-ui-react/hooks/useDismiss.ts:304

The timeout's callback does nothing once touchStateRef.current is null, which the line above sets. Every touchstart also restarts the timeout. No test can pin this line. Drop it, or keep it as explicit cleanup.

4. ℹ️ The compositionstart listener is now largely redundant

Location: packages/react/src/floating-ui-react/hooks/useDismiss.ts:316

Escape is now also checked against native.isComposing, and handleCompositionEnd now sets the ref itself. So the ref only has to cover Safari's keydown after compositionend. Removing handleCompositionStart and its document listener saves about 70 B and one listener per open popup. The existing "IME is active" test would then need isComposing: true on its keyDown, which matches what browsers actually send. Optional.

Verdict

Approve after nits - no blocking issues; the 🟠 stale Tooltip positionReference is worth fixing first, ideally by dropping Tooltip's eager mount (Simplification 1).


🤖 Review generated with Claude Code

@michaldudak

Copy link
Copy Markdown
Member

PR review

Nothing blocks merge. The trigger re-render fix checks out. The new Popover and Tooltip selectors behave exactly like the expressions they replace, and the deleted onNestedDialogOpen(0, 0) call is safe to remove. One medium regression comes from the eager mounting: a closed Dialog that has a handle now records nested-dialog counts. The other findings are small test gaps and one optional simplification. Everything was checked by reading the code; I didn't run any tests.

Bugs (2)

1. 🟠 A closed dialog with a handle picks up nested-dialog counts and opens as "not topmost"

Location: packages/react/src/dialog/root/useRenderDialogRoot.tsx:92, packages/react/src/dialog/root/useDialogRoot.ts:90

const shouldRenderInteractions = open || mounted || handle != null;
// …
store.useContextCallback('onNestedDialogOpen', (dialogCount, drawerCount) => {
  setOwnNestedOpenDialogs(dialogCount);
  setOwnNestedOpenDrawers(drawerCount);
});

Before this PR, a closed parent Dialog had no DialogInteractions mounted. Any onNestedDialogOpen call it received while closed went to an unmounted component and was lost, and every open session started with the counts at 0. With a handle, DialogInteractions now stays mounted while closed. So a child Root placed inside the parent Root but outside its Popup sets the parent's count while the parent is closed, and that count carries into the parent's next open. The parent then opens with isTopmost === false. This affects Dialog, AlertDialog and Drawer.

Failure scenario: The parent is <Dialog.Root handle={help}> wrapping a page section. Inside that section is a separate <Dialog.Root> (C), and C's popup contains a <Dialog.Trigger handle={help}>.

  1. The user opens C, then clicks the help trigger. The parent opens on top of C.
  2. Escape now closes C, the dialog underneath, and leaves the help dialog open.
  3. Clicking the help dialog's backdrop does nothing.
  4. The help dialog shows data-nested-dialog-open and --nested-dialogs: 1. On a Drawer, the Android CloseWatcher is skipped.

The same thing happens when the parent closes while C stays open and the parent is reopened later.

Fix:

  • In the onNestedDialogOpen callback, ignore non-zero counts while !store.select('open'), but still accept the zeroed (0, 0) close notification.
  • Reset both counts in a layout effect when open becomes false.
  • Add a regression test for the setup above.

2. ℹ️ isComposingRef can stay true if the listener effect re-runs within 0–5 ms of compositionend

Location: packages/react/src/floating-ui-react/hooks/useDismiss.ts:325

isComposingRef.current = true;
compositionTimeout.start(platform.engine.webkit ? 5 : 0, () => { … });

compositionTimeout is created on each effect run, and the cleanup clears it without resetting the ref. If a dependency changes inside that window while the popup is open (for example, Dialog's escapeKey: isTopmost flipping), Escape stops working for the rest of that open session. A composition that starts while the popup is open already hit this before the PR; the new line adds the case where the composition started before opening. The window is tiny, so this is only a note. The fix would be to reset the ref in the cleanup when the timeout is still pending.

Tests (3)

1. 🟡 The prop-only reopen test doesn't check the stickIfOpen reset

Location: packages/react/src/popover/root/PopoverRoot.detached-triggers.test.tsx:391

The test uses fake timers but never advances the clock. store.context.stickIfOpen therefore stays true from the hover-open throughout. Deleting store.context.stickIfOpen = true from PopoverRoot's useOpenStateTransitions callback would most likely still pass.

Fix: Add clock.tick(PATIENT_CLICK_THRESHOLD) after the hover-open and before setOpen(false). The click after the reopen then only keeps the popup open if the close-time reset ran.

2. 🟡 No handle-backed Dialog nesting cycle or Drawer render-count test

Location: packages/react/src/dialog/root/DialogRoot.detached-triggers.test.tsx, packages/react/src/drawer/popup/DrawerPopup.test.tsx:449

Once a handle-backed DialogInteractions stays mounted, dropping the parent's count depends only on the effect cleanup. No test opens, closes and reopens a handle-backed child dialog and checks the parent's data-nested-dialog-open and --nested-dialogs each time. Drawer goes through the same eager mount, but has no "does not re-render inactive detached triggers" test like the AlertDialog one.

The two new tests for the removed else-branch also depend on JSX order. They only fail if the closed handle-backed Root comes after the open sibling, so its layout effect runs last. A one-line comment would protect that ordering.

3. 🟡 Per-trigger ownership of data-pressed and focus guards isn't checked

Location: packages/react/src/popover/root/PopoverRoot.detached-triggers.test.tsx:310, around :1961

The test that moves the popup to another trigger only checks aria-expanded. Nothing checks that data-pressed (now from isPressOpenedByTrigger) or the focus guards (now from hasTriggerFocusGuards) leave trigger 1 and move to trigger 2 when it takes over. Adding not.toHaveAttribute assertions on trigger 1 after "Open Trigger 2" would cover it.

Simplifications (1)

1. ℹ️ compositionstart tracking is now redundant with native.isComposing

Location: packages/react/src/floating-ui-react/hooks/useDismiss.ts:208-210, :721

Every supported browser sets isComposing: true on keydowns during a composition, so the ref only needs to cover Safari's Escape keydown that arrives after compositionend. Since compositionend now sets the ref itself, the compositionstart listener and handleCompositionStart could go. That saves one document listener per open popup and a few bytes. The existing test "does not dismiss with escape key if IME is active" would then need isComposing: true on its keydown, which is the more realistic simulation anyway.

Verdict

Approve after nits - no blocking findings; the 🟠 nested-count retention only shows up when a nested dialog Root sits outside a handle-backed parent's Popup.


🤖 Review generated with Claude Code

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 7, 2026
@atomiks
atomiks force-pushed the claude/charming-mendeleev-a942e9 branch from 9db1021 to f5cdc95 Compare October 7, 2026 07:09
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 7, 2026
@michaldudak

Copy link
Copy Markdown
Member

PR review

The author fixed every finding from the last round except the optional compositionstart simplification. Nothing in the new code blocks merge. The PR's source changes are byte-identical to what I reviewed before; the only new code is the review-feedback commit (f5cdc95ca). One gap remains: no test covers the new reset of the nested-dialog count. CI's test jobs are red, but not clearly because of this PR (see the last finding).

How the earlier findings were handled:

  • Nested count while closed (was 🟠): onNestedDialogOpen now ignores updates while the dialog is neither open nor mounted, and a layout effect zeroes the counts once the dialog fully closes. I checked it against:

    • the original help-dialog scenario;
    • controlled open, which syncs after a child's layout effect, the same as on master;
    • the close transition, where updates are still accepted while mounted;
    • the order of a child's unmount and the reset in the same commit.

    None of these regress.

  • IME timeout (was ℹ️): the timeout is now a useTimeout that survives effect re-runs, so the flag can't get stuck.

  • Test gaps:

    • The stickIfOpen test now advances the clock.
    • The trigger-takeover test now checks data-pressed and focus-guard ownership.
    • There's a new Drawer render-count test and a nested open/close/reopen test.
    • The order-dependent tests now have a comment.

Tests (2)

1. 🟡 The reset of the nested count on close isn't tested

Location: packages/react/src/dialog/root/useDialogRoot.ts:101

useIsoLayoutEffect(() => {
  if (!open && !mounted) {
    setOwnNestedOpenDialogs(0);
    setOwnNestedOpenDrawers(0);
  }
}, [open, mounted]);

Neither new test fails if this effect is deleted:

  • In the help-dialog test, the parent is closed the whole time the section dialog opens, so the guard in the callback does all the work.
  • The open/close test nests the child inside the parent's Popup, so the child's own close notification zeroes the count.

The effect only matters in this sequence:

  1. The parent P is open.
  2. A child Root that sits inside P's Root but outside its Popup opens, so P's count becomes 1.
  3. P closes (for example through the handle or controlled open) while the child stays open.
  4. P reopens. Without the reset it still shows data-nested-dialog-open, and Escape and backdrop presses don't close it.

Fix: Add that sequence as a test, and assert that the reopened P has no data-nested-dialog-open and closes on Escape.

2. ℹ️ CI's PreviewCardViewport failure didn't reproduce locally

Location: packages/react/test/viewportConformanceTests.tsx:145

On this head, the React 19 "Browser tests" job fails "does not restart a transition when the active trigger remounts while closed and retained", in both its strict and non-strict variants and on both attempts. The popup is no longer data-closed after the trigger remounts. Master's run of the same test passes.

All the other red test jobs fail only FieldsetLegend "reflects the disabled state", which master fails as well.

I couldn't reproduce the PreviewCardViewport failure:

  • That test file alone passed.
  • The whole preview-card directory passed in five parallel runs and two serial ones.
  • A copy of the scenario passed 15 repeats at 6× CPU throttling, both with and without the cursor parked on the trigger.
  • The full Chromium suite on this head passed everything except FieldsetLegend.

The fixture uses no handle, so the eager mount doesn't apply. The PR's only change in that path is in useDismiss (state resets on close, the composition timer, the isComposing check), and none of it can open a popup. The test came from master's #5887/#5892 a day ago, so it may be a new flake rather than something this PR caused.

Fix: Re-run the job. If it fails again, pull the DOM dump from the CircleCI step log to see what reopened the popup.

Verdict

Approve after nits - the earlier 🟠 is fixed; what's left is a 🟡 test gap and a CI failure I couldn't tie to the PR.


🤖 Review generated with Claude Code

@atomiks
atomiks merged commit 1f112a5 into mui:master Oct 7, 2026
21 of 22 checks passed
@atomiks
atomiks deleted the claude/charming-mendeleev-a942e9 branch October 7, 2026 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance scope: all components Widespread work has an impact on almost all components. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[popover][preview card][dialog] Opening or closing re-renders every detached trigger

2 participants