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}; } // =============================================================================