Skip to content

Commit eee2115

Browse files
fix(select): do not focus disabled popover option (#28309)
Issue number: resolves #28284 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> Select focuses the first popover option when no value is provided. This means that the first option is focused even if it disabled. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - Select focuses the first **enabled** popover option when no value is provided. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change, please describe the impact and migration path for existing applications below. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> --------- Co-authored-by: Amanda Johnston <[email protected]>
1 parent d005735 commit eee2115

File tree

2 files changed

+72
-19
lines changed

2 files changed

+72
-19
lines changed

core/src/components/select/select.tsx

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -316,29 +316,46 @@ export class Select implements ComponentInterface {
316316

317317
// focus selected option for popovers
318318
if (this.interface === 'popover') {
319-
let indexOfSelected = this.childOpts.map((o) => o.value).indexOf(this.value);
320-
indexOfSelected = indexOfSelected > -1 ? indexOfSelected : 0; // default to first option if nothing selected
321-
const selectedItem = overlay.querySelector<HTMLElement>(
322-
`.select-interface-option:nth-child(${indexOfSelected + 1})`
323-
);
319+
const indexOfSelected = this.childOpts.map((o) => o.value).indexOf(this.value);
324320

325-
if (selectedItem) {
326-
focusElement(selectedItem);
321+
if (indexOfSelected > -1) {
322+
const selectedItem = overlay.querySelector<HTMLElement>(
323+
`.select-interface-option:nth-child(${indexOfSelected + 1})`
324+
);
327325

326+
if (selectedItem) {
327+
focusElement(selectedItem);
328+
329+
/**
330+
* Browsers such as Firefox do not
331+
* correctly delegate focus when manually
332+
* focusing an element with delegatesFocus.
333+
* We work around this by manually focusing
334+
* the interactive element.
335+
* ion-radio and ion-checkbox are the only
336+
* elements that ion-select-popover uses, so
337+
* we only need to worry about those two components
338+
* when focusing.
339+
*/
340+
const interactiveEl = selectedItem.querySelector<HTMLElement>('ion-radio, ion-checkbox');
341+
if (interactiveEl) {
342+
interactiveEl.focus();
343+
}
344+
}
345+
} else {
328346
/**
329-
* Browsers such as Firefox do not
330-
* correctly delegate focus when manually
331-
* focusing an element with delegatesFocus.
332-
* We work around this by manually focusing
333-
* the interactive element.
334-
* ion-radio and ion-checkbox are the only
335-
* elements that ion-select-popover uses, so
336-
* we only need to worry about those two components
337-
* when focusing.
347+
* If no value is set then focus the first enabled option.
338348
*/
339-
const interactiveEl = selectedItem.querySelector<HTMLElement>('ion-radio, ion-checkbox');
340-
if (interactiveEl) {
341-
interactiveEl.focus();
349+
const firstEnabledOption = overlay.querySelector<HTMLElement>(
350+
'ion-radio:not(.radio-disabled), ion-checkbox:not(.checkbox-disabled)'
351+
);
352+
if (firstEnabledOption) {
353+
focusElement(firstEnabledOption.closest('ion-item')!);
354+
355+
/**
356+
* Focus the option for the same reason as we do above.
357+
*/
358+
firstEnabledOption.focus();
342359
}
343360
}
344361
}
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import { expect } from '@playwright/test';
2+
import { configs, test } from '@utils/test/playwright';
3+
4+
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => {
5+
test.describe(title('select: disabled options'), () => {
6+
test('should not focus a disabled option when no value is set', async ({ page, skip }) => {
7+
// TODO (FW-2979)
8+
skip.browser('webkit', 'Safari 16 only allows text fields and pop-up menus to be focused.');
9+
10+
test.info().annotations.push({
11+
type: 'issue',
12+
description: 'https://github.com/ionic-team/ionic-framework/issues/28284',
13+
});
14+
15+
await page.setContent(
16+
`
17+
<ion-select interface="popover">
18+
<ion-select-option value="a" disabled="true">A</ion-select-option>
19+
<ion-select-option value="b">B</ion-select-option>
20+
</ion-select>
21+
`,
22+
config
23+
);
24+
25+
const select = page.locator('ion-select');
26+
const popover = page.locator('ion-popover');
27+
const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
28+
29+
await select.click();
30+
await ionPopoverDidPresent.next();
31+
32+
const popoverOption = popover.locator('.select-interface-option:nth-of-type(2) ion-radio');
33+
await expect(popoverOption).toBeFocused();
34+
});
35+
});
36+
});

0 commit comments

Comments
 (0)