fix(ui): harden the Astryx floating kernel (PR 5 review round 2) - #1672
Merged
Conversation
…ard activation The per-gesture guard cleared its flag only inside a completed click, so a press that light-dismissed the popover and then ended off the trigger (drag off, pointercancel) left the flag set — and the next keyboard Enter/Space, which has no paired pointerdown, was silently swallowed. Judge only pointer-sourced clicks (event.detail > 0) and clear the flag before the defaultPrevented early return. Found independently by both round-2 reviews. The e2e journey pins it: aborted press dismisses, then Enter (detail 0) must still open. Red-verified against the pre-fix build.
The Base UI trigger closed the tooltip on activation (closeOnClick default); the Astryx adapter dropped that, so clicking a chrome icon button left its hint floating under the pointer. Astryx's useTooltip exposes no hide(), so the trigger goes through the primitive's own seams: hidePopover() on the layer element (its toggle listener syncs React state back) plus a synthetic mouseleave that cancels a show still pending its delay. The trigger's click handler now also composes render-element and call-site onClick, matching the Base UI mergeProps behavior. Known residual, recorded: Astryx exposes no way to also suppress the follow-up focus-show path a keyboard activation could schedule; hover paths are fully covered. E2E pins the synchronous dismissal with a one-shot check (every other hide path is delayed >=100ms); red-verified pre-fix.
When the open prop contradicts the layer (open held true + Escape), the reconcile effect's show()/hide() re-fired onOpenChange — reporting the parent's own instruction back as if it were a second user request. Silence the request callback during prop-driven reconcile (useLayer invokes it synchronously inside show()/hide(), so a flag around the calls is exact); onOpenChangeComplete keeps firing because it reports lifecycle, not intent, and TimePicker keys its settled state on it.
The initialFocus fallback selector could pick a disabled control, making focus() a silent no-op. Mirror the exclusion Astryx's own FOCUSABLE_SELECTOR applies (not exported from the package, hence the local declaration).
The header claimed useFocusTrap's restore effect was the sole focus-return authority; per the HTML spec the show-popover algorithm records the previously focused element even for imperative showPopover(), and hidePopover() returns focus natively. The trap restore is the backstop for the paths the browser skips (light dismiss). Behavior unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refs #1565. PR #1665 merged while its second review round was still landing; these are the adjudicated round-2 fixes (Codex fix-verification pass + an independent fresh-eye pass, both PASS with no P0/P1), cherry-picked onto current
maincontent-identical to the validated tree.event.detail > 0) and clears before thedefaultPreventedearly return.closeOnClickdefault); the Astryx adapter dropped that. Restored through Astryx's own seams —hidePopover()on the layer element (itstogglelistener syncs state back) plus a syntheticmouseleavethat cancels a show still pending its delay.onOpenChange: withopenheld and the layer moving natively (Escape), the reconcile effect re-reported the parent's own instruction as a new request. Prop-driven reconcile is now silent ononOpenChange;onOpenChangeCompletestill fires (lifecycle, and TimePicker keys its settled state on it).useFocusTrapas backstop.Verification
@maka/ui: typecheck clean, unit 280/280, Biome format:check clean (all on this branch, post-refactor(ui): migrate Markdown core to Astryx #1668 base).floating-layerse2e on this branch: 2/2, including the two new behavior pins (aborted-press → Enter reopens; one-shot tooltip-hidden right after trigger click). Both pins were red-verified against a pre-fix build (each fails at exactly the guarded assertion) before landing.Review focus
The tooltip dismissal goes through native
hidePopover()+ a syntheticmouseleavebecause Astryx'suseTooltipexposes nohide(); if upstream grows one, the adapter should switch to it.