From 9868e762f32df966b73e840f8c02cb640f04cabe Mon Sep 17 00:00:00 2001 From: Cindy Zhang Date: Thu, 13 Aug 2026 19:23:10 -0700 Subject: [PATCH] fix(selector): overlay the trigger only when the selected option covers it (#5004) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .changeset/selector-overlay-invariant.md | 7 ++++ packages/core/src/Selector/Selector.test.tsx | 43 +++++++++++++++++--- packages/core/src/Selector/Selector.tsx | 31 ++++++++++---- packages/core/src/Selector/hooks.ts | 43 ++++++++++++++++---- 4 files changed, 102 insertions(+), 22 deletions(-) create mode 100644 .changeset/selector-overlay-invariant.md diff --git a/.changeset/selector-overlay-invariant.md b/.changeset/selector-overlay-invariant.md new file mode 100644 index 0000000000000..63e87b8f2a560 --- /dev/null +++ b/.changeset/selector-overlay-invariant.md @@ -0,0 +1,7 @@ +--- +'@astryxdesign/core': patch +--- + +[fix] Selector: only overlay the menu on the trigger when the selected option is what lands there. With no selection, or when the viewport clamp slides a different option over the trigger, the menu now opens below with the standard clearance — a press on the trigger dismissed it instead of committing whatever was painted on top + +@cixzhang diff --git a/packages/core/src/Selector/Selector.test.tsx b/packages/core/src/Selector/Selector.test.tsx index 225aa2326bb28..f478e986a4997 100644 --- a/packages/core/src/Selector/Selector.test.tsx +++ b/packages/core/src/Selector/Selector.test.tsx @@ -379,7 +379,10 @@ describe('Selector', () => { ); }); - it('clamps the default selected-item overlay to the viewport', async () => { + it('opens below when the viewport clamp slides another option over the trigger', async () => { + // The default rects clamp the menu to a 200px viewport, which lands the + // selected option well above the trigger — the option covering it would be + // committed by a press meant to dismiss the menu (#5004). const restoreRects = mockSelectorRects(); const user = userEvent.setup(); try { @@ -395,12 +398,40 @@ describe('Selector', () => { await user.click(screen.getByRole('combobox')); const popover = screen .getByRole('listbox', {hidden: true}) - .closest('[popover]'); + .closest('[popover]') as HTMLElement; await waitFor(() => { - expect(popover?.getAttribute('style')).toContain( - 'margin-block-start: -110px', + expect(popover.style.getPropertyValue('--x-marginBlockStart')).toBe( + spacingVars['--spacing-1'], + ); + }); + expect(popover.getAttribute('style')).not.toContain( + 'margin-block-start: -', + ); + } finally { + restoreRects(); + } + }); + + it('opens below when nothing is selected', async () => { + const restoreRects = mockSelectorRects({viewportHeight: 800}); + const user = userEvent.setup(); + try { + render( + , + ); + + await user.click(screen.getByRole('combobox')); + const popover = screen + .getByRole('listbox', {hidden: true}) + .closest('[popover]') as HTMLElement; + await waitFor(() => { + expect(popover.style.getPropertyValue('--x-marginBlockStart')).toBe( + spacingVars['--spacing-1'], ); }); + expect(popover.getAttribute('style')).not.toContain( + 'margin-block-start: -', + ); } finally { restoreRects(); } @@ -563,7 +594,7 @@ describe('Selector', () => { }); it('stays flush in the default selected-item overlay', async () => { - const restoreRects = mockSelectorRects(); + const restoreRects = mockSelectorRects({viewportHeight: 800}); const user = userEvent.setup(); try { render( @@ -581,7 +612,7 @@ describe('Selector', () => { .closest('[popover]') as HTMLElement; await waitFor(() => { expect(popover.getAttribute('style')).toContain( - 'margin-block-start: -110px', + 'margin-block-start: -61px', ); }); expect(popover.style.getPropertyValue('--x-marginBlockStart')).toBe(''); diff --git a/packages/core/src/Selector/Selector.tsx b/packages/core/src/Selector/Selector.tsx index 3ad734a63072d..357087fdeff02 100644 --- a/packages/core/src/Selector/Selector.tsx +++ b/packages/core/src/Selector/Selector.tsx @@ -848,15 +848,28 @@ export function Selector( // Calculate offset to position selected item over trigger. Explicit // placement opts out of the selector-specific overlay behavior and uses the // standard layer positioning API instead. - const shouldOverlaySelectedItem = placement == null && !hasSearch; - const {offset: rawOffset, isPositioned: rawIsPositioned} = - useSelectedItemOffset({ - isOpen: popover.isOpen && shouldOverlaySelectedItem, - selectedItemIndex, - listboxId, - listboxRef, - anchorRef, - }); + // The overlay puts the menu on top of its own trigger, so whatever option is + // painted there is what a press on the trigger commits. It is only safe when + // that option is the selected one — the press re-commits the current value + // and reads as a dismissal. No selection means there is no such option, and + // the viewport clamp can slide a different one over the trigger (#5004); + // either way the menu opens below instead. + const canOverlaySelectedItem = + placement == null && !hasSearch && selectedItemIndex >= 0; + const { + offset: rawOffset, + isPositioned: rawIsPositioned, + isSelectedItemOverTrigger, + } = useSelectedItemOffset({ + isOpen: popover.isOpen && canOverlaySelectedItem, + selectedItemIndex, + listboxId, + listboxRef, + anchorRef, + }); + + const shouldOverlaySelectedItem = + canOverlaySelectedItem && isSelectedItemOverTrigger; const selectedItemOffset = shouldOverlaySelectedItem ? rawOffset : 0; const isPositioned = shouldOverlaySelectedItem ? rawIsPositioned : true; diff --git a/packages/core/src/Selector/hooks.ts b/packages/core/src/Selector/hooks.ts index f5f467c489a85..8ed0a03655166 100644 --- a/packages/core/src/Selector/hooks.ts +++ b/packages/core/src/Selector/hooks.ts @@ -19,6 +19,10 @@ import type {SelectorOptionData} from './types'; // the same mathematical center, so compensate before viewport clamping. const SELECTED_ITEM_OPTICAL_OFFSET = 1; +// The row is deliberately shifted by that optical pixel, so allow exactly that +// much slack before calling the trigger uncovered. +const COVERAGE_TOLERANCE = SELECTED_ITEM_OPTICAL_OFFSET; + /** * Return an element's document-relative layout top without CSS transforms. * getBoundingClientRect includes the popover's entry scale, which would make @@ -49,6 +53,13 @@ interface UseSelectedItemOffsetOptions { interface UseSelectedItemOffsetResult { offset: number; isPositioned: boolean; + /** + * Whether the selected option, at its final clamped position, still covers + * the trigger. False means the overlay's premise no longer holds — the + * option sitting over the trigger is some other option, so a press meant to + * dismiss the menu would commit it (#5004). + */ + isSelectedItemOverTrigger: boolean; } /** @@ -70,13 +81,21 @@ export function useSelectedItemOffset({ }: UseSelectedItemOffsetOptions): UseSelectedItemOffsetResult { const [offset, setOffset] = useState(0); const [isPositioned, setIsPositioned] = useState(false); + const [isSelectedItemOverTrigger, setIsSelectedItemOverTrigger] = + useState(true); const commitPosition = useCallback( - (nextOffset: number, nextIsPositioned: boolean) => { + ( + nextOffset: number, + nextIsPositioned: boolean, + nextIsSelectedItemOverTrigger: boolean, + ) => { // eslint-disable-next-line @eslint-react/set-state-in-effect -- selector popover position is derived from DOM layout setOffset(nextOffset); // eslint-disable-next-line @eslint-react/set-state-in-effect -- selector popover position is derived from DOM layout setIsPositioned(nextIsPositioned); + // eslint-disable-next-line @eslint-react/set-state-in-effect -- selector popover position is derived from DOM layout + setIsSelectedItemOverTrigger(nextIsSelectedItemOverTrigger); }, [], ); @@ -84,12 +103,12 @@ export function useSelectedItemOffset({ useIsomorphicLayoutEffect(() => { if (!isOpen) { // Reset offset when closed - commitPosition(0, false); + commitPosition(0, false, true); return; } if (!listboxRef.current || !anchorRef.current) { - commitPosition(0, true); + commitPosition(0, true, true); return; } @@ -100,7 +119,7 @@ export function useSelectedItemOffset({ const targetItem = document.getElementById(targetItemId); if (!targetItem) { - commitPosition(0, true); + commitPosition(0, true, true); return; } @@ -110,7 +129,7 @@ export function useSelectedItemOffset({ // offset* metrics intentionally exclude the popover's entry transform. const listboxHeight = listbox.offsetHeight; if (listboxHeight <= 0) { - commitPosition(0, true); + commitPosition(0, true, true); return; } @@ -136,7 +155,17 @@ export function useSelectedItemOffset({ // anchorRect.bottom to clampedTop. const clampedOffset = Math.max(0, anchorRect.bottom - clampedTop); - commitPosition(clampedOffset, true); + // Where the target row actually lands once the menu is clamped. The clamp + // can slide the list far enough that a different option sits over the + // trigger, and that option is what a press on the trigger commits (#5004). + const targetItemTop = + clampedTop + itemCenterInListbox - targetItem.offsetHeight / 2; + const targetItemBottom = targetItemTop + targetItem.offsetHeight; + const isSelectedItemOverTrigger = + targetItemTop - COVERAGE_TOLERANCE <= anchorRect.top && + targetItemBottom + COVERAGE_TOLERANCE >= anchorRect.bottom; + + commitPosition(clampedOffset, true, isSelectedItemOverTrigger); }, [ isOpen, selectedItemIndex, @@ -146,7 +175,7 @@ export function useSelectedItemOffset({ commitPosition, ]); - return {offset, isPositioned}; + return {offset, isPositioned, isSelectedItemOverTrigger}; } // =============================================================================