From beaba20573b2bbe7a642cdf92686325a0e238580 Mon Sep 17 00:00:00 2001 From: AK <144495202+AKnassa@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:24:29 -0400 Subject: [PATCH] feat(selector): open the menu below by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The selected-item overlay — the open menu pulled up so the selected option sits over the trigger, native-select style — is no longer the default. Selector now opens below with the standard --spacing-1 clearance, the same recipe DropdownMenu, MultiSelector, and ComplexSelector already use; search mode gains the clearance too (it used to sit flush). The overlay survives as opt-in hasSelectedItemOverlay, and only engages on below placement — an explicit above/start/end still wins, as before. Why the flip, measured rather than assumed: - The trigger keeps DOM focus while the popup is open and the overlay covers it entirely — SC 2.4.11 (AA) focus-obscured territory. - The overlay quietly degenerates: with the selected option past the listbox fold the menu pins to the viewport top (the measured margin equals triggerRect.bottom), and the no-value placeholder state that every form starts in still pulls the menu over its own trigger. - It was already only half the default: hasSearch switches to plain below, MultiSelector hard-codes below, and every other menu-shaped popup in core opens below. placement was also undocumented everywhere that matters — absent from Selector.doc.mjs, so missing from the docsite props table and from astryx component Selector --dense. Both it (@default 'below') and the new prop are documented now. Mechanically small: overlay mode was already below placement plus a measured negative margin-block-start, so the flip removes the margin and adds the offset; useSelectedItemOffset is untouched and stays public API. The clearance is gated off in overlay mode — offset sets both block edges (flip-safe per #4803) while the overlay margin is inline, and mixing them would offset the far edge asymmetrically. Verified: 3 new tests red against the untouched tree, then green; 113 pass in Selector.test.tsx; full suite 9887 pass with only the two pre-existing macOS cli-casing failures from the recorded baseline; docsite 372 pass; core tsc, docsite and storybook typecheck, CI=true lint, check:repo (changeset included), and prettier all clean. Storybook screenshots confirm both modes. Changeset: [breaking] minor per the 0.x convention. To restore the old look on an instance, add hasSelectedItemOverlay. Refs #4227 --- .../selector-default-placement-below.md | 9 ++ apps/storybook/stories/Selector.stories.tsx | 30 ++++- packages/core/src/Selector/Selector.doc.mjs | 19 ++- packages/core/src/Selector/Selector.test.tsx | 114 +++++++++++++++++- packages/core/src/Selector/Selector.tsx | 36 ++++-- packages/core/src/Selector/hooks.ts | 6 +- 6 files changed, 196 insertions(+), 18 deletions(-) create mode 100644 .changeset/selector-default-placement-below.md diff --git a/.changeset/selector-default-placement-below.md b/.changeset/selector-default-placement-below.md new file mode 100644 index 000000000000..b4f77a0262eb --- /dev/null +++ b/.changeset/selector-default-placement-below.md @@ -0,0 +1,9 @@ +--- +'@astryxdesign/core': minor +--- + +[breaking] Selector menus open below the trigger by default (#4227). The old native-select behavior — the open menu overlaying the trigger with the selected option pinned over it — is now opt-in via `hasSelectedItemOverlay`. `placement` keeps working as before and now documents its `'below'` default; Selector menus also gain the standard `--spacing-1` clearance that DropdownMenu, MultiSelector, and ComplexSelector already use (search mode included, which used to sit flush). + +Why the flip: the overlay covered the trigger while it kept DOM focus (WCAG 2.4.11 focus-obscured territory), quietly degenerated once the selected option sat past the listbox fold (the menu pinned to the top of the viewport, including in the no-value placeholder state every form starts in), and made default Selector the odd one out — MultiSelector, ComplexSelector, DropdownMenu, and Selector-with-hasSearch all open below already. To restore the previous look on a given instance, add `hasSelectedItemOverlay`. + +@AKnassa diff --git a/apps/storybook/stories/Selector.stories.tsx b/apps/storybook/stories/Selector.stories.tsx index b86630df5898..17415b6fadf7 100644 --- a/apps/storybook/stories/Selector.stories.tsx +++ b/apps/storybook/stories/Selector.stories.tsx @@ -62,7 +62,12 @@ const meta: Meta = { control: 'select', options: ['above', 'below', 'start', 'end'], description: - 'Explicit menu placement. Leave unset for selected-item overlay behavior.', + 'Menu placement relative to the trigger. Defaults to below with the standard clearance.', + }, + hasSelectedItemOverlay: { + control: 'boolean', + description: + 'Native-select-style overlay: the open menu is pulled up so the selected option sits over the trigger. Only applies with placement below (the default).', }, isDisabled: { control: 'boolean', @@ -746,6 +751,29 @@ export const PlacementAbove: Story = { }, }; +export const SelectedItemOverlay: Story = { + render: args => { + const { + value: argsValue, + onChange: _onChange, + changeAction: _changeAction, + hasClear: _hc, + ...rest + } = args; + const [value, setValue] = useState(argsValue ?? 'Banana'); + return ( + setValue(v)} + hasSelectedItemOverlay + /> + ); + }, +}; + export const StatusVariantComparison: Story = { render: () => { const [a, setA] = useState(); diff --git a/packages/core/src/Selector/Selector.doc.mjs b/packages/core/src/Selector/Selector.doc.mjs index ebdeccbae309..149a25949431 100644 --- a/packages/core/src/Selector/Selector.doc.mjs +++ b/packages/core/src/Selector/Selector.doc.mjs @@ -28,7 +28,10 @@ export const docs = { }, {className: 'astryx-selector-option'}, {className: 'astryx-selector-empty-state'}, - {className: 'astryx-selector-clear-icon', deprecatedFor: 'input-clear-icon'}, + { + className: 'astryx-selector-clear-icon', + deprecatedFor: 'input-clear-icon', + }, {className: 'astryx-selector-indicator-icon', states: ['state']}, {className: 'astryx-selector-check'}, ], @@ -78,6 +81,20 @@ export const docs = { description: 'Placeholder text for the search input.', default: "'Search...'", }, + { + name: 'placement', + type: "'above' | 'below' | 'start' | 'end'", + description: + 'Menu placement relative to the trigger (e.g. above for bottom-fixed toolbars). The menu opens below with the standard clearance by default, like DropdownMenu.', + default: "'below'", + }, + { + name: 'hasSelectedItemOverlay', + type: 'boolean', + description: + 'Overlays the open menu on the trigger so the selected option sits directly over it, like a native macOS select; the menu is pulled up by a measured offset and clamped to the viewport. Only applies with placement below (the default); an explicit non-below placement or hasSearch uses standard layer positioning instead.', + default: 'false', + }, { name: 'placeholder', type: 'string', diff --git a/packages/core/src/Selector/Selector.test.tsx b/packages/core/src/Selector/Selector.test.tsx index dab7ca95eab2..26129c8bd487 100644 --- a/packages/core/src/Selector/Selector.test.tsx +++ b/packages/core/src/Selector/Selector.test.tsx @@ -28,6 +28,7 @@ import {__resetLiveRegionsForTest} from '../hooks/useAnnounce'; import {defineTheme} from '../theme/defineTheme'; import {Theme} from '../theme/Theme'; import {generateThemeCSS} from '../theme/generateThemeRules'; +import {spacingVars} from '../theme/tokens.stylex'; function generateThemeTestCSS(theme: Parameters[0]) { const {prose, component} = generateThemeCSS(theme); @@ -378,7 +379,7 @@ describe('Selector', () => { ); }); - it('clamps the default selected-item overlay to the viewport', async () => { + it('clamps the selected-item overlay to the viewport (hasSelectedItemOverlay)', async () => { const restoreRects = mockSelectorRects(); const user = userEvent.setup(); try { @@ -388,18 +389,24 @@ describe('Selector', () => { options={OPTIONS} value="Banana" onChange={() => {}} + hasSelectedItemOverlay />, ); 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', ); }); + // Overlay mode owns its geometry through the measured margin; the + // standard layer clearance must stay off or it would offset the + // opposite block edge asymmetrically. + expect(popover.style.getPropertyValue('--x-marginBlockStart')).toBe(''); + expect(popover.style.getPropertyValue('--x-marginBlockEnd')).toBe(''); } finally { restoreRects(); } @@ -430,6 +437,7 @@ describe('Selector', () => { options={OPTIONS} value="Banana" onChange={() => {}} + hasSelectedItemOverlay />, ); @@ -480,7 +488,7 @@ describe('Selector', () => { expect(inputDropdownClass).not.toBe(ghostDropdownClass); }); - it('does not apply selected-item overlay offset when placement is explicit', async () => { + it('explicit placement wins over hasSelectedItemOverlay', async () => { const restoreRects = mockSelectorRects(); const user = userEvent.setup(); try { @@ -491,6 +499,7 @@ describe('Selector', () => { value="Banana" onChange={() => {}} placement="above" + hasSelectedItemOverlay />, ); @@ -500,7 +509,7 @@ describe('Selector', () => { .closest('[popover]'); await waitFor(() => { expect(popover?.getAttribute('style')).not.toContain( - 'margin-block-start', + 'margin-block-start: -', ); }); } finally { @@ -508,6 +517,103 @@ describe('Selector', () => { } }); + describe('default placement (#4227)', () => { + it('opens below the trigger with the standard menu clearance by default', async () => { + const restoreRects = mockSelectorRects(); + const user = userEvent.setup(); + try { + render( + {}} + />, + ); + + await user.click(screen.getByRole('combobox')); + const popover = screen + .getByRole('listbox', {hidden: true}) + .closest('[popover]') as HTMLElement; + // DropdownMenu's clearance on both block edges, so the gap survives + // a position-try-fallbacks flip to above (#4803). + await waitFor(() => { + expect(popover.style.getPropertyValue('--x-marginBlockStart')).toBe( + spacingVars['--spacing-1'], + ); + }); + expect(popover.style.getPropertyValue('--x-marginBlockEnd')).toBe( + spacingVars['--spacing-1'], + ); + // Standard below positioning, same recipe as DropdownMenu, + // MultiSelector, and ComplexSelector. + expect(popover.getAttribute('style')).toContain( + 'position-area: self-block-end span-self-inline-end', + ); + // No selected-item overlay pulling the menu up over the trigger. + expect(popover.getAttribute('style')).not.toContain( + 'margin-block-start: -', + ); + } finally { + restoreRects(); + } + }); + + it('applies the standard clearance to explicit placements', async () => { + const user = userEvent.setup(); + render( + {}} + placement="above" + />, + ); + + 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.style.getPropertyValue('--x-marginBlockEnd')).toBe( + spacingVars['--spacing-1'], + ); + }); + + it('search mode gets the clearance and never overlays, even with hasSelectedItemOverlay', async () => { + const user = userEvent.setup(); + render( + {}} + hasSearch + hasSelectedItemOverlay + />, + ); + + // In hasSearch mode the trigger is a plain button, not a combobox. + await user.click(screen.getByRole('button', {name: 'Fruit'})); + 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: -', + ); + }); + }); + describe('hasClear', () => { it('shows selected value label when hasClear is enabled', () => { render( diff --git a/packages/core/src/Selector/Selector.tsx b/packages/core/src/Selector/Selector.tsx index 2353436eef1b..0e7886985f33 100644 --- a/packages/core/src/Selector/Selector.tsx +++ b/packages/core/src/Selector/Selector.tsx @@ -531,15 +531,22 @@ interface SelectorPropsBase< searchPlaceholder?: string; /** - * Position placement relative to the trigger. - * - * Omit to use the selector's default selected-item overlay behavior: the - * selected item is positioned over the trigger and clamped to the viewport. - * Set a placement to opt into explicit layer positioning (for example, + * Position placement relative to the trigger (for example, * `placement="above"` for bottom-fixed toolbars). + * @default 'below' */ placement?: LayerPlacement; + /** + * Whether to overlay the open menu on the trigger so the selected option + * sits directly over it, like a native macOS select. The menu is pulled up + * by a measured offset and clamped to the viewport. Only applies with + * placement below (the default); an explicit non-below placement or + * hasSearch uses standard layer positioning instead. + * @default false + */ + hasSelectedItemOverlay?: boolean; + /** * Whether the dropdown starts open on mount. * Useful for showcases and previews. @@ -671,6 +678,7 @@ export function Selector( hasSearch = false, searchPlaceholder: searchPlaceholderFromProps, placement, + hasSelectedItemOverlay = false, isDefaultOpen = false, 'data-testid': testId, width, @@ -829,10 +837,14 @@ export function Selector( [announce, selectableItems], ); - // 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; + // Calculate offset to position selected item over trigger. The overlay is + // opt-in (hasSelectedItemOverlay) and rides on below placement — an + // explicit non-below placement or search mode uses the standard layer + // positioning API instead. + const shouldOverlaySelectedItem = + hasSelectedItemOverlay && + !hasSearch && + (placement == null || placement === 'below'); const {offset: rawOffset, isPositioned: rawIsPositioned} = useSelectedItemOffset({ isOpen: popover.isOpen && shouldOverlaySelectedItem, @@ -1412,6 +1424,12 @@ export function Selector( { placement: popoverPlacement, alignment: 'start', + // Standard menu clearance (the DropdownMenu/MultiSelector recipe), + // except in overlay mode: there the measured negative margin owns + // the block geometry and must stay flush against the anchor. + offset: shouldOverlaySelectedItem + ? undefined + : spacingVars['--spacing-1'], xstyle: [styles.popover, layerAnimations[popoverPlacement]], style: popoverOffsetStyle, }, diff --git a/packages/core/src/Selector/hooks.ts b/packages/core/src/Selector/hooks.ts index f5f467c489a8..b306ffe11c25 100644 --- a/packages/core/src/Selector/hooks.ts +++ b/packages/core/src/Selector/hooks.ts @@ -57,9 +57,9 @@ interface UseSelectedItemOffsetResult { * * The desired dropdown top is calculated directly from the anchor center and * selected-item center, then clamped to the viewport. This preserves the - * default "selected item over trigger" behavior while letting the menu slide - * upward near the bottom edge or downward near the top edge instead of being - * clipped off-screen. + * opt-in "selected item over trigger" behavior (hasSelectedItemOverlay) while + * letting the menu slide upward near the bottom edge or downward near the top + * edge instead of being clipped off-screen. */ export function useSelectedItemOffset({ isOpen,