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
81 changes: 81 additions & 0 deletions static/app/components/core/compactSelect/compactSelect.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,87 @@ describe('CompactSelect', () => {
expect(screen.queryByRole('option', {name: 'Option One'})).not.toBeInTheDocument();
});

it('selects the single remaining option when Enter is pressed', async () => {
const onChange = jest.fn();
render(
<CompactSelect
search={{placeholder: 'Search here…'}}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
]}
value={undefined}
onChange={onChange}
/>
);

await userEvent.click(screen.getByRole('button'));
await userEvent.click(screen.getByPlaceholderText('Search here…'));

// type 'Two' to narrow the list to exactly one option
await userEvent.keyboard('Two');
expect(screen.getByRole('option', {name: 'Option Two'})).toBeInTheDocument();
expect(screen.queryByRole('option', {name: 'Option One'})).not.toBeInTheDocument();

// pressing Enter should select the single visible option
await userEvent.keyboard('{Enter}');
expect(onChange).toHaveBeenCalledWith(expect.objectContaining({value: 'opt_two'}));
});

it('does not auto-select when multiple options are visible and Enter is pressed', async () => {
const onChange = jest.fn();
render(
<CompactSelect
search={{placeholder: 'Search here…'}}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two'},
]}
value={undefined}
onChange={onChange}
/>
);

await userEvent.click(screen.getByRole('button'));
await userEvent.click(screen.getByPlaceholderText('Search here…'));

// type 'Option' — both options are still visible
await userEvent.keyboard('Option');
expect(screen.getByRole('option', {name: 'Option One'})).toBeInTheDocument();
expect(screen.getByRole('option', {name: 'Option Two'})).toBeInTheDocument();

// pressing Enter should not select anything
await userEvent.keyboard('{Enter}');
expect(onChange).not.toHaveBeenCalled();
});

it('does not auto-select when one enabled and one disabled option are visible', async () => {
const onChange = jest.fn();
render(
<CompactSelect
search={{placeholder: 'Search here…'}}
options={[
{value: 'opt_one', label: 'Option One'},
{value: 'opt_two', label: 'Option Two', disabled: true},
]}
value={undefined}
onChange={onChange}
/>
);

await userEvent.click(screen.getByRole('button'));
await userEvent.click(screen.getByPlaceholderText('Search here…'));

// type 'Option' — both options are visible (one enabled, one disabled)
await userEvent.keyboard('Option');
expect(screen.getByRole('option', {name: 'Option One'})).toBeInTheDocument();
expect(screen.getByRole('option', {name: 'Option Two'})).toBeInTheDocument();

// pressing Enter must not auto-select even though only one option is enabled
await userEvent.keyboard('{Enter}');
expect(onChange).not.toHaveBeenCalled();
});

it('restores full list when search query is cleared', async () => {
render(
<CompactSelect
Expand Down
19 changes: 18 additions & 1 deletion static/app/components/core/compactSelect/control.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -272,9 +272,26 @@ export function Control({
?.focus();
}

// Prevent form submissions on Enter key press in search box
// Prevent form submissions on Enter key press in search box.
// If there is exactly one visible option (enabled or disabled), and it is
// enabled, select it automatically. We count all visible options so that a
// disabled-but-visible option does not trick us into auto-selecting when
// the user still sees multiple items in the list.
if (e.key === 'Enter') {
e.preventDefault();
const role = grid ? 'row' : 'option';
const allVisibleOptions = [
...(overlayRef.current?.querySelectorAll<HTMLLIElement>(`li[role="${role}"]`) ??
[]),
];
const firstOption = allVisibleOptions[0];
if (
allVisibleOptions.length === 1 &&
firstOption &&
firstOption.getAttribute('aria-disabled') !== 'true'
) {
firstOption.click();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don’t think going via the dom is what we should be doing here. Control has access to all items, so it should know if there’s only one item. It doesn’t have access to onChange, but what we probably should do is something similar that onClear is doing, because in the parent, we can easily invoke onChange:

onClear={({overlayState}) => {
if (clearable) {
if (multiple) {
onChange([]);
} else {
onChange(undefined);
}
if (shouldCloseOnSelect?.({...listProps, selectedOptions: []})) {
overlayState?.close();
}
}
}}

Concretely, I’d suggest:

  • add on onEnter callback
  • have the parent decide:
    • if there is only one element in items, invoke onChange. close select if necessary (shouldCloseOnSelect, see onClear. This could be extracted to its own function triggerOnChange.
    • if there are more items, maybe we could have Enter focus the list instead like tab does?

}
Comment thread
JonasBa marked this conversation as resolved.
}

// Continue propagation, otherwise the overlay won't close on Esc key press
Expand Down
Loading