Skip to content

Commit 1ac1315

Browse files
avinashbotavinashbot
authored
revert: Revert "fix: Highlight the first select option by default if none are selected" (#3590)
1 parent d6c0425 commit 1ac1315

File tree

10 files changed

+45
-93
lines changed

10 files changed

+45
-93
lines changed

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ export interface HighlightedOptionState<OptionType> {
2323
export interface HighlightedOptionHandlers<OptionType> {
2424
// Mouse handlers
2525
setHighlightedIndexWithMouse(index: number, moveFocus?: boolean): void;
26-
highlightFirstOptionWithMouse(): void;
2726
// Keyboard handlers
2827
moveHighlightWithKeyboard(direction: -1 | 1): void;
2928
highlightOptionWithKeyboard(option: OptionType): void;
@@ -74,7 +73,6 @@ export function useHighlightedOption<OptionType>({
7473
{
7574
setHighlightedIndexWithMouse: (index: number, moveFocus = false) =>
7675
setHighlightedIndex(index, new HighlightType('mouse', moveFocus)),
77-
highlightFirstOptionWithMouse: () => moveHighlightFrom(1, -1, new HighlightType('mouse', true)),
7876
moveHighlightWithKeyboard: (direction: -1 | 1) => moveHighlight(direction, new HighlightType('keyboard')),
7977
highlightOptionWithKeyboard: (option: OptionType) => highlightOption(option, new HighlightType('keyboard')),
8078
resetHighlightWithKeyboard: () => setHighlightedIndex(-1, new HighlightType('keyboard')),

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

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

7269
test.each([

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

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

793-
expect(wrapper.findDropdown().findOption(2)!.findDisabledReason()!.getElement()).toHaveTextContent(
787+
expect(wrapper.findDropdown().findOption(1)!.findDisabledReason()!.getElement()).toHaveTextContent(
794788
'disabled reason'
795789
);
796790
});

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -154,18 +154,20 @@ describe('Multiselect with "select all" control', () => {
154154
wrapper.openDropdown();
155155
const dropdown = wrapper.findDropdown();
156156
const optionsContainer = dropdown.findOptionsContainer()!;
157-
// When opening the dropdown the first option ("Select all") gets highlighted if none is selected.
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);
158159
optionsContainer.keydown(KeyCode.space);
159160
expect(dropdown.findOptionByValue('1')).toBeNull();
160161
});
161162

162-
describe.each([false, true])('virtualScroll=%s', virtualScroll => {
163+
describe.each([false, true])('virtuaScroll=%s', virtualScroll => {
163164
test('selects all options when none are selected', () => {
164165
const onChange = jest.fn();
165166
const wrapper = renderMultiselectWithSelectAll({ onChange, virtualScroll });
166167
wrapper.openDropdown();
167168
const optionsContainer = wrapper.findDropdown().findOptionsContainer()!;
168-
// When opening the dropdown the first option ("Select all") gets highlighted if none is selected.
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);
169171
optionsContainer.keydown(KeyCode.space);
170172
expect(onChange).toHaveBeenCalledWith(
171173
expect.objectContaining({ detail: { selectedOptions: optionsWithoutGroups } })
@@ -185,14 +187,14 @@ describe('Multiselect with "select all" control', () => {
185187
const wrapper = createWrapper(container).findMultiselect()!;
186188
wrapper.openDropdown();
187189
const selectAll = wrapper.findDropdown().findSelectAll()!;
188-
expect(selectAll.getElement()).toHaveTextContent('Custom Select all text');
190+
expect(selectAll.getElement().textContent).toBe('Custom Select all text');
189191
});
190192

191193
test('uses i18nStrings.selectAllText from i18nStrings prop', () => {
192194
const wrapper = renderMultiselectWithSelectAll({ i18nStrings: { selectAllText: 'Custom Select all text' } });
193195
wrapper.openDropdown();
194196
const selectAll = wrapper.findDropdown().findSelectAll()!;
195-
expect(selectAll.getElement()).toHaveTextContent('Custom Select all text');
197+
expect(selectAll.getElement().textContent).toBe('Custom Select all text');
196198
});
197199

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

‎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: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,15 @@ 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+
7685
test(
7786
'no match - no selection, with dropdown closed',
7887
setupTest(optionsType, async page => {
@@ -103,6 +112,15 @@ describe(`Select (Native Search)`, () => {
103112
await expect(page.getTriggerLabel()).resolves.toMatch('Third thing');
104113
})
105114
);
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+
);
106124
});
107125

108126
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: 15 });
283+
await page.windowScrollTo({ top: 25 });
284284
await page.clickSelect();
285-
await expect(page.getWindowScroll()).resolves.toEqual({ top: 15, left: 0 });
285+
await expect(page.getWindowScroll()).resolves.toEqual({ top: 25, left: 0 });
286286
})
287287
);

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

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -52,41 +52,32 @@ 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: [
56-
{ label: 'First', value: '1' },
57-
{ label: 'Second', value: '2', disabled: true, disabledReason: 'disabled reason' },
58-
],
55+
options: [{ label: 'First', value: '1', disabled: true, disabledReason: 'disabled reason' }],
5956
});
6057
wrapper.openDropdown();
6158

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

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

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

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

8879
expect(
89-
wrapper.findDropdown({ expandToViewport }).findOption(2)!.findDisabledReason()!.getElement()
80+
wrapper.findDropdown({ expandToViewport }).findOption(1)!.findDisabledReason()!.getElement()
9081
).toHaveTextContent('disabled reason');
9182
});
9283

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

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -186,24 +186,6 @@ 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-
207189
test('should open and navigate to the first option (keyboard:down)', () => {
208190
const hook = renderHook(useSelect, {
209191
initialProps,
@@ -301,24 +283,6 @@ describe('useSelect', () => {
301283
expect(hook.result.current.highlightType.moveFocus).toBe(true);
302284
});
303285

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-
322286
test('should open and highlight the selected option (mouse)', () => {
323287
const hook = renderHook(useSelect, {
324288
initialProps: { ...initialProps, filteringType: 'none', selectedOptions: [initialProps.options[1].option] },

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

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ export function useSelect({
8989
resetHighlightWithKeyboard,
9090
setHighlightedIndexWithMouse,
9191
highlightOptionWithKeyboard,
92-
highlightFirstOptionWithMouse,
9392
goHomeWithKeyboard,
9493
goEndWithKeyboard,
9594
},
@@ -301,20 +300,11 @@ export function useSelect({
301300
useEffect(() => {
302301
// highlight the first selected option, when opening the Select component without filter input
303302
// keep the focus in the filter input when opening, so that screenreader can recognize the combobox
304-
if (isOpen && !prevOpen && options.length > 0 && !hasFilter) {
303+
if (isOpen && !prevOpen && hasSelectedOption && !hasFilter) {
305304
if (openedWithKeyboard) {
306-
if (__selectedOptions[0]) {
307-
highlightOptionWithKeyboard(__selectedOptions[0]);
308-
} else {
309-
goHomeWithKeyboard();
310-
}
305+
highlightOptionWithKeyboard(__selectedOptions[0]);
311306
} else {
312-
if (!__selectedOptions[0] || !options.includes(__selectedOptions[0])) {
313-
highlightFirstOptionWithMouse();
314-
} else {
315-
const highlightedIndex = options.indexOf(__selectedOptions[0]);
316-
setHighlightedIndexWithMouse(highlightedIndex, true);
317-
}
307+
setHighlightedIndexWithMouse(options.indexOf(__selectedOptions[0]), true);
318308
}
319309
}
320310
}, [
@@ -323,8 +313,6 @@ export function useSelect({
323313
hasSelectedOption,
324314
setHighlightedIndexWithMouse,
325315
highlightOptionWithKeyboard,
326-
highlightFirstOptionWithMouse,
327-
goHomeWithKeyboard,
328316
openedWithKeyboard,
329317
options,
330318
prevOpen,

0 commit comments

Comments
 (0)