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,