Repository navigation
fix(HoverCard,Tooltip): wait for the trigger to have a layout box before showing - #5398
HelloOjasMutreja wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
PR Analysis Report🔎 Preview ·
|
a413d10 to
39af550
Compare
39af550 to
76bdb39
Compare
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Thanks—HoverCard now anchors correctly, but Tooltip still leaves the user’s hint detached at the viewport corner, and this head also makes the UI suite red.
packages/core/src/Layer/showWhenAnchored.ts:27— In Chromium, the controlled Tooltip-in-Dialog story still opens at(0, 4)while its trigger is around(206, 298). The same exact-head run centers HoverCard correctly. Please keep trigger readiness in Layer’s existing lifecycle owner rather than adding a caller-level timing abstraction, and cover both controlled paths in a real-browser regression.internal/test-utils/src/setup.ts:115— This global ResizeObserver stub changes unrelated TabList behavior: the exact-head test callsscrollBytwice; current main passes 74/74, and the head also passes when only this setup change is removed. Please keep the test double local to the affected tests.
If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
76bdb39 to
6e4d9f5
Compare
|
Moved trigger-readiness into Tracing this live in Chromium turned up more than the box-readiness gap alone. HoverCard's own controlled-in-Dialog story was passing by coincidence, not because its readiness handling differed from Tooltip's: with the trigger's box confirmed present, Past that, real-Chromium tracing surfaced a second issue: even with a real box, showing a layer while an ancestor's own CSS entry animation (Dialog's scale/translate transition) is still running anchors against a mid-transform position that also never gets revisited, since a transform changes nothing On the Added Also verified: full |
b695d9a to
9ca8b36
Compare
|
Rebased onto current main. One real conflict in playwright.config.ts: main's own Verified after rebase: typecheck clean, and the affected suites (Popover, PowerSearch, Tokenizer, Layer, Tooltip, HoverCard) pass, isolated and in a narrower batch. A full combined run of all six directories intermittently worker-crashes or shows a single unrelated PowerSearchFilterEditor failure in this environment; reproduces identically on a clean, unmodified checkout of current main with the same broad batch, so it is local Vitest worker-pool flakiness under a large parallel run here, not something this PR introduced. |
|
Checked the pr-a11y failure on this run: it's in Checkbox.a11y.chromium.spec.ts (a menu-item inventory mismatch, "Show archived" showing up as unexpectedly inert), a file this PR doesn't touch and unrelated to HoverCard/Tooltip anchoring. Not something this diff caused. |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for moving anchor readiness into Layer and adding the real-Chromium Tooltip/HoverCard regression. A few current paths still need correction before this is ready:
show()waits for every trigger or ancestor animation to finish, so an infinitely pulsing trigger such asStatusDotcan prevent its Tooltip from opening forever. Bound the wait to finite entry motion and cover a continuous-animation trigger.- The delayed path switches from
getBoundingClientRect()toResizeObserverEntry.contentRect. Text-only Tooltip/HoverCard triggers are inline spans, whose observed content box can remain empty, so a controlled/default-open layer can stay closed. Use a readiness signal that covers inline triggers and add that browser case. contextRef(null)forgets the old trigger without removing this layer's anchor name. When an external anchor remains mounted or the ref moves, both old and new elements can keep the same association. Preserve transient same-element churn while cleaning a real detach/rebind.- The changeset still names the deleted
showWhenAnchoredhelper; update it to describe the current Layer implementation. - The current Layer contract requires real Chromium and WebKit evidence for anchor-geometry changes, but this regression is Chromium-only. Add the equivalent WebKit proof.
[Automated review]
|
Both problems are addressed in the latest push.
Not verified here: I cannot run Storybook or the browser specs on this machine ( |
There was a problem hiding this comment.
Thanks — the pinned Tooltip and HoverCard in a Dialog now anchor to their trigger in Chromium and WebKit, and the trigger-swap restart is fixed, but a few things still need changing before this can merge:
isAnchorReady()only counts a trigger as ready when its closest<dialog>isopen, which a non-modalDrawer(hasScrim={false}, shown withshowPopover()) never is, so the drawer's close-button tooltip, aDropdownMenuor aPopoverinside it no longer open; please also accept a dialog that is showing as a popover, and add a test with a trigger inside a non-modal Drawer.- The unbounded
requestAnimationFramepoll inshow()conflicts with the current Layer runtime rule that "the runtime polls nothing" (docs/architecture/layer-runtime.mdINV12, alsodocs/specs/AST-059-layer-viewport-inset/spec.mdFR8); please replace it with observer- or event-driven signals. - The branch conflicts with
mainincontextRef, wheremainnow moves an open layer's anchor observers to a replaced trigger, and CI has not run on this head; please rebase and keep both behaviors.
[Automated review]
| return false; | ||
| } | ||
| const dialog = anchor.closest('dialog'); | ||
| return dialog == null || dialog.open; |
There was a problem hiding this comment.
dialog.open stays false for a <dialog popover> shown with showPopover(), and the non-modal Drawer (hasScrim={false}) is one, so a trigger inside it never counts as ready: in Chromium and WebKit its close-button tooltip, a DropdownMenu and a Popover stay closed, and the frame poll runs until hide(). Counting a dialog that is showing as a popover as open (for example dialog.open || dialog.matches(':popover-open')), with a test that puts a trigger inside a non-modal Drawer, would fix it.
| const poll = () => { | ||
| if (isAnchorReady(anchor)) { | ||
| resolve(); | ||
| return; | ||
| } | ||
| state.rafHandle = requestAnimationFrame(poll); | ||
| }; | ||
| state.rafHandle = requestAnimationFrame(poll); |
There was a problem hiding this comment.
This loop polls every frame, which the current Layer runtime record rules out: docs/architecture/layer-runtime.md INV12 says "the runtime polls nothing" (as does docs/specs/AST-059-layer-viewport-inset/spec.md FR8). With PinnedTooltipInModal loaded and its Dialog still closed, it runs 60 callbacks a second in Chromium, against none on the base branch. An intersection observer on the trigger and a mutation observer on the dialog's open attribute could carry the inline-trigger and not-yet-modal cases instead.
|
|
||
| `Dialog` mounts its children before it opens (`isOpen` gates `showModal()`, not rendering), so a `HoverCard` or `Tooltip` inside one with a controlled `isOpen={true}` runs its show effect while the trigger sits in a still non-modal `<dialog>` and has no box. A trigger with no box is not a valid CSS anchor: the popover resolves to its fallback position in the viewport corner, and unlike a live layout change that resolution is not revisited once the trigger later gets a box. | ||
|
|
||
| `Layer`'s `show()` now opens immediately when the trigger already has a box, so a menu or popover still opens in the same task as the press that asked for it. Only a boxless trigger waits: first for it to get a layout box, then for any running, finite animation on an ancestor to finish, so the layer anchors to the trigger's settled position and not to a mid-transition one. Paused animations and infinite ones are not waited on. |
There was a problem hiding this comment.
A trigger that has a box but sits in a <dialog> that is not open also waits now, so "Only a boxless trigger waits" no longer describes the change.
|
Reworked and rebased onto current main (one commit, 2741421):
Verification: |
dfab0a9 to
2741421
Compare
|
On the failing |
There was a problem hiding this comment.
Thanks — the pinned Tooltip and HoverCard in a Dialog now anchor to their trigger in Chromium and WebKit, layers inside a non-modal Drawer open again, and the wait no longer polls, but two things still need changing before this can merge:
isAnchorReady()treats any 0×0 trigger as boxless, so a trigger rendered with an empty box never opens: a ContextMenu in a shadow root without Astryx's styles stays closed on right-click, which is whypr-a11yfails bothMenuPressShadowRootContextMenu tests on this head, and checking for no layout box at all (for exampleanchor.getClientRects().length === 0) keeps every case this PR fixes.- The
SheetWithHoverTipstory moves back to the deprecatedBottomSheetSwitcher, undoing main's migration of it, so please restore main's version of that story.
[Automated review]
|
|
||
| function isAnchorReady(anchor: HTMLElement): boolean { | ||
| const rect = anchor.getBoundingClientRect(); | ||
| if (rect.width === 0 && rect.height === 0) { |
There was a problem hiding this comment.
A trigger that is rendered but has an empty box also lands here and never becomes ready: the ContextMenu's cursor anchor inside a shadow root without Astryx's styles is an empty inline <span> (one client rect, 0×0), so no observer or dialog signal ever reports it and the menu never opens. That is what fails the two MenuPressShadowRoot ContextMenu cases in pr-a11y on this head; base opens the menu. Testing for no layout box at all, for example anchor.getClientRects().length === 0 (which a display: none trigger and one inside a closed <dialog> both return), would keep every case this PR fixes.
| </Tooltip> | ||
| </VStack> | ||
| </BottomSheet> | ||
| <BottomSheetSwitcher |
There was a problem hiding this comment.
This brings back the deprecated BottomSheetSwitcher that main's #7648 migrated this story away from, and nothing in the anchoring fix needs it; the BottomSheetSwitcher record says stories teach the standalone BottomSheet replacement. Could this hunk and the import on line 33 go back to main's version?
|
|
||
| `Dialog` mounts its children before it opens (`isOpen` gates `showModal()`, not rendering), so a `HoverCard` or `Tooltip` inside one with a controlled `isOpen={true}` runs its show effect while the trigger sits in a still non-modal `<dialog>` and has no box. A trigger with no box is not a valid CSS anchor: the popover resolves to its fallback position in the viewport corner, and unlike a live layout change that resolution is not revisited once the trigger later gets a box. | ||
|
|
||
| `Layer`'s `show()` now opens immediately when the trigger already has a box, so a menu or popover still opens in the same task as the press that asked for it. Only a boxless trigger waits: first for it to get a layout box, then for any running, finite animation on an ancestor to finish, so the layer anchors to the trigger's settled position and not to a mid-transition one. Paused animations and infinite ones are not waited on. |
There was a problem hiding this comment.
A trigger with a box inside a dialog that is not showing waits too (line 11 says so), so "Only a boxless trigger waits" undersells the change.
2741421 to
ab380fb
Compare
|
Rebased onto main and pushed ab380fb with all three points:
I ran the built Storybook in Chromium: the three |
There was a problem hiding this comment.
Thanks — at this head the pinned Tooltip and HoverCard in a Dialog anchor to their trigger in Chromium and WebKit, a ContextMenu whose cursor anchor has an empty box opens again, and the SheetWithHoverTip story is back to main's version, so this review found nothing left for you to change. Approval still waits on cixzhang's earlier change request, which stays open until cixzhang re-reviews this head or dismisses it; both of its points are now addressed. CI has not run on this head yet because it needs a maintainer's ci:run label.
[Automated review]
|
The last bot review found nothing left to change on this head, and main is merged in again (the Layer code moved into |
There was a problem hiding this comment.
Thanks: at this head the controlled Tooltip and HoverCard in a Dialog anchor to their trigger in Chromium and WebKit, and a rendered trigger still opens in the same task, but one change is still needed. The anchor wait in useLayer's show() creates its own ResizeObserver (line 1645) and MutationObserver (line 1652), while Core already owns both concerns in observeResize (utils/sharedResizeObserver.ts) and observeMutations (utils/sharedMutationObserver.ts), which architecture:react-component-runtime INV16 requires callers to use. Please route the wait through those two utilities (the IntersectionObserver and the dialog toggle listener can stay), check the useLayer tests that count ResizeObserver instances, and refresh the description, which still describes the removed requestAnimationFrame poll. Merging will also need cixzhang's earlier change request to be re-reviewed or dismissed; both of its points hold at this head.
[Automated review]
| const observer = new ResizeObserver(check); | ||
| const intersection = | ||
| typeof IntersectionObserver === 'undefined' | ||
| ? null | ||
| : new IntersectionObserver(check); | ||
| const mutations = | ||
| dialog && typeof MutationObserver !== 'undefined' | ||
| ? new MutationObserver(check) | ||
| : null; |
There was a problem hiding this comment.
Core already owns both of these concerns: observeResize (utils/sharedResizeObserver.ts) and observeMutations (utils/sharedMutationObserver.ts), and architecture:react-component-runtime INV16 asks callers to use them rather than build their own observer. Something like const stopResize = observeResize(anchor, check) and const stopDialog = dialog ? observeMutations(dialog, {attributes: true, attributeFilter: ['open']}, check) : null, both called from release(), keeps one observation path. observeResize calls check once synchronously, which is harmless here. The IntersectionObserver and the toggle listener can stay.
|
Thanks, routed. The anchor wait in Checked locally: the Layer, Tooltip and HoverCard suites pass (380 of 381; the one failure is |
…ithHoverTip story
…n observers The anchor wait in show() created its own ResizeObserver and MutationObserver. Route it through observeResize and observeMutations, which architecture:react-component-runtime INV16 requires callers to use. The IntersectionObserver and the dialog toggle listener stay.
The harness attached the trigger with an inline ref, which detaches and reattaches it on every render. Each detach now defers its anchor-name removal by a microtask, so the replay test, which holds the ledger's microtasks by hand, held those too. A stable ref leaves it holding only the ledger's.
8c83f74 to
e2d6bf0
Compare
|
Rebased onto main: the Layer code had moved again (the popover-operation helpers are in One test change outside my own: the focus-return harness in The Layer, Tooltip and HoverCard suites pass (495 of 496; the one failure is |
There was a problem hiding this comment.
Thanks: at this head the controlled Tooltip and HoverCard in a Dialog anchor to their trigger in Chromium and WebKit, and the wait now uses Core's shared observers. One change is still needed: the branch fails the repository's strict lint, which CI runs once the ci:run label is added, with 8 errors from ASTRYX_STRICT_LINT=1 pnpm lint:
useLayer.test.tsxline 1229: the newuseCallbackreadspopoverandtooltipbut lists only theirrefs; destructure the two refs into locals and list those.useLayer.tsxlines 895, 1638, 1647, 1660, 1767 and 1800: the new comments cite an issue number or a spec claim (#5398,INV12,INV16,INV6); keeping only the reason also clears the two older sites the lint then reports at lines 1828 and 2121.
The description still describes the removed requestAnimationFrame poll, so a refresh would help reviewers, and merging will also need cixzhang's earlier change request to be re-reviewed or dismissed; both of its points hold at this head.
[Automated review]
| popover.ref(el); | ||
| tooltip.ref(el); | ||
| }, | ||
| [popover.ref, tooltip.ref], |
There was a problem hiding this comment.
Strict lint, which CI runs, fails here: @eslint-react/exhaustive-deps reports that this useCallback reads popover and tooltip. Destructuring the refs keeps the callback stable and satisfies the rule:
const {ref: popoverRef} = popover;
const {ref: tooltipRef} = tooltip;
const triggerRef = useCallback(
(el: HTMLButtonElement | null) => {
popoverRef(el);
tooltipRef(el);
},
[popoverRef, tooltipRef],
);| openNow(); | ||
| return; | ||
| } | ||
| // The wait is driven by signals, never by polling (INV12: the runtime |
There was a problem hiding this comment.
Strict lint fails on the provenance in the new comments: #5398 at lines 895, 1647 and 1767, INV12 here, INV16 at line 1660 and INV6 at line 1800. These, and this 18-line block that cites a claim, use up the file's counted baseline, so the older sites at lines 1828 and 2121 report too. Keeping only the reason in each comment clears all eight errors.
| * a later Effect, so in that window the trigger already has a box while the | ||
| * dialog is not yet in the top layer; opening a layer then paints it before | ||
| * the dialog is modal. | ||
| */ |
There was a problem hiding this comment.
Non-blocking: the JSDoc above describes isAnchorReady but sits on isShownAsPopover. Moving it to isAnchorReady (line 867) and shortening it would help, for example: "A valid anchor has a layout box, and its <dialog>, if any, is showing. A non-modal Drawer is shown as a popover and is never open."
…mments and test harness The new comments no longer cite an issue number or an invariant id, and the focus-return harness lists the two refs it reads in its callback's dependencies.
|
Pushed b4bad83 for the strict-lint findings. The five new comments in On @cixzhang's earlier change request, as far as I can tell both points are addressed at this head, but I would like a re-review to confirm rather than assume:
|
There was a problem hiding this comment.
Thanks: at this head the strict-lint findings are fixed, and the controlled Tooltip and HoverCard in a Dialog anchor to their trigger in Chromium and WebKit, so this review found nothing left for you to change. Approval still waits on cixzhang's earlier change request, which stays open until cixzhang re-reviews this head or dismisses it; both of its points are addressed here. CI has not run on this head yet because it needs a maintainer's ci:run label. The inline notes on useLayer.tsx are optional comment tidy-ups.
[Automated review]
| pendingAnchorElementRef.current = null; | ||
| }, []); | ||
|
|
||
| // A trigger with no box — 0x0, as every element not yet laid out reports — |
There was a problem hiding this comment.
Non-blocking: isAnchorReady treats an empty 0×0 box as ready, so "0x0" here (and at line 1566) no longer describes what waits, and line 1606 says a trigger in a not-yet-modal <dialog> has no box while isAnchorReady's comment says it already has one. Something like "A trigger with no layout box, or one in a <dialog> that is not showing, is not a valid CSS anchor." covers both cases.
| * Only a FINITE animation is waited on. `Animation.finished` never resolves | ||
| * for one with `iterationCount: Infinity` (it never reaches the finished play | ||
| * state), so a continuously-animating ancestor — `StatusDot`'s pulse, e.g. — | ||
| * would otherwise block a layer that anchors to it from ever opening. |
There was a problem hiding this comment.
Non-blocking: the blank line after this JSDoc detaches it from waitForAncestorAnimations, and the text can be much shorter, for example: "A transform moves the anchor without a resize. Wait for running, finite ancestor animations to end. Read the list one frame late: a just-started CSS animation is not in it yet."
| // A caller's OWN ref to the trigger can churn identity for reasons that | ||
| // have nothing to do with the trigger itself changing (e.g. Tooltip | ||
| // imperatively re-invoking a combined ref callback whenever one of its | ||
| // interaction handlers changes identity) — React or the caller then calls | ||
| // this ref with `null` and immediately after with the very same element. | ||
| // Removing `anchor-name` on that `null` call and re-adding it on the | ||
| // reattach is a real DOM mutation each time, and an already-open layer's | ||
| // `position-try-fallbacks` resolution does not automatically get revisited | ||
| // once the anchor comes back: the layer is left stuck at the unanchored | ||
| // fallback position for the rest of that open, even though the | ||
| // trigger never actually left the document. | ||
| // | ||
| // `addAnchorName` is already idempotent (skips the DOM write if the name | ||
| // is already present), so a `null` call defers the removal instead of | ||
| // applying it immediately: `triggerRef.current` is left pointing at the | ||
| // stale element, and a microtask is queued to actually remove its anchor | ||
| // name — but only if nothing has reattached to that exact element by | ||
| // then. A transient null-then-same-element churn cancels the pending | ||
| // removal on the reattach, so the anchor name (and the layer's anchor | ||
| // resolution) is never disturbed. A `null` followed by a DIFFERENT real | ||
| // element still removes the old element's name immediately, in the same | ||
| // synchronous call that adds the new one — no need to wait, since that is | ||
| // a genuine reassignment, not churn. A `null` followed by nothing (a real | ||
| // unmount) lets the microtask's removal go through, so a detached trigger | ||
| // does not keep carrying a dead anchor name forever (the current Popover | ||
| // contract: detaching the trigger removes its anchor association). |
There was a problem hiding this comment.
Non-blocking: three sentences keep the reason this block gives: "A combined ref can call this ref with null and then with the same element. Removing anchor-name in that gap strands an open layer at its fallback position. So a null call removes the name one microtask later, unless the same element attaches again first."
Fixes #5374.
Problem
Dialogmounts its children before it opens:isOpengates the<dialog>'s ownshowModal(), not whether{children}renders. So aHoverCardorTooltipinside one with a controlledisOpen={true}(orisDefaultOpen) runs its show effect while the trigger sits in a still non-modal<dialog>and has no layout box (0x0).A trigger with no box is not a valid CSS anchor.
position-anchorresolves to its fallback position in the viewport corner, and unlike a live layout change that resolution is not revisited once the trigger later gets a box. I measured the card staying at(0, 4)for more than 3 seconds after the dialog visibly opened, so this is a permanently invalid anchor, not a one-frame race. ArequestAnimationFramedefer and asetTimeout(50)defer both failed to fix it, which points at the missing box itself being the precondition.Fix
All in
useLayer'sshow(), the one place every context-mode open goes through:ResizeObserverentry (a block-level trigger gaining a box), on anIntersectionObserverreport (which reaches adisplay: inlinetrigger, such as a text-only trigger's wrapper<span>, where aResizeObserverdelivers no entry), and on the enclosing<dialog>'sopenattribute andtoggleevent. These use Core's shared observers, and each registration is released on its own. There is no time limit: the wait ends when the trigger gets a box or the open is cancelled.ResizeObserver, norequestAnimationFrame, or no Web Animations API, as in jsdom) the layer opens immediately.Separately, a transient
nullthen same-element call on the trigger ref (what a combined ref utility makes when a handler changes identity) no longer removes and re-adds the element'sanchor-name. That churn could also strand an open layer in the corner.What changed after review
The first version waited at least a frame on every open, including for a trigger that already had a box. That moved a DropdownMenu's open from press to release, failed the
Core/Layerviewport-inset stories (no open layer right after the click) and the DateInput and DateTimeInput FR10 suites (their no-opResizeObserverstubs never ended the wait). It also had these gaps, all fixed here:ResizeObserverleft the settle wait uncancellable byhide(), because the final cancel function overwrote its handle;ResizeObservermocks I had edited in Popover, PowerSearch and Tokenizer tests, theclearTimeoutshoist inuseTooltip, and the Playwright glob are all reverted, since they only existed to accommodate the always-wait version. This PR no longer touches any of those files.Since the last review the unbounded
requestAnimationFramepoll is gone (it is replaced by the signals above), the branch is rebased on currentmain, and the new comments and test harness passASTRYX_STRICT_LINT=1 pnpm lint.Verification
Reproduced against the exact story from the issue (
Core/Layer Dismissal,Pinned Hover Card In Modal), where the Dialog opens later via a click.Before, card stuck in the corner, disconnected from its trigger:
After, anchored above "Card trigger", matching
placement="above":Those two images are from the earlier version of this fix; the visible outcome for a boxless trigger is unchanged.
Tests (jsdom, with layout stubbed): eight new
show()readiness tests inuseLayer.test.tsx. I broke the code four ways to confirm each is caught: waiting on the box path again fails the same-task open test, dropping the running-only filter fails the paused-animation test, removing the trigger restart fails the replacement test, and a cancel that skips the settle wait fails thehide()test. The wholeuseLayerfile passes (78 tests), as do the Layer, Popover, Tooltip, HoverCard, DropdownMenu, DateInput, DateTimeInput, DateRangeInput, PowerSearch, Tokenizer, Selector and MultiSelector suites, apart from one failure that is also onmain. That one islayerViewportInset.test.ts, which compares Windows backslash paths to forward-slash names. Core typecheck, eslint and prettier are clean.Limits
I could not run the browser parts here: the Storybook build and dev server crash on my machine with a native out-of-memory error. So the
Core/Layerstory play-guards, theanchorReadinessPlaywright spec and the WebKit run are not verified by me, and the "after" image above is from the earlier version. TheCore/Layerand DateInput FR10 failures from the last review should be gone by construction, because a trigger with a box now opens synchronously and jsdom without the Web Animations API no longer waits, but that is for CI to confirm.