Skip to content

[tooltip] Close when active trigger unmounts#4886

Open
michaldudak wants to merge 5 commits into
mui:masterfrom
michaldudak:codex/tooltip-active-trigger-removal-close
Open

[tooltip] Close when active trigger unmounts#4886
michaldudak wants to merge 5 commits into
mui:masterfrom
michaldudak:codex/tooltip-active-trigger-removal-close

Conversation

@michaldudak
Copy link
Copy Markdown
Member

@michaldudak michaldudak commented May 22, 2026

Summary

  • Clear stale active-trigger state when the active trigger unregisters while the tooltip is open.
  • Close the tooltip instead of implicitly re-anchoring it to another trigger.
  • Cover contained and detached trigger removal.

Bug

When the active trigger unmounts, unregistering removes the DOM node from the trigger map but leaves the active trigger id/element in store state. Conditional or virtualized triggers can then leave the tooltip open against stale anchor state instead of closing.

Reproduction

Test plan

  • pnpm test:jsdom popupStoreUtils --no-watch
  • pnpm test:chromium TooltipRoot.detached-triggers --no-watch

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 22, 2026

commit: 2523647

@code-infra-dashboard
Copy link
Copy Markdown

code-infra-dashboard Bot commented May 22, 2026

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+527B(+0.11%) 🔺+127B(+0.09%)

Details of bundle changes

Performance

Total duration: 1,521.77 ms 🔺+308.78 ms(+25.5%) | Renders: 50 (+0) | Paint: 2,307.71 ms 🔺+477.99 ms(+26.1%)

Test Duration Renders
Tabs mount (200 instances) 313.81 ms 🔺+84.79 ms(+37.0%) 4 (+0)
Menu mount (300 instances) 170.53 ms 🔺+35.67 ms(+26.5%) 2 (+0)
Slider mount (300 instances) 202.91 ms 🔺+35.00 ms(+20.8%) 3 (+0)
Scroll Area mount (300 instances) 112.80 ms 🔺+32.25 ms(+40.0%) 3 (+0)
Menu open (500 items) 100.49 ms 🔺+21.81 ms(+27.7%) 12 (+0)

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


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

@michaldudak michaldudak added type: bug It doesn't behave as expected. component: tooltip Changes related to the tooltip component. labels May 22, 2026
@netlify
Copy link
Copy Markdown

netlify Bot commented May 22, 2026

Deploy Preview for base-ui ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2523647
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a195d43471a90000871e918
😎 Deploy Preview https://deploy-preview-4886--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.

@michaldudak michaldudak force-pushed the codex/tooltip-active-trigger-removal-close branch from b0d6bb6 to b83fbbc Compare May 22, 2026 10:26
@michaldudak michaldudak marked this pull request as ready for review May 29, 2026 09:39
Copy link
Copy Markdown
Member Author

Codex Review (GPT-5.5)

Dominant type: bug-fix. I reviewed the full PR diff against freshly fetched upstream/master; the patch addresses stale tooltip active-trigger state when the owning trigger unregisters, with focused coverage for contained and detached trigger setups.

1. Bugs / Issues (None)

I did not find any concrete blocking or non-blocking issues in the implementation.

Root Cause & Patch Assessment

The fix targets the shared active-trigger reconciliation point in useImplicitActiveTrigger, which is the right layer for detecting that the active trigger id no longer maps to a registered element. Tooltip opts into closing on that condition, while other popup consumers retain the previous “clear stale active trigger” behavior without being forced closed.

The microtask guard is doing useful work here: it avoids closing if the same trigger remounts before the deferred check runs, and it respects canceled onOpenChange events before clearing active-trigger state.

Test Coverage Assessment

Coverage looks proportionate for this bug. The PR adds utility-level coverage for unregistering the active trigger and Chromium tooltip coverage for both contained and detached triggers, including a canceled close path.

I ran:

pnpm test:jsdom popupStoreUtils --no-watch
pnpm test:chromium TooltipRoot.detached-triggers --no-watch
pnpm typescript

All passed. I did not replay the new regression assertions against upstream/master.

Recommendation

Approve ✅

The change is narrowly scoped, covered at the shared utility and Tooltip integration levels, and the relevant targeted tests plus typecheck pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: tooltip Changes related to the tooltip component. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant