Skip to content

Commit 6d60bab

Browse files
CopilotmfranzkeCopilotCopilotnmerget
authored
fix(custom-select): keyboard navigation for option groups in single-select mode (#4921)
* Initial plan * Investigation: Identified root cause of keyboard navigation issue in custom-select option groups Co-authored-by: mfranzke <787658+mfranzke@users.noreply.github.com> * Fix keyboard navigation issue in custom-select option groups + add comprehensive test Co-authored-by: mfranzke <787658+mfranzke@users.noreply.github.com> * Final validation and cleanup - fix confirmed working Co-authored-by: mfranzke <787658+mfranzke@users.noreply.github.com> * Consolidate option groups tests into existing test functions Co-authored-by: mfranzke <787658+mfranzke@users.noreply.github.com> * Update packages/components/src/components/custom-select/custom-select.spec.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * test: replace hard-coded timeouts with reliable waitForFocusChange helper function * fix: issue with test in custom-select.spec.tsx * chore: add changeset --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mfranzke <787658+mfranzke@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Nicolas Merget <nicolas.merget@deutschebahn.com> Co-authored-by: Nicolas Merget <104347736+nmerget@users.noreply.github.com>
1 parent e969425 commit 6d60bab

File tree

3 files changed

+188
-46
lines changed

3 files changed

+188
-46
lines changed

‎.changeset/keyboard-navigation-option-groups.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@db-ux/core-components": patch
3+
"@db-ux/ngx-core-components": patch
4+
"@db-ux/react-core-components": patch
5+
"@db-ux/v-core-components": patch
6+
"@db-ux/wc-core-components": patch
7+
---
8+
9+
- fix(custom-select): keyboard navigation for option groups in single-select mode:
10+
- Fixes a keyboard accessibility issue where users could not navigate to options in subsequent option groups using arrow keys in single-select mode.
11+
- Now, all options are accessible via keyboard regardless of group boundaries.
12+
13+

‎packages/components/src/components/custom-select/custom-select.lite.tsx‎

Lines changed: 61 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -297,47 +297,74 @@ export default function DBCustomSelect(props: DBCustomSelectProps) {
297297
event.key === 'ArrowDown' ||
298298
event.key === 'ArrowRight'
299299
) {
300-
if (listElement?.nextElementSibling) {
301-
listElement?.nextElementSibling
302-
?.querySelector('input')
303-
?.focus();
304-
} else {
300+
// Find next element with input, skipping group titles
301+
let nextElement =
302+
listElement?.nextElementSibling;
303+
while (nextElement) {
304+
const nextInput =
305+
nextElement.querySelector('input');
306+
if (nextInput) {
307+
nextInput.focus();
308+
break;
309+
}
310+
nextElement =
311+
nextElement.nextElementSibling;
312+
}
313+
314+
if (!nextElement) {
305315
// We are on the last checkbox we move to the top checkbox
306316
state.handleFocusFirstDropdownCheckbox(
307317
activeElement
308318
);
309319
}
310320
} else {
311-
if (listElement?.previousElementSibling) {
312-
listElement?.previousElementSibling
313-
?.querySelector('input')
314-
?.focus();
315-
} else if (
316-
detailsRef.querySelector(
317-
`input[type="checkbox"]`
318-
) !== activeElement
319-
) {
320-
// We are on the top list checkbox but there is a select all checkbox as well
321-
state.handleFocusFirstDropdownCheckbox(
322-
activeElement
323-
);
324-
} else {
325-
// We are on the top checkbox, we need to move to the search
326-
// or to the last checkbox
327-
const search = getSearchInput(detailsRef);
328-
if (search) {
329-
delay(() => {
330-
search.focus();
331-
}, 100);
321+
// Find previous element with input, skipping group titles
322+
let prevElement =
323+
listElement?.previousElementSibling;
324+
while (prevElement) {
325+
const prevInput =
326+
prevElement.querySelector('input');
327+
if (prevInput) {
328+
prevInput.focus();
329+
break;
330+
}
331+
prevElement =
332+
prevElement.previousElementSibling;
333+
}
334+
335+
if (!prevElement) {
336+
// Check if we have a "select all" checkbox (only relevant for multi-select)
337+
const selectAllCheckbox =
338+
detailsRef.querySelector(
339+
`input[type="checkbox"]`
340+
);
341+
if (
342+
selectAllCheckbox &&
343+
selectAllCheckbox !== activeElement
344+
) {
345+
// We are on the top list checkbox but there is a select all checkbox as well
346+
state.handleFocusFirstDropdownCheckbox(
347+
activeElement
348+
);
332349
} else {
333-
const checkboxList: HTMLInputElement[] =
334-
Array.from(
335-
detailsRef?.querySelectorAll(
336-
`input[type="checkbox"],input[type="radio"]`
337-
)
338-
);
339-
if (checkboxList.length) {
340-
checkboxList.at(-1)?.focus();
350+
// We are on the top checkbox, we need to move to the search
351+
// or to the last checkbox
352+
const search =
353+
getSearchInput(detailsRef);
354+
if (search) {
355+
delay(() => {
356+
search.focus();
357+
}, 100);
358+
} else {
359+
const checkboxList: HTMLInputElement[] =
360+
Array.from(
361+
detailsRef?.querySelectorAll(
362+
`input[type="checkbox"],input[type="radio"]`
363+
)
364+
);
365+
if (checkboxList.length) {
366+
checkboxList.at(-1)?.focus();
367+
}
341368
}
342369
}
343370
}

‎packages/components/src/components/custom-select/custom-select.spec.tsx‎

Lines changed: 114 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,22 @@ import { DBCustomSelect } from './index';
55
// @ts-ignore - vue can only find it with .ts as file ending
66
import { DEFAULT_VIEWPORT } from '../../shared/constants.ts';
77

8+
// Helper function to wait for focus changes instead of using hard-coded timeouts
9+
const waitForFocusChange = async (
10+
page: any,
11+
expectedValue: string,
12+
timeout = 5000
13+
) => {
14+
await page.waitForFunction(
15+
(expected: any) => {
16+
const activeElement = document.activeElement as HTMLInputElement;
17+
return activeElement && activeElement.value === expected;
18+
},
19+
expectedValue,
20+
{ timeout }
21+
);
22+
};
23+
824
const comp: any = (
925
<DBCustomSelect
1026
options={[{ value: 'Option 1' }, { value: 'Option 2' }]}
@@ -39,6 +55,21 @@ const selectAllSelect: any = (
3955
placeholder="Placeholder"></DBCustomSelect>
4056
);
4157

58+
const optionGroupsComp: any = (
59+
<DBCustomSelect
60+
options={[
61+
{ label: 'Option group 1', isGroupTitle: true },
62+
{ value: 'G1:Option 1' },
63+
{ value: 'G1:Option 2' },
64+
{ label: 'Option group 2', isGroupTitle: true },
65+
{ value: 'G2:Option 1' },
66+
{ value: 'G2:Option 2' }
67+
]}
68+
label="Test Option Groups"
69+
placeholder="Placeholder"
70+
/>
71+
);
72+
4273
const tagSelectWithCustomRemoveTexts: any = (
4374
<DBCustomSelect
4475
options={[
@@ -51,7 +82,7 @@ const tagSelectWithCustomRemoveTexts: any = (
5182
selectedType="tag"
5283
removeTagsTexts={[
5384
'Remove Red Color',
54-
'Remove Blue Color',
85+
'Remove Blue Color',
5586
'Remove Green Color'
5687
]}
5788
values={['Blue', 'Green']}
@@ -84,6 +115,14 @@ const testA11y = () => {
84115

85116
expect(accessibilityScanResults.violations).toEqual([]);
86117
});
118+
test('option groups should be accessible', async ({ page, mount }) => {
119+
await mount(optionGroupsComp);
120+
const accessibilityScanResults = await new AxeBuilder({ page })
121+
.include('.db-custom-select')
122+
.analyze();
123+
124+
expect(accessibilityScanResults.violations).toEqual([]);
125+
});
87126
};
88127

89128
const testAction = () => {
@@ -93,7 +132,7 @@ const testAction = () => {
93132
await expect(summary).not.toContainText('Option 1');
94133
await page.keyboard.press('Tab');
95134
await page.keyboard.press('ArrowDown');
96-
await page.waitForTimeout(1000); // wait for focus to apply
135+
await waitForFocusChange(page, 'Option 1');
97136
await page.keyboard.press('Space');
98137
await expect(summary).toContainText('Option 1');
99138
});
@@ -104,7 +143,7 @@ const testAction = () => {
104143
await expect(summary).not.toContainText('Option 1');
105144
await page.keyboard.press('Tab');
106145
await page.keyboard.press('ArrowDown');
107-
await page.waitForTimeout(1000); // wait for focus to apply
146+
await waitForFocusChange(page, 'Option 1');
108147
await page.keyboard.press('Space');
109148
await page.keyboard.press('Escape');
110149
await expect(summary).toContainText('Option 1');
@@ -139,7 +178,7 @@ const testAction = () => {
139178
await expect(summary).not.toContainText('Option 1');
140179
await page.keyboard.press('Tab');
141180
await page.keyboard.press('ArrowDown');
142-
await page.waitForTimeout(1000); // wait for focus to apply
181+
await waitForFocusChange(page, 'Option 1');
143182
await page.keyboard.press('Enter');
144183
await expect(summary).toContainText('Option 1');
145184
// For single select, dropdown should be closed after Enter
@@ -152,32 +191,95 @@ const testAction = () => {
152191
await expect(summary).not.toContainText('Option 1');
153192
await page.keyboard.press('Tab');
154193
await page.keyboard.press('ArrowDown');
155-
await page.waitForTimeout(1000); // wait for focus to apply
194+
await waitForFocusChange(page, 'Option 1');
156195
await page.keyboard.press('Enter');
157196
// For multiple select, dropdown should remain open after Enter
158197
await expect(component.locator('details')).toHaveAttribute('open');
159198
await page.keyboard.press('Escape');
160199
await expect(summary).toContainText('Option 1');
161200
});
162201

163-
test('custom removeTagsTexts should correspond to correct options', async ({ mount }) => {
202+
test('option groups keyboard navigation: should navigate between option groups correctly', async ({
203+
page,
204+
mount
205+
}) => {
206+
const component = await mount(optionGroupsComp);
207+
208+
// Open the dropdown and focus first option
209+
await page.keyboard.press('Tab');
210+
await page.keyboard.press('ArrowDown');
211+
await waitForFocusChange(page, 'G1:Option 1');
212+
213+
// Should be focused on G1:Option 1
214+
const focused1 = await page.evaluate(
215+
() => (document.activeElement as HTMLInputElement)?.value
216+
);
217+
expect(focused1).toBe('G1:Option 1');
218+
219+
// Navigate to G1:Option 2
220+
await page.keyboard.press('ArrowDown');
221+
await waitForFocusChange(page, 'G1:Option 2');
222+
const focused2 = await page.evaluate(
223+
() => (document.activeElement as HTMLInputElement)?.value
224+
);
225+
expect(focused2).toBe('G1:Option 2');
226+
227+
// CRITICAL TEST: Navigate from G1:Option 2 to G2:Option 1
228+
// This should skip the "Option group 2" title and focus on G2:Option 1
229+
// This is the core issue that was fixed in #4920
230+
await page.keyboard.press('ArrowDown');
231+
await waitForFocusChange(page, 'G2:Option 1');
232+
const focused3 = await page.evaluate(
233+
() => (document.activeElement as HTMLInputElement)?.value
234+
);
235+
expect(focused3).toBe('G2:Option 1'); // This was previously broken
236+
237+
// Continue to G2:Option 2
238+
await page.keyboard.press('ArrowDown');
239+
await waitForFocusChange(page, 'G2:Option 2');
240+
const focused4 = await page.evaluate(
241+
() => (document.activeElement as HTMLInputElement)?.value
242+
);
243+
expect(focused4).toBe('G2:Option 2');
244+
245+
// Test reverse navigation
246+
await page.keyboard.press('ArrowUp');
247+
await waitForFocusChange(page, 'G2:Option 1');
248+
const focused5 = await page.evaluate(
249+
() => (document.activeElement as HTMLInputElement)?.value
250+
);
251+
expect(focused5).toBe('G2:Option 1');
252+
253+
await page.keyboard.press('ArrowUp');
254+
await waitForFocusChange(page, 'G1:Option 2');
255+
const focused6 = await page.evaluate(
256+
() => (document.activeElement as HTMLInputElement)?.value
257+
);
258+
expect(focused6).toBe('G1:Option 2');
259+
});
260+
261+
test('custom removeTagsTexts should correspond to correct options', async ({
262+
mount
263+
}) => {
164264
const component = await mount(tagSelectWithCustomRemoveTexts);
165-
265+
166266
// Should have tags for Blue and Green (selected values)
167267
const tags = component.locator('.db-tag');
168268
await expect(tags).toHaveCount(2);
169-
269+
170270
// Get the remove buttons and their tooltip text
171-
const removeButtons = component.locator('.db-tag .db-tab-remove-button');
271+
const removeButtons = component.locator(
272+
'.db-tag .db-tab-remove-button'
273+
);
172274
await expect(removeButtons).toHaveCount(2);
173-
275+
174276
// The first selected option is 'Blue' (index 1 in options array)
175277
// Should have 'Remove Blue Color' tooltip
176278
const firstRemoveButton = removeButtons.first();
177279
const firstTooltip = firstRemoveButton.locator('.db-tooltip');
178280
await expect(firstTooltip).toContainText('Remove Blue Color');
179-
180-
// The second selected option is 'Green' (index 2 in options array)
281+
282+
// The second selected option is 'Green' (index 2 in options array)
181283
// Should have 'Remove Green Color' tooltip
182284
const secondRemoveButton = removeButtons.last();
183285
const secondTooltip = secondRemoveButton.locator('.db-tooltip');

0 commit comments

Comments
 (0)