[popups] Fix rendered trigger id ownership - #5110
Conversation
atomiks
added
type: regression
commit: |
Bundle size
PerformanceTotal duration: 1,415.74 ms +140.99 ms(+11.1%) | Renders: 78 (+0)
10 tests within noise — details Metric alarms
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
This is a tight, well-targeted fix for #5108 (tooltip triggers rendered with a custom id closing immediately after opening). The reassociation logic in useImplicitActiveTrigger is correct, self-limiting (slow-path only, runs once per open), and properly regression-tested at both the store and component level. Nothing here is merge-blocking. Two informational notes below: a one-time viewport content remount that this fix newly introduces for a narrow combination, and a stale function-level comment.
Bugs (0)
No findings.
Tests (0)
No findings.
Simplifications (0)
No findings.
Docs (1)
1. ℹ️ note — function comment doesn't mention the new reassociation path
Location: packages/react/src/utils/popups/popupStoreUtils.ts:323-339
* When a popup opens without an explicit trigger id and exactly one trigger is registered, that
* trigger is claimed as the active trigger. When the active trigger id is still registered but its
* element changed, the active element is refreshed. When the active trigger unregisters, the
* default path preserves existing ownership so non-closing popup families do not silently claim a
* different trigger while staying open.The JSDoc enumerates the three existing reconciliation paths but not the new one this PR adds: when the active trigger id is not found by direct lookup, the registry is scanned for the same element under a different id and the active id is reassociated (the whole point of the fix). The non-obvious branch — a DOM id differing from Base UI's internal trigger id — is exactly the kind of thing the next maintainer will want explained here.
Failure scenario: A future reader sees the getById miss fall through to a loop with no comment explaining why an element can be registered under an id other than activeTriggerId, and either misreads it as dead code or breaks it during refactor.
Fix: Add a sentence, e.g. "When the active trigger id is missing from the registry but the same element is registered under a different id (e.g. the rendered element carries its own DOM id), the active id is reassociated to the registered id instead of being treated as lost."
Verdict
Approve — correct, minimal fix with solid regression coverage; only an optional comment update and one narrow heads-up below.
A heads-up worth recording even though it doesn't rise to a finding: the reassociation flips activeTriggerId from the DOM id to the internal registered id once, just after open. For consumers of <X.Viewport> (usePopupContentKey keys its remount on activeTriggerId), a trigger rendered with a custom id will now produce a single content remount right after opening. For Tooltip/PreviewCard (closeOnActiveTriggerUnmount: true) this is strictly better than the prior behavior (full close). For Popover/Menu it's a new one-time remount in exchange for correctly marking the trigger active (isOpenedByTrigger / data-popup-open were previously wrong in this case). It's an acceptable trade, but if the remount ever matters, keying usePopupContentKey on activeTriggerElement identity instead of activeTriggerId would be stable across the reassociation.
🤖 Review generated with Claude Code
| const reactiveTriggerCount = store.useState('triggerCount'); | ||
| // Subscribe to the active trigger id so the reconciliation below reruns when ownership moves to | ||
| // another trigger while the popup stays open (e.g. a focus/hover handoff between triggers). | ||
| const reactiveActiveTriggerId = store.useState('activeTriggerId'); |
There was a problem hiding this comment.
nit: I'm not fan of this name. Could we call it just activeTriggerId (and rename what's currently activeTriggerId in effects to currentActiveTriggerId - there are some places that already use this convention)
atomiks
added
the
component: preview card
atomiks
added
component: popover
atomiks
added
scope: all components
|
@atomiks and @michaldudak , do you intend to release a point release that includes this fix? Or will it be part of |
|
@ciampo whether a patch release goes out before 1.7.0 depends on how often this is actually hit — I'm not sure how common putting an id on the element passed to the The workaround for the time being is simple at least: move the |
|
@atomiks sounds good, thank you for the reply |
|
@atomiks this issue is hurting users, why would a patch release not be the best solution here? |
|
@Kosai106 for context we limit stable releases to once per month and reserve patch releases for widespread regressions without a practical workaround. So far, we’ve received relatively few reports of this issue, and there is a straightforward workaround. If the workaround isn’t viable (placing the The next stable release is scheduled for next week as well, so the fix will be live shortly for everyone. (This release came a little later than usual because we’ve shifted our monthly cadence to the first week of each month) |
5 participants
Fixes #5108
A trigger rendered with an element that already has its own
id(e.g.render={<button id="custom-button" />}) detaches the trigger's DOMidfrom the internal id Base UI registers it under. The active trigger is tracked by DOMid(setPopupOpenStatereadstrigger.id), while the registry and ownership selectors key off the internal id, so the trigger is never recognized as active. This affects every popup family that sharesuseImplicitActiveTrigger, at two severity levels:Affected components & severity
🔴 Closes immediately after opening — Tooltip, Preview Card
Both enable
closeOnActiveTriggerUnmount, so the failed registry lookup is treated as the active trigger unmounting and the popup closes right after it opens. This is the #5108 regression introduced in #4886 ([tooltip][preview card]) — 1.6.0 fails, 1.5.0 works.🟡 Opens, but trigger ownership / ARIA is wrong — Popover, Menu, Dialog
No close-on-unmount, so the popup stays open, but the trigger never registers as active:
data-popup-openis missing andaria-expanded/aria-controlsare stale. This one is pre-existing (independent of #4886), surfaced by the same id mismatch.Root cause
setPopupOpenStaterecordsactiveTriggerIdfrom the trigger's DOMid. When arenderid overrides Base UI's internal id, that value is not in the trigger registry, sogetByIdmisses and the ownership selectors (isOpenedByTrigger, etc.) never match the trigger.Fix
In
useImplicitActiveTrigger, when the active id misses a direct registry lookup, scan for the same element registered under a different id and reassociate to the registered id instead of treating it as lost. The reconciliation is also made reactive to active-trigger-id changes so it covers trigger-to-trigger handoffs while the popup stays open.Perf
The scan only runs after the fast
getByIdlookup misses, and it is self-healing (subsequent lookups hit the fast path). Measured across the multi-trigger suite: 0 scans in normal flows (108 effect runs → 5 scans total, all in the rare custom-id / lost-trigger paths), ~1.4 iterations per scan, bounded by the number of registered triggers.Changes