[tooltip] Fix flickering when nesting tooltip inside another tooltip's trigger#4501
[tooltip] Fix flickering when nesting tooltip inside another tooltip's trigger#4501michaldudak wants to merge 14 commits intomui:masterfrom
Conversation
…s trigger Prevent the outer tooltip from opening when the cursor is over an enabled nested tooltip trigger. When the outer tooltip is already open and the pointer moves onto a nested trigger, close it via a bubbling custom DOM event so that only the innermost tooltip is visible. Disabled nested triggers are excluded from detection so the parent tooltip still opens. The close path is gated to hover-opened tooltips only — focus-opened tooltips remain visible until blur. Closes mui#4499 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
michaldudak
added
the
component: tooltip
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
commit: |
michaldudak
requested review from
LukasTy,
atomiks,
colmtuite and
flaviendelangle
as code owners
Codex ReviewOverviewThis PR fixes the nested-tooltip flicker reported in #4499 by preventing a parent tooltip from opening, or staying open, while the pointer is over an enabled nested trigger. It does that by adding a nested-trigger guard in the tooltip hover path, closing hover-opened ancestors on nested trigger entry, and documenting/testing the new behavior. Findings (None)No blocking issues found in this patch. I reviewed the hover-delay path in Confidence: 4/5High confidence. The branch includes targeted coverage for the key delayed-hover sequence where the pointer enters the outer trigger first and then moves onto the nested trigger before the delay expires. Residual risk is limited to the usual complexity around hover timing and composed event flow. Notes
|
atomiks
left a comment
atomiks
left a comment
There was a problem hiding this comment.
Codex Review (GPT-5.4)
This is a bug-fix PR that does fix the light-DOM nested-tooltip flicker path, and the targeted TooltipRoot suite plus fresh size snapshots against master back that up. The remaining concern is that the new nested-trigger detection is not shadow-DOM-safe, and the current shape adds roughly +996 B parsed / +326 B gzip to @base-ui/react/tooltip for a tooltip-only edge case.
1. Bugs / Issues
1. 🟡 Nested-trigger detection is not shadow-DOM-safe (non-blocking)
In packages/react/src/tooltip/trigger/TooltipTrigger.tsx, the delayed-open guard finds the hovered nested trigger via document.elementFromPoint(...).closest(...), while the already-open close path depends on a retargeted bubbling DOM event. This is code-based reasoning rather than a local repro, but in shadow-root embeddings closest() does not walk past the shadow boundary to the trigger host, so the ancestor tooltip can still open even though the pointer is over an enabled nested trigger.
From packages/react/src/tooltip/trigger/TooltipTrigger.tsx:
const elementUnderCursor = doc.elementFromPoint(pos.x, pos.y);
if (!elementUnderCursor) {
return true;
}
const nearestTrigger = elementUnderCursor.closest(ENABLED_TOOLTIP_TRIGGER_SELECTOR);
if (nearestTrigger && nearestTrigger !== triggerEl && contains(triggerEl, nearestTrigger)) {
return false;
}This matters because the rest of the floating layer explicitly uses shadow-safe targeting helpers like contains and getTarget, so this branch narrows Tooltip’s composed-tree guarantees relative to adjacent primitives.
Fix: Replace the closest()/custom-event combination with a composed-path-aware signal, or track nested-trigger hover from bubbling pointerover/mouseover state so the guard stays shadow-DOM-safe.
2. Root Cause & Patch Assessment
The branch addresses both halves of the reported bug in normal light DOM: it suppresses a delayed outer open when the pointer is already over the inner trigger, and it closes a hover-opened outer tooltip when the pointer moves onto the nested trigger. The new TooltipRoot tests cover those light-DOM cases well, and I did not find another concrete interaction defect in that path.
The downside is that the fix is split across two mechanisms in TooltipTrigger: cursor-position sampling plus elementFromPoint() for future opens, and a bubbling custom event for already-open ancestors. That makes the patch heavier and harder to reason about than the original bug warrants.
3. Pattern Consistency Assessment
Existing floating utilities in this repo consistently lean on store-based trigger tracking and shadow-safe helpers. This patch is the one place that falls back to raw DOM traversal plus a public marker attribute, so it breaks that pattern instead of extending it.
4. Simplification Opportunities
The biggest simplification opportunity is to keep this tooltip-specific behavior out of useHoverReferenceInteraction. Adding shouldOpen widens a shared hover primitive for a case that only Tooltip uses, which means extra hot-path branching for other consumers as well.
Bundle snapshot results against fresh master:
| Entrypoint | master |
PR | Delta |
|---|---|---|---|
@base-ui/react/tooltip |
95,381 / 32,223 |
96,377 / 32,549 |
+996 parsed / +326 gzip |
@base-ui/react |
465,728 / 146,125 |
466,689 / 146,437 |
+961 parsed / +312 gzip |
I would try to collapse this to one local signal in TooltipTrigger instead of the current two-part design, and I would avoid documenting data-tooltip-trigger unless the intent is to support that marker as public API long-term.
6. Test Coverage Assessment
pnpm test:jsdom TooltipRoot --no-watch passed, including the new nested-tooltip cases. Those tests cover the light-DOM happy path, delayed-open cancellation, already-open ancestor close, and the explicit disabled prop case.
The main missing regression case is a shadow-root embedding, which is exactly where the current detection logic looks fragile. If this approach stays, I would add that case before merging.
Merge Confidence Score
Overall merge confidence is 3/5. The reported flicker looks fixed for the newly tested light-DOM scenarios, but I would address issue 1 before merging if Tooltip is expected to preserve the same shadow-DOM behavior as the rest of the floating utilities. Even if that risk is accepted, I would still simplify the implementation so the ~0.3 KB gzip increase is paying for the smallest possible fix.
atomiks
left a comment
atomiks
left a comment
There was a problem hiding this comment.
Follow-up on the simplification point: I think this can likely be collapsed further than just moving the logic out of useHoverReferenceInteraction.
Right now Tooltip is paying for two separate mechanisms for the same nested-trigger behavior:
- suppress a future delayed open when the pointer is already over an inner trigger
- close an already-open outer tooltip when the pointer moves onto that inner trigger
Those could probably be driven from one local TooltipTrigger signal such as “pointer is currently over a nested enabled tooltip trigger”, derived from bubbling pointerover/mouseover state. If that works, you should be able to drop most or all of:
lastMousePositionRefelementFromPoint()- the custom bubbling DOM event
- the layout-effect listener for that event
- the need to surface
data-tooltip-triggeras public docs/API
So splitting the generic hover hook would stop other components from paying for this, but Tooltip would still carry most of the current cost unless the duplicated local machinery is collapsed too. My impression is that the real bundle win is not just “make it tooltip-only”, but “make it one local mechanism instead of two.”
I think the one caveat is that Tooltip still needs some private way to cancel a pending delayed open, so there may still be a small pre-open guard somewhere. But that should be materially smaller than the current elementFromPoint() + custom-event combination.
![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: d527ac0ce1
ℹ️ 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".
packages/react/src/floating-ui-react/hooks/useHoverReferenceInteraction.ts
Show resolved
Hide resolved
|
I believe we have to resort to checking the I also simplified the implementation and now use just a single call in As for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d33beed4a
ℹ️ 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".
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad7d99f6cd
ℹ️ 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".
| !store.select('open') && | ||
| getOpenDelay() === 0 && | ||
| triggerEl |
There was a problem hiding this comment.
When the pointer remains over an enabled nested trigger longer than the parent tooltip’s non-zero open delay, the parent’s pending open is suppressed and consumed while nested hover is active. After moving back to the parent area, this branch only reopens when getOpenDelay() === 0, but Tooltip hover uses move: false, so there is no subsequent mouseenter/mousemove path to schedule reopening. In practice, delayed parent tooltips can get stuck closed until the cursor fully leaves and re-enters the trigger.
Useful? React with 👍 / 👎.
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review: [tooltip] Fix flickering when nesting tooltip inside another tooltip's trigger
PR: #4501
Author: @michaldudak
Base: master
Summary
This PR prevents the outer tooltip from opening when the cursor is over an enabled nested tooltip trigger. When the outer tooltip is already open and the pointer moves onto a nested trigger, it closes via the bubbling mouseover event so that only the innermost tooltip is visible. Disabled nested triggers are excluded from detection so the parent tooltip still opens. The close path is gated to hover-opened tooltips only -- focus-opened tooltips remain visible until blur.
Files changed: 10 (+651 / -23)
Critical Issues (1)
1. Missing test: focus-opened tooltip should NOT be closed when hovering nested trigger
handleNestedTriggerHover explicitly checks store.select('lastOpenChangeReason') === REASONS.triggerHover before closing the parent. This means a focus-opened outer tooltip should remain open when the pointer moves onto a nested trigger. This is a critical behavioral distinction -- accidentally closing a keyboard user's tooltip would be an accessibility regression. There is no test verifying this path.
A test should: focus the outer trigger to open the outer tooltip, then hover the nested trigger, and assert the outer tooltip remains visible.
Important Issues (4)
2. Missing test: rapid oscillation between nested trigger and parent area
The most common flickering scenario in production is rapid back-and-forth pointer movement between the nested trigger and the parent area. The existing tests cover one-directional movements but not the oscillating case. A test should simulate: hover nested trigger, move to outer area, quickly move back to nested trigger, and verify only the inner tooltip is shown.
3. PR description inaccuracy: "bubbling custom DOM event"
The PR body states "close it via a bubbling custom DOM event." The actual implementation uses the native mouseover event (which naturally bubbles) combined with handleNestedTriggerHover on onMouseOver. There is no custom DOM event (new CustomEvent(...) or dispatchEvent(...)) involved. The description should say something like "close it when the bubbling mouseover event reaches the outer trigger and detects a nested trigger target."
4. Dual state management paths for isNestedTriggerHoveredRef
The ref is updated from two different locations:
detectNestedTriggerHoversets it ononMouseOveronMouseLeaveunconditionally resets it tofalse
This creates two separate sources of truth. If the logic for what constitutes "nested trigger hovered" ever changes in detectNestedTriggerHover, the raw reset in onMouseLeave could become inconsistent. Additionally, if a pending open timer fires between the mouseover (which set the ref to true) and the mouseleave (which resets it to false), the tooltip open could be incorrectly suppressed, though this window is likely extremely small.
5. shouldOpen return-value contract is implicit
The shouldOpenRef.current?.() !== false pattern means any falsy-but-not-false value (undefined, null, 0, "") is treated as "should open." If a future contributor modifies shouldOpen to return undefined as a signal for "don't open," this pattern silently allows opening. TypeScript constrains this at compile time, but at runtime a function that accidentally returns undefined (forgot the return statement) would silently allow opening instead of preventing it. A brief inline comment would aid maintainability.
Suggestions (4)
6. Extract checkShouldOpen() helper
The pattern shouldOpenRef.current?.() !== false appears 4 times in useHoverReferenceInteraction.ts. Extracting a local helper:
function checkShouldOpen(): boolean {
return shouldOpenRef.current?.() !== false;
}would replace the somewhat cryptic double-negation with a readable call at each site.
7. Pass getOpenDelay directly as restMs
Instead of:
restMs() {
return getOpenDelay();
},Since useStableCallback already returns a stable function reference:
restMs: getOpenDelay,8. Missing test: three levels of nesting
The implementation uses closest() to find the nearest enabled [data-tooltip-trigger] ancestor, then checks nearestTrigger !== triggerEl && contains(triggerEl, nearestTrigger). With three levels (grandparent > parent > child), hovering the child should suppress both grandparent and parent tooltips. A basic smoke test would confirm the closest() traversal works correctly at multiple depths.
9. Missing test: Tooltip.Provider with nested tooltips
The experiment page (nested.tsx) demonstrates nested tooltips wrapped in Tooltip.Provider, but no test exercises this combination. The getOpenDelay function interacts with providerContext?.delay and the group delay logic. A test with <Tooltip.Provider> wrapping nested tooltips would verify the delay computation does not interfere with nesting suppression.
Strengths
-
Solid architecture: The
shouldOpencallback is a clean, generic integration point inuseHoverReferenceInteractionthat doesn't couple the hook to tooltip-specific nesting knowledge. The three-piece chain (detectNestedTriggerHover+isNestedTriggerHoveredRef+shouldOpen) cleanly separates DOM detection from generic open-gating. -
Shadow DOM support: The new
closest()utility correctly traverses shadow boundaries, and there is a dedicated test for it. This is the kind of environment-specific edge case that often gets missed. -
Well-written tests: 8 tests covering core scenarios with realistic event sequences, behavior-oriented assertions, and proper cleanup (shadow DOM test uses
finallyblock). The "cancel pending open" test directly targets the flickering bug. -
Good comments: The zero-delay comment in
handleNestedTriggerHoverexplains "why" rather than "what." TheshouldOpenJSDoc is well-structured. Theclosest()JSDoc concisely explains the traversal strategy. -
Correct handling of disabled nested triggers: The
ENABLED_TOOLTIP_TRIGGER_SELECTORuses:not([data-trigger-disabled])so disabled nested triggers are transparent to the parent, allowing the parent tooltip to open normally. -
handleNestedTriggerHoverstructure: The enter/leave cases are well-paired in a single function with a clear early return. Splitting into separate handlers would duplicate detection logic for no benefit.
Recommended Action
- Add the focus-opened tooltip test (critical for accessibility)
- Fix the PR description to accurately describe the mechanism
- Consider adding the rapid oscillation test
- Consider the minor code simplifications (items 6 and 7)
github-actions
bot
added
the
PR: out-of-date
4 participants
Prevent the outer tooltip from opening when the cursor is over an enabled nested tooltip trigger. When the outer tooltip is already open and the pointer moves onto a nested trigger, close it via a bubbling custom DOM event so that only the innermost tooltip is visible. Disabled nested triggers are excluded from detection so the parent tooltip still opens.
The close path is gated to hover-opened tooltips only — focus-opened tooltips remain visible until blur.
Closes #4499