fix(selector): overlay the trigger only when the selected option covers it (#5004) - #5010
fix(selector): overlay the trigger only when the selected option covers it (#5004)#5010cixzhang wants to merge 1 commit into
Conversation
…rs it (#5004) The selected-item overlay puts the menu on top of its own trigger, so the option painted there is what a press on the trigger commits. That is only harmless while the option under the trigger is the selected one. With no selection the first option takes that spot, and the viewport clamp can slide an unrelated option there — in both cases a press meant to dismiss the menu silently changes the value. Fall back to the standard below placement whenever the invariant does not hold, which also restores click-to-close for those states.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsSelector (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
|
Parking this: the reported symptom is the trigger press closing and immediately reopening the popup — a light-dismiss/click race, not the overlay geometry — and that is fixed in #5018, which also stops the clear and status buttons on a trigger from dismissing the popup they belong to. The geometry finding here still stands and is documented in #5004: with the selected-item overlay, the option painted over the trigger is what a press on the trigger commits, so a placeholder Selector can be given a value by someone trying to dismiss the menu. Where that lands depends on the default-placement decision in #4976, so it should not be settled by a drive-by fix from me. The branch stays if we want it back. |
1 participant
Closes #5004.
The bug
The selected-item overlay paints the menu on top of its own trigger, so whichever option lands over the trigger is what a press on the trigger commits. That is harmless in exactly one case — when the option sitting there is the one already selected, the press re-commits the current value and reads as a dismissal (this is what a native
<select>does).Two states break that invariant, and both were live. Measured in Chrome 151 against
main, pressing the trigger of an open menu at its own coordinates:ApplePick one…→AppleOption 31Option 38→Option 31BananaBanana→Banana✅Identical with mouse and with touch. The second row is the worse one: an existing value is silently replaced by a different one.
It is geometry, not a popover race
Worth stating because it is the obvious suspect. Instrumenting the second press:
clickat the trigger, then the light-dismisstoggle15 ms later — the toggle always arrives after the click, so the "closed by light dismiss, then re-opened by the click" race that DropdownMenu and Popover guard with a 50 ms window never fires here. Nothing in the timing needs fixing.The rule
Use the overlay only when the option under the trigger is the selected one. Nothing selected means no such option; a viewport clamp that slides the list can put a different one there. In both cases the menu opens below with the standard
--spacing-1clearance — the same geometry every other Astryx menu uses, and the trigger stays uncovered, so click-to-close works.useSelectedItemOffsetnow reports whether the target row still covers the anchor after clamping (it already computed everything needed); the tolerance is the one deliberate optical pixel the row is shifted by. Where the invariant holds, the measured offset is unchanged.How this sits with #4976
Orthogonal, and this does not pre-empt that decision. #4976 decides what the default placement should be; this fixes the overlay itself, which survives there as the opt-in
hasSelectedItemOverlay— carrying both failures with it. Whichever lands first, the other rebases onto the same few lines.Risk
Placeholder-state Selectors — the state every form starts in — now open below with a gap instead of over the trigger. That is a visible change, and a subset of the one #4976 proposes for all Selectors. Clamped menus open below rather than pinning to the viewport edge.
useSelectedItemOffsetis exported from the package; its return type gains a field, which is additive.Adjacent, not fixed here
[396, 428]against the selected option's[395, 427]— that bottom pixel row hits the next option. It comes from the deliberate 1 px optical offset, so straightening it is a visual-alignment call, not a geometry bug.Test plan
Unit — two new tests, both written red-first (they fail on unmodified
Selector.tsx/hooks.ts): no selection, and a clamp that slides another option over the trigger. The overlay geometry tests were asserting the clamped case as correct behavior, so they now run against rects where the alignment actually lands (-61px), which is what they were meant to cover.packages/core/src/Selector: 120 passing. Fullpackages/core: 6310 passing — the one failure is the Table 500-row perf budget, which flakes under full parallel load on this laptop and passes in isolation.Real Chromium (Playwright against local Storybook, mouse and touch, pressing the trigger of an open menu at its own coordinates):
AppleOption 31placement="above"/hasSearch/ MultiSelectorRepo —
tsc --noEmitclean, strict ESLint clean,pnpm check:repogreen.