Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/selector-default-placement-below.md
Original file line number Diff line number Diff line change
@@ -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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search mode isn't flush any more, #5003 landed that. Needs a reword.


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
30 changes: 29 additions & 1 deletion apps/storybook/stories/Selector.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,12 @@ const meta: Meta<typeof Selector> = {
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',
Expand Down Expand Up @@ -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 (
<Selector
{...rest}
label="Native-select overlay"
options={['Apple', 'Banana', 'Cherry', 'Date']}
value={value}
onChange={v => setValue(v)}
hasSelectedItemOverlay
/>
);
},
};

export const StatusVariantComparison: Story = {
render: () => {
const [a, setA] = useState<string | undefined>();
Expand Down
19 changes: 18 additions & 1 deletion packages/core/src/Selector/Selector.doc.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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'},
],
Expand Down Expand Up @@ -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',
Expand Down
114 changes: 110 additions & 4 deletions packages/core/src/Selector/Selector.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof generateThemeCSS>[0]) {
const {prose, component} = generateThemeCSS(theme);
Expand Down Expand Up @@ -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 {
Expand All @@ -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();
}
Expand Down Expand Up @@ -430,6 +437,7 @@ describe('Selector', () => {
options={OPTIONS}
value="Banana"
onChange={() => {}}
hasSelectedItemOverlay
/>,
);

Expand Down Expand Up @@ -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 {
Expand All @@ -491,6 +499,7 @@ describe('Selector', () => {
value="Banana"
onChange={() => {}}
placement="above"
hasSelectedItemOverlay
/>,
);

Expand All @@ -500,14 +509,111 @@ describe('Selector', () => {
.closest('[popover]');
await waitFor(() => {
expect(popover?.getAttribute('style')).not.toContain(
'margin-block-start',
'margin-block-start: -',
);
});
} finally {
restoreRects();
}
});

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(
<Selector
label="Fruit"
options={OPTIONS}
value="Banana"
onChange={() => {}}
/>,
);

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(
<Selector
label="Fruit"
options={OPTIONS}
value="Banana"
onChange={() => {}}
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(
<Selector
label="Fruit"
options={OPTIONS}
value="Banana"
onChange={() => {}}
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(
Expand Down
36 changes: 27 additions & 9 deletions packages/core/src/Selector/Selector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by the 'overlay' / 'offset' placement values. Comes out with the flip.


/**
* Whether the dropdown starts open on mount.
* Useful for showcases and previews.
Expand Down Expand Up @@ -671,6 +678,7 @@ export function Selector<T extends SelectorOptionType>(
hasSearch = false,
searchPlaceholder: searchPlaceholderFromProps,
placement,
hasSelectedItemOverlay = false,
isDefaultOpen = false,
'data-testid': testId,
width,
Expand Down Expand Up @@ -829,10 +837,14 @@ export function Selector<T extends SelectorOptionType>(
[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,
Expand Down Expand Up @@ -1412,6 +1424,12 @@ export function Selector<T extends SelectorOptionType>(
{
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,
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/Selector/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading