Skip to content

[popups] Improve mount performance#4649

Draft
atomiks wants to merge 10 commits intomui:masterfrom
atomiks:popups/perf-gain
Draft

[popups] Improve mount performance#4649
atomiks wants to merge 10 commits intomui:masterfrom
atomiks:popups/perf-gain

Conversation

@atomiks
Copy link
Copy Markdown
Contributor

@atomiks atomiks commented Apr 20, 2026

Changes

This PR trims popup work from the closed path so we only pay for interaction setup when a popup is actually open or in its mounted transition state.

  • Deferred root-side interaction wiring in the popup-heavy components
  • Removed the generic useRole + useInteractions hooks and replaced them explicit trigger/popup ARIA and mergeProps
  • Reduced extra popup-store synchronization work by moving floating-root setup into the store layer and syncing only the dynamic parts after commit. Renders down from 2 to 1 for Tooltip, Dialog, Popover, Preview Card. Menu is still 2.
  • Avoided idle close-completion logic in the shared popup utilities so closed popups that never mounted do not pay teardown costs. (Massive 85% teardown gain here.)
  • Kept Menu on the safer path for correctness (trying to match the others broke stuff too easily), but still reduced some closed-path root work and tightened trigger ownership behavior.
  • Added regression coverage for detached triggers, multiple triggers, first-open behavior, and popup ownership in the touched popup suites.

Why

The profiling showed two main costs:

  • closed popups were still doing too much interaction/setup work on initial mount
  • unmount was spending a lot of time cleaning up bookkeeping that never mattered for popups that were never opened

So the changes were aimed at:

  • making mount cheaper by deferring non-essential popup interactions (gated by open || mounted)
  • making unmount cheaper by skipping dead close-transition work
  • keeping detached-trigger, controlled, and multi-trigger behavior correct while doing that
Benchmark upstream/master This PR Delta
Contained triggers mount (all: tooltip + popover + dialog + menu) 127.66 ms 73.62 ms -54.04 ms (-42.3%)
Contained triggers mount (tooltip only) 18.88 ms 11.50 ms -7.38 ms (-39.1%)
Contained triggers mount (preview card only) 18.90 ms 10.40 ms -8.50 ms (-45.0%)
Contained triggers mount (popover only) 26.12 ms 12.44 ms -13.68 ms (-52.4%)
Contained triggers mount (dialog only) 21.70 ms 10.14 ms -11.56 ms (-53.3%)
Contained triggers mount (menu only) 32.54 ms 26.08 ms -6.46 ms (-19.9%)
Menu open 7.44 ms 7.38 ms -0.06 ms (-0.8%)
Select open 51.19 ms 51.43 ms +0.24 ms (+0.5%)
Contained triggers warm unmount (all) 119.95 ms 17.25 ms -102.7 ms (-85.6%)

@atomiks atomiks added performance scope: all components Widespread work has an impact on almost all components. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Apr 20, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 20, 2026

commit: eab0afb

@code-infra-dashboard
Copy link
Copy Markdown

code-infra-dashboard bot commented Apr 20, 2026

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+1.44KB(+0.31%) 🔺+392B(+0.27%)

Details of bundle changes

Performance

Total duration: 1,200.54 ms ▼-129.36 ms(-9.7%) | Renders: 50 (▼-3) | Paint: 1,841.27 ms ▼-249.85 ms(-11.9%)

Test Duration Renders
Select mount (200 instances) 192.42 ms 🔺+45.19 ms(+30.7%) 3 (+0)
Popover mount (300 instances) 62.86 ms ▼-36.41 ms(-36.7%) 1 (▼-1)
Dialog mount (300 instances) 53.54 ms ▼-28.94 ms(-35.1%) 1 (▼-1)
Menu mount (300 instances) 118.26 ms ▼-41.73 ms(-26.1%) 2 (+0)
Tooltip mount (300 contained roots) 47.17 ms ▼-15.68 ms(-25.0%) 1 (▼-1)

...and 7 more. View full report

Details of benchmark changes


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

@atomiks atomiks changed the title [all components] Improve popup performance [popups] Improve mount performance Apr 20, 2026
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 20, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit eab0afb
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/69e648ddf26d5f0008a39fec
😎 Deploy Preview https://deploy-preview-4649--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@michaldudak
Copy link
Copy Markdown
Member

Critical (must fix before merge)

C1 — Tooltip popup no longer sets role="tooltip"

File: packages/react/src/tooltip/popup/TooltipPopup.tsx
With useRole deleted, PopoverPopup (role: 'dialog') and MenuPopup (role: 'menu') got hardcoded replacements; TooltipPopup did not. The new test in TooltipRoot.test.tsx:38-55 asserts aria-describedby is gone from the trigger — so tooltips are now invisible to assistive tech. A11y regression with no loud failure.

C2 — TooltipTrigger onClick handler is either dead or double-fires

File: packages/react/src/tooltip/trigger/TooltipTrigger.tsx:143-147

onClick(event) {
  if (closeOnClick && !store.select('open')) {
    store.setOpen(false, createChangeEventDetails(REASONS.triggerPress, event.nativeEvent));
  }
},

Guard fires only when the tooltip is not open, then calls setOpen(false) — either a no-op, or (since TooltipStore.setOpen has no same-state early-return) a duplicate onOpenChange fire. The new "calls onOpenChange once" test at TooltipRoot.test.tsx:891-914 likely passes by accident. Condition looks inverted.

C3 — Render-time mutation of store.state.floatingRootContext

File: packages/react/src/utils/popups/popupStoreUtils.ts:223
usePopupRootSync assigns store.state.floatingRootContext = … during render. Only the external-store branch calls notifyAll() inside a layout effect; non-external subscribers via store.useState('floatingRootContext') are not woken. Works today only because useSyncedFloatingRootContext returns a stable reference — any future change to that ref (e.g., Strict Mode double-render) silently desyncs subscribers. Use store.set(...) or useSyncedValue to make notification explicit.

C4 — openMethod never cleared on unmount; detached-trigger click never writes it

  • popupStoreUtils.ts:236-240 resets openMethod only when !open. If root unmounts while open === true, or a handle-based consumer has no mounted root, openMethod leaks to the next opener.
  • DialogTrigger.tsx:88 and PopoverTrigger.tsx:145 route handle-equipped triggers to rootTriggerProps, which relies on a mounted root running useOpenMethodTriggerProps. Handle-only consumers get no openMethod written. Silent focus-style / iOS touch regression.

Important

I1 — MenuRoot rewritten with zero new Menu tests

MenuRoot.tsx lost useRole, gained usePopupRootSync, gated dismiss/typeahead on open, yet MenuRoot.test.tsx and MenuRoot.detached-triggers.test.tsx are untouched. Gaps:

  • Dismiss/typeahead now enabled: open && !disabled — while the close-animation plays, Escape / outside-press / typeahead silently do nothing. Inconsistent with popupProps memo at MenuRoot.tsx:513 which uses open || mounted.
  • aria-haspopup: 'menu' and popup aria-labelledby: activeTriggerId moved from useRole to manual wiring — no tests assert these remain.
  • MenuRoot.tsx:103 changed open: defaultOpenopen: openProp ?? defaultOpen (SSR-visible first-paint change) with no test.

I2 — PreviewCard refactor entirely untested

No changes to preview-card test files. With useRole deleted, the aria-expanded/trigger ARIA path is gone and no replacement is visible in the PR. Also:

  • PreviewCardRoot.tsx:85-86 uses dismiss.reference ?? EMPTY_OBJECT — Dialog/Popover roots omit the fallback, creating an inconsistent contract (see I3).
  • PreviewCardStore.ts:122-134 conditionally calls useSyncedValue('floatingRootContext', …) only for external stores; no handle-based preview-card test exercises this.

I3 — inactiveTriggerProps = dismiss.trigger may be undefined in Dialog/Popover

useDialogRoot.ts:152-153 and the equivalent in PopoverRoot.tsx:129-150 don't apply the ?? EMPTY_OBJECT fallback. useDismiss returns {} when disabled, so current behavior may be safe, but the divergence from PreviewCardRoot signals unclear contract. Either remove the preview-card fallback or add it to Dialog/Popover.

I4 — Stale open read in Menu triggers

MenuTrigger.tsx:223-225 and MenuSubmenuTrigger.tsx:161-163 call useOpenMethodTriggerProps(store.select('open'), …). store.select doesn't subscribe, so the click handler sees a stale open. DialogTrigger/PopoverTrigger use store.useState('open') here — Menu should match.

I5 — usePopupRootSync helper has no tests

popupStoreUtils.ts:199-239 is shared by Dialog/Popover/Menu/Tooltip roots but popupStoreUtils.test.tsx wasn't updated. The external-store notifyAll() path is entirely uncovered.

I6 — Duplicated trigger-prop assembly across triggers

aria-haspopup / aria-expanded / aria-controls and the controlsPopup = open && (isOpenedByThisTrigger || activeTriggerId == null || store.context.triggerElements.size === 1) expression are copy-pasted across DialogTrigger, PopoverTrigger, MenuTrigger. AGENTS.md explicitly calls out avoiding duplication — extract into a shared hook next to useTriggerDataForwarding.

I7 — Inconsistent Root context shapes after refactor

DialogRootContext.ts and PopoverRootContext.ts switched from { store } to the store directly, but MenuRootContext.ts still wraps { store, parent }. Internal types only, but confusing to readers.


Medium / Low

  • M1MenuRoot.tsx:372 reads store.state.floatingRootContext directly; every other consumer uses store.useState(...). Diverges from the pattern.
  • M2 — One-frame stale triggerProps after popup opens in Dialog/Popover (*Interactions renders only when open || mounted, so the first render missing dismiss handlers). Minor — one lost frame of outside-press handling.
  • L1useOpenChangeComplete now gated mounted && !open && !preventUnmountingOnClose (popupStoreUtils.ts:186) — intentional but changes when the completion callback fires.
  • L2usePopupRootSync types eventDetails as any at popupStoreUtils.ts:214. Erodes type safety around reason codes. AGENTS.md discourages as any casts.
  • L3 — Dropped sentence in the iOS Safari comment at useOpenInteractionType.ts:11-12 — the "interactionType will be '' in that case" clause explained why the || fallback trips. Restore it.
  • L4 — Nested role: 'menuitem' for nested-menu triggers previously supplied by useRole — not restored. Verify MenuSubmenuTrigger still produces the right role.
  • L5useRole previously used getFloatingFocusElement(floatingElement)?.id || defaultFloatingId; the replacements use plain floatingId. Minor edge case when a child owns focus.

@atomiks atomiks requested a review from romgrk April 20, 2026 13:27
Comment on lines -184 to 199
enabled: !preventUnmountingOnClose,
enabled: mounted && !open && !preventUnmountingOnClose,
open,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line fixes unmount/teardown perf:

Contained triggers warm unmount (all) 119.95 ms 17.25 ms -102.7 ms (-85.6%)

* Documentation: [Base UI Dialog](https://base-ui.com/react/components/dialog)
*/
export function DialogRoot<Payload>(props: DialogRoot.Props<Payload>) {
export const DialogRoot = function DialogRoot<Payload>(props: DialogRoot.Props<Payload>) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this useful?

externalStore: DialogStore<Payload> | undefined,
initialState?: Partial<State<Payload>>,
) {
// eslint-disable-next-line react-hooks/rules-of-hooks
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also wrap the method body with enable/disable eslint comments.

Comment on lines 428 to +432
const dialogRootContext = useDialogRootContext(false);

const open = dialogRootContext.store.useState('open');
const nestedOpenDialogCount = dialogRootContext.store.useState('nestedOpenDialogCount');
const popupElement = dialogRootContext.store.useState('popupElement');
const open = dialogRootContext.useState('open');
const nestedOpenDialogCount = dialogRootContext.useState('nestedOpenDialogCount');
const popupElement = dialogRootContext.useState('popupElement');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the variable to store? It would match the rest of the codebase.

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: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants