Skip to content

Commit a98874e

Browse files
avinashbotavinashbot
authored
fix: Highlight the first select option by default if none are selected (#3594)
1 parent d234463 commit a98874e

File tree

10 files changed

+93
-45
lines changed

10 files changed

+93
-45
lines changed

‎src/internal/components/options-list/utils/use-highlight-option.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export interface HighlightedOptionState<OptionType> {
2323
export interface HighlightedOptionHandlers<OptionType> {
2424
// Mouse handlers
2525
setHighlightedIndexWithMouse(index: number, moveFocus?: boolean): void;
26+
highlightFirstOptionWithMouse(): void;
2627
// Keyboard handlers
2728
moveHighlightWithKeyboard(direction: -1 | 1): void;
2829
highlightOptionWithKeyboard(option: OptionType): void;
@@ -73,6 +74,7 @@ export function useHighlightedOption<OptionType>({
7374
{
7475
setHighlightedIndexWithMouse: (index: number, moveFocus = false) =>
7576
setHighlightedIndex(index, new HighlightType('mouse', moveFocus)),
77+
highlightFirstOptionWithMouse: () => moveHighlightFrom(1, -1, new HighlightType('mouse', true)),
7678
moveHighlightWithKeyboard: (direction: -1 | 1) => moveHighlight(direction, new HighlightType('keyboard')),
7779
highlightOptionWithKeyboard: (option: OptionType) => highlightOption(option, new HighlightType('keyboard')),
7880
resetHighlightWithKeyboard: () => setHighlightedIndex(-1, new HighlightType('keyboard')),

‎src/multiselect/__tests__/multiselect-embedded.test.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ test.each([false, true])('renders options, virtualScroll=%s', virtualScroll => {
6363
const items = createWrapper()
6464
.findAllByClassName(selectableItemsStyles['selectable-item'])
6565
.map(w => w.getElement());
66-
expect(items.map(item => item.textContent)).toEqual(['First', 'Second', 'Third', 'Fourth']);
66+
67+
['First', 'Second', 'Third', 'Fourth'].forEach((expected, i) => {
68+
expect(items[i]).toHaveTextContent(expected);
69+
});
6770
});
6871

6972
test.each([

‎src/multiselect/__tests__/multiselect.test.tsx

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -755,7 +755,10 @@ describe('Disabled item with reason', () => {
755755
test('has no tooltip open by default', () => {
756756
const { wrapper } = renderMultiselect(
757757
<Multiselect
758-
options={[{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }]}
758+
options={[
759+
{ label: 'First', value: '1', disabled: false },
760+
{ label: 'Second', value: '2', disabled: true, disabledReason: 'disabled reason' },
761+
]}
759762
selectedOptions={[]}
760763
/>
761764
);
@@ -777,14 +780,17 @@ describe('Disabled item with reason', () => {
777780
test('open tooltip when the item is highlighted', () => {
778781
const { wrapper } = renderMultiselect(
779782
<Multiselect
780-
options={[{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }]}
783+
options={[
784+
{ label: 'First', value: '1', disabled: false },
785+
{ label: 'Second', value: '2', disabled: true, disabledReason: 'disabled reason' },
786+
]}
781787
selectedOptions={[]}
782788
/>
783789
);
784790
wrapper.openDropdown();
785-
wrapper.findTrigger().keydown(KeyCode.down);
791+
wrapper.findDropdown().findOptionsContainer()!.keydown(KeyCode.down);
786792

787-
expect(wrapper.findDropdown().findOption(1)!.findDisabledReason()!.getElement()).toHaveTextContent(
793+
expect(wrapper.findDropdown().findOption(2)!.findDisabledReason()!.getElement()).toHaveTextContent(
788794
'disabled reason'
789795
);
790796
});

‎src/multiselect/__tests__/select-all.test.tsx

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -154,20 +154,18 @@ describe('Multiselect with "select all" control', () => {
154154
wrapper.openDropdown();
155155
const dropdown = wrapper.findDropdown();
156156
const optionsContainer = dropdown.findOptionsContainer()!;
157-
// When opening the dropdown no option gets highlighted if none is selected. Move one position down to highlight the "Select all" control.
158-
optionsContainer.keydown(KeyCode.down);
157+
// When opening the dropdown the first option ("Select all") gets highlighted if none is selected.
159158
optionsContainer.keydown(KeyCode.space);
160159
expect(dropdown.findOptionByValue('1')).toBeNull();
161160
});
162161

163-
describe.each([false, true])('virtuaScroll=%s', virtualScroll => {
162+
describe.each([false, true])('virtualScroll=%s', virtualScroll => {
164163
test('selects all options when none are selected', () => {
165164
const onChange = jest.fn();
166165
const wrapper = renderMultiselectWithSelectAll({ onChange, virtualScroll });
167166
wrapper.openDropdown();
168167
const optionsContainer = wrapper.findDropdown().findOptionsContainer()!;
169-
// When opening the dropdown no option gets highlighted if none is selected. Move one position down to highlight the "Select all" control.
170-
optionsContainer.keydown(KeyCode.down);
168+
// When opening the dropdown the first option ("Select all") gets highlighted if none is selected.
171169
optionsContainer.keydown(KeyCode.space);
172170
expect(onChange).toHaveBeenCalledWith(
173171
expect.objectContaining({ detail: { selectedOptions: optionsWithoutGroups } })
@@ -187,14 +185,14 @@ describe('Multiselect with "select all" control', () => {
187185
const wrapper = createWrapper(container).findMultiselect()!;
188186
wrapper.openDropdown();
189187
const selectAll = wrapper.findDropdown().findSelectAll()!;
190-
expect(selectAll.getElement().textContent).toBe('Custom Select all text');
188+
expect(selectAll.getElement()).toHaveTextContent('Custom Select all text');
191189
});
192190

193191
test('uses i18nStrings.selectAllText from i18nStrings prop', () => {
194192
const wrapper = renderMultiselectWithSelectAll({ i18nStrings: { selectAllText: 'Custom Select all text' } });
195193
wrapper.openDropdown();
196194
const selectAll = wrapper.findDropdown().findSelectAll()!;
197-
expect(selectAll.getElement().textContent).toBe('Custom Select all text');
195+
expect(selectAll.getElement()).toHaveTextContent('Custom Select all text');
198196
});
199197

200198
test('uses i18nStrings.selectAllText from i18nStrings prop when both i18n provider and i18nStrings prop are provided ', () => {
@@ -214,7 +212,7 @@ describe('Multiselect with "select all" control', () => {
214212
const wrapper = createWrapper(container).findMultiselect()!;
215213
wrapper.openDropdown();
216214
const selectAll = wrapper.findDropdown().findSelectAll()!;
217-
expect(selectAll.getElement().textContent).toBe('Custom Select all text from i18nStrings');
215+
expect(selectAll.getElement()).toHaveTextContent('Custom Select all text from i18nStrings');
218216
});
219217
});
220218

‎src/select/__integ__/page-objects/select-page.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ export default class SelectPageObject<
8484
assert.equal(
8585
await this.isExisting(this.wrapper.findDropdown().findHighlightedOption().toSelector()),
8686
isDisplayed,
87-
`There should ${isDisplayed ? '' : 'not '} be a highlighted option`
87+
`There should ${isDisplayed ? '' : 'not '}be a highlighted option`
8888
);
8989
}
9090

‎src/select/__integ__/select-native-search.test.ts

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -73,15 +73,6 @@ describe(`Select (Native Search)`, () => {
7373
})
7474
);
7575

76-
test(
77-
'no match - no highlight, with dropdown open',
78-
setupTest(optionsType, async page => {
79-
await page.clickSelect();
80-
await page.keys(['a']);
81-
await page.assertHighlightedOption(false);
82-
})
83-
);
84-
8576
test(
8677
'no match - no selection, with dropdown closed',
8778
setupTest(optionsType, async page => {
@@ -112,15 +103,6 @@ describe(`Select (Native Search)`, () => {
112103
await expect(page.getTriggerLabel()).resolves.toMatch('Third thing');
113104
})
114105
);
115-
116-
test(
117-
'no highlight when the option does not start with the search substring',
118-
setupTest(optionsType, async page => {
119-
await page.clickSelect();
120-
await page.keys(['a']);
121-
await page.assertHighlightedOption(false);
122-
})
123-
);
124106
});
125107

126108
describe('Options - Semi-extended', () => {

‎src/select/__integ__/select.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,8 @@ test(
280280
await browser.url('/#/light/select/select.test.scroll-padding');
281281
const page = new SelectPageObject(browser, createWrapper().findSelect());
282282
await page.setWindowSize({ width: 800, height: 250 });
283-
await page.windowScrollTo({ top: 25 });
283+
await page.windowScrollTo({ top: 15 });
284284
await page.clickSelect();
285-
await expect(page.getWindowScroll()).resolves.toEqual({ top: 25, left: 0 });
285+
await expect(page.getWindowScroll()).resolves.toEqual({ top: 15, left: 0 });
286286
})
287287
);

‎src/select/__tests__/disabled.test.tsx

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,32 +52,41 @@ describe.each([false, true])('expandToViewport=%s', expandToViewport => {
5252
describe('Disabled item with reason', () => {
5353
test('has no tooltip open by default', () => {
5454
const { wrapper } = renderSelect({
55-
options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }],
55+
options: [
56+
{ label: 'First', value: '1' },
57+
{ label: 'Second', value: '2', disabled: true, disabledReason: 'disabled reason' },
58+
],
5659
});
5760
wrapper.openDropdown();
5861

59-
expect(wrapper.findDropdown({ expandToViewport }).findOption(1)!.findDisabledReason()).toBe(null);
62+
expect(wrapper.findDropdown({ expandToViewport }).findOption(2)!.findDisabledReason()).toBe(null);
6063
});
6164

6265
test('has no tooltip without disabledReason', () => {
6366
const { wrapper } = renderSelect({
64-
options: [{ label: 'First', value: '1', disabled: true }],
67+
options: [
68+
{ label: 'First', value: '1' },
69+
{ label: 'Second', value: '2', disabled: true },
70+
],
6571
});
6672
wrapper.openDropdown();
67-
wrapper.findTrigger()!.keydown(KeyCode.down);
73+
wrapper.findDropdown({ expandToViewport }).findOptionsContainer()!.keydown(KeyCode.down);
6874

69-
expect(wrapper.findDropdown({ expandToViewport }).findOption(1)!.findDisabledReason()).toBe(null);
75+
expect(wrapper.findDropdown({ expandToViewport }).findOption(2)!.findDisabledReason()).toBe(null);
7076
});
7177

7278
test('open tooltip when the item is highlighted', () => {
7379
const { wrapper } = renderSelect({
74-
options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }],
80+
options: [
81+
{ label: 'First', value: '1' },
82+
{ label: 'Second', value: '2', disabled: true, disabledReason: 'disabled reason' },
83+
],
7584
});
7685
wrapper.openDropdown();
77-
wrapper.findTrigger().keydown(KeyCode.down);
86+
wrapper.findDropdown({ expandToViewport }).findOptionsContainer()!.keydown(KeyCode.down);
7887

7988
expect(
80-
wrapper.findDropdown({ expandToViewport }).findOption(1)!.findDisabledReason()!.getElement()
89+
wrapper.findDropdown({ expandToViewport }).findOption(2)!.findDisabledReason()!.getElement()
8190
).toHaveTextContent('disabled reason');
8291
});
8392

‎src/select/__tests__/use-select.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,24 @@ describe('useSelect', () => {
186186
expect(testEvent.defaultPrevented).toBe(true);
187187
});
188188

189+
test('should highlight the first option by default (keyboard:enter)', () => {
190+
const hook = renderHook(useSelect, { initialProps: { ...initialProps, filteringType: 'none' } });
191+
192+
const { getTriggerProps } = hook.result.current;
193+
const triggerProps = getTriggerProps();
194+
act(() => triggerProps.onKeyDown && triggerProps.onKeyDown(createTestEvent(KeyCode.enter)));
195+
expect(hook.result.current.isOpen).toBe(true);
196+
expect(hook.result.current.highlightedOption).toEqual({
197+
type: 'child',
198+
option: {
199+
label: 'Child 1',
200+
value: 'child1',
201+
},
202+
});
203+
expect(hook.result.current.highlightType.type).toBe('keyboard');
204+
expect(hook.result.current.highlightType.moveFocus).toBe(true);
205+
});
206+
189207
test('should open and navigate to the first option (keyboard:down)', () => {
190208
const hook = renderHook(useSelect, {
191209
initialProps,
@@ -283,6 +301,24 @@ describe('useSelect', () => {
283301
expect(hook.result.current.highlightType.moveFocus).toBe(true);
284302
});
285303

304+
test('should highlight the first option by default (mouse)', () => {
305+
const hook = renderHook(useSelect, { initialProps: { ...initialProps, filteringType: 'none' } });
306+
307+
const { getTriggerProps } = hook.result.current;
308+
const triggerProps = getTriggerProps();
309+
act(() => triggerProps.onMouseDown && triggerProps.onMouseDown(createCustomEvent({})));
310+
expect(hook.result.current.isOpen).toBe(true);
311+
expect(hook.result.current.highlightedOption).toEqual({
312+
type: 'child',
313+
option: {
314+
label: 'Child 1',
315+
value: 'child1',
316+
},
317+
});
318+
expect(hook.result.current.highlightType.type).toBe('mouse');
319+
expect(hook.result.current.highlightType.moveFocus).toBe(true);
320+
});
321+
286322
test('should open and highlight the selected option (mouse)', () => {
287323
const hook = renderHook(useSelect, {
288324
initialProps: { ...initialProps, filteringType: 'none', selectedOptions: [initialProps.options[1].option] },

‎src/select/utils/use-select.ts

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ export function useSelect({
8989
resetHighlightWithKeyboard,
9090
setHighlightedIndexWithMouse,
9191
highlightOptionWithKeyboard,
92+
highlightFirstOptionWithMouse,
9293
goHomeWithKeyboard,
9394
goEndWithKeyboard,
9495
},
@@ -300,11 +301,20 @@ export function useSelect({
300301
useEffect(() => {
301302
// highlight the first selected option, when opening the Select component without filter input
302303
// keep the focus in the filter input when opening, so that screenreader can recognize the combobox
303-
if (isOpen && !prevOpen && hasSelectedOption && !hasFilter) {
304+
if (isOpen && !prevOpen && options.length > 0 && !hasFilter) {
304305
if (openedWithKeyboard) {
305-
highlightOptionWithKeyboard(__selectedOptions[0]);
306+
if (__selectedOptions[0]) {
307+
highlightOptionWithKeyboard(__selectedOptions[0]);
308+
} else {
309+
goHomeWithKeyboard();
310+
}
306311
} else {
307-
setHighlightedIndexWithMouse(options.indexOf(__selectedOptions[0]), true);
312+
if (!__selectedOptions[0] || !options.includes(__selectedOptions[0])) {
313+
highlightFirstOptionWithMouse();
314+
} else {
315+
const highlightedIndex = options.indexOf(__selectedOptions[0]);
316+
setHighlightedIndexWithMouse(highlightedIndex, true);
317+
}
308318
}
309319
}
310320
}, [
@@ -313,6 +323,8 @@ export function useSelect({
313323
hasSelectedOption,
314324
setHighlightedIndexWithMouse,
315325
highlightOptionWithKeyboard,
326+
highlightFirstOptionWithMouse,
327+
goHomeWithKeyboard,
316328
openedWithKeyboard,
317329
options,
318330
prevOpen,

0 commit comments

Comments
 (0)