fix(selector): overlay the trigger only when the selected option covers it (#5004)

.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

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(
+        <Selector label="Fruit" options={OPTIONS} placeholder="Pick one" />,
+      );
+
+      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('');

packages/core/src/Selector/Selector.tsx

@@ -848,15 +848,28 @@ export function Selector<T extends SelectorOptionType>(
   // 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;

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,26 +81,34 @@ 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);
     },
     [],
   );
 
   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};
 }
 
 // =============================================================================