Skip to content

Commit 55a8cf3

Browse files
ShaneKShaneK
committed
fix(select): commit value and dismiss on Enter for popover and modal interfaces
1 parent 0db5b40 commit 55a8cf3

10 files changed

Lines changed: 267 additions & 12 deletions

File tree

‎core/src/components/radio-group/radio-group.tsx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,19 @@ export class RadioGroup implements ComponentInterface {
290290
// to the bottom of the screen
291291
ev.preventDefault();
292292
}
293+
294+
// Inside a select interface, Enter commits the focused radio
295+
// value (matching native <select>). The !ev.repeat guard stops
296+
// a held Enter on the triggering ion-select from re-committing
297+
// once focus lands in the opened popover/modal.
298+
if (ev.key === 'Enter' && inSelectInterface && !ev.repeat) {
299+
const previousValue = this.value;
300+
this.value = current.value;
301+
if (previousValue !== this.value) {
302+
this.emitValueChange(ev);
303+
}
304+
ev.preventDefault();
305+
}
293306
}
294307
}
295308

‎core/src/components/radio-group/test/basic/radio-group.e2e.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,32 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
4646
await radioFixture.expectChecked(false);
4747
});
4848

49+
test('Enter outside a select interface should not change the value', async ({ page }, testInfo) => {
50+
testInfo.annotations.push({
51+
type: 'issue',
52+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
53+
});
54+
55+
await page.setContent(
56+
`
57+
<ion-radio-group value="one">
58+
<ion-item>
59+
<ion-radio id="one" value="one">One</ion-radio>
60+
</ion-item>
61+
<ion-item>
62+
<ion-radio id="two" value="two">Two</ion-radio>
63+
</ion-item>
64+
</ion-radio-group>
65+
`,
66+
config
67+
);
68+
69+
await radioFixture.checkRadio('keyboard', 'ion-radio#two', 'Enter');
70+
71+
const group = page.locator('ion-radio-group');
72+
await expect(group).toHaveJSProperty('value', 'one');
73+
});
74+
4975
test('click should not deselect without allowEmptySelection', async ({ page }) => {
5076
await page.setContent(
5177
`

‎core/src/components/radio-group/test/fixtures.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,13 @@ export class RadioFixture {
1111
this.page = page;
1212
}
1313

14-
async checkRadio(method: 'keyboard' | 'mouse', selector = 'ion-radio') {
14+
async checkRadio(method: 'keyboard' | 'mouse', selector = 'ion-radio', key: 'Space' | 'Enter' = 'Space') {
1515
const { page } = this;
1616
const radio = (this.radio = page.locator(selector));
1717

1818
if (method === 'keyboard') {
1919
await radio.focus();
20-
await page.keyboard.press('Space');
20+
await page.keyboard.press(key);
2121
} else {
2222
await radio.click();
2323
}

‎core/src/components/select-modal/select-modal.tsx‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ import type { SelectModalOption } from './select-modal-interface';
2121
export class SelectModal implements ComponentInterface {
2222
@Element() el!: HTMLIonSelectModalElement;
2323

24+
// Tracks the option that received Enter-keydown so keyup only
25+
// dismisses when the press started on the same option. Prevents
26+
// Enter on the triggering ion-select from auto-dismissing.
27+
private pendingEnterTarget: HTMLElement | null = null;
28+
2429
@Prop() header?: string;
2530

2631
/**
@@ -99,14 +104,21 @@ export class SelectModal implements ComponentInterface {
99104
justify="start"
100105
labelPlacement="end"
101106
onClick={() => this.closeModal()}
107+
onKeyDown={(ev) => {
108+
if (ev.key === 'Enter' && !ev.repeat) {
109+
this.pendingEnterTarget = ev.currentTarget as HTMLElement;
110+
}
111+
}}
102112
onKeyUp={(ev) => {
103113
if (ev.key === ' ') {
104-
/**
105-
* Selecting a radio option with keyboard navigation,
106-
* either through the Enter or Space keys, should
107-
* dismiss the modal.
108-
*/
114+
// Space selects and dismisses in one press.
109115
this.closeModal();
116+
} else if (ev.key === 'Enter') {
117+
const shouldClose = this.pendingEnterTarget === ev.currentTarget;
118+
this.pendingEnterTarget = null;
119+
if (shouldClose) {
120+
this.closeModal();
121+
}
110122
}
111123
}}
112124
>

‎core/src/components/select-modal/test/basic/select-modal.e2e.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,35 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {
6464
await expect(selectModalPage.modal).not.toBeVisible();
6565
});
6666

67+
// eslint-disable-next-line @typescript-eslint/no-unused-vars
68+
test('pressing Enter on an unselected option should dismiss the modal', async ({ page: _page }, testInfo) => {
69+
testInfo.annotations.push({
70+
type: 'issue',
71+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
72+
});
73+
74+
await selectModalPage.setup(config, options, false);
75+
76+
await selectModalPage.pressEnterOnOption('apple');
77+
await selectModalPage.ionModalDidDismiss.next();
78+
await expect(selectModalPage.modal).not.toBeVisible();
79+
});
80+
81+
test('pressing Enter on a selected option should dismiss the modal', async ({ browserName }, testInfo) => {
82+
testInfo.annotations.push({
83+
type: 'issue',
84+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
85+
});
86+
87+
test.skip(browserName === 'firefox', 'Same behavior as ROU-5437');
88+
89+
await selectModalPage.setup(config, checkedOptions, false);
90+
91+
await selectModalPage.pressEnterOnOption('apple');
92+
await selectModalPage.ionModalDidDismiss.next();
93+
await expect(selectModalPage.modal).not.toBeVisible();
94+
});
95+
6796
test('clicking the close button should dismiss the modal', async () => {
6897
await selectModalPage.setup(config, options, false);
6998

‎core/src/components/select-modal/test/fixtures.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ export class SelectModalPage {
6363
await option.press('Space');
6464
}
6565

66+
async pressEnterOnOption(value: string) {
67+
const option = this.getOption(value);
68+
await option.press('Enter');
69+
}
70+
6671
private getOption(value: string) {
6772
const { multiple, selectModal } = this;
6873
const selector = multiple ? 'ion-checkbox' : 'ion-radio';

‎core/src/components/select-popover/select-popover.tsx‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ import type { SelectPopoverOption } from './select-popover-interface';
2222
})
2323
export class SelectPopover implements ComponentInterface {
2424
@Element() el!: HTMLIonSelectPopoverElement;
25+
26+
// Tracks the option that received Enter-keydown so keyup only
27+
// dismisses when the press started on the same option. Prevents
28+
// Enter on the triggering ion-select from auto-dismissing.
29+
private pendingEnterTarget: HTMLElement | null = null;
30+
2531
/**
2632
* The header text of the popover
2733
*/
@@ -159,14 +165,21 @@ export class SelectPopover implements ComponentInterface {
159165
value={option.value}
160166
disabled={option.disabled}
161167
onClick={() => this.dismissParentPopover()}
168+
onKeyDown={(ev) => {
169+
if (ev.key === 'Enter' && !ev.repeat) {
170+
this.pendingEnterTarget = ev.currentTarget as HTMLElement;
171+
}
172+
}}
162173
onKeyUp={(ev) => {
163174
if (ev.key === ' ') {
164-
/**
165-
* Selecting a radio option with keyboard navigation,
166-
* either through the Enter or Space keys, should
167-
* dismiss the popover.
168-
*/
175+
// Space selects and dismisses in one press.
169176
this.dismissParentPopover();
177+
} else if (ev.key === 'Enter') {
178+
const shouldDismiss = this.pendingEnterTarget === ev.currentTarget;
179+
this.pendingEnterTarget = null;
180+
if (shouldDismiss) {
181+
this.dismissParentPopover();
182+
}
170183
}
171184
}}
172185
>

‎core/src/components/select-popover/test/basic/select-popover.e2e.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,35 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {
6363
await selectPopoverPage.ionPopoverDidDismiss.next();
6464
await expect(selectPopoverPage.popover).not.toBeVisible();
6565
});
66+
67+
// eslint-disable-next-line @typescript-eslint/no-unused-vars
68+
test('pressing Enter on an unselected option should dismiss the popover', async ({ page: _page }, testInfo) => {
69+
testInfo.annotations.push({
70+
type: 'issue',
71+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
72+
});
73+
74+
await selectPopoverPage.setup(config, options, false);
75+
76+
await selectPopoverPage.pressEnterOnOption('apple');
77+
await selectPopoverPage.ionPopoverDidDismiss.next();
78+
await expect(selectPopoverPage.popover).not.toBeVisible();
79+
});
80+
81+
test('pressing Enter on a selected option should dismiss the popover', async ({ browserName }, testInfo) => {
82+
testInfo.annotations.push({
83+
type: 'issue',
84+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
85+
});
86+
87+
test.skip(browserName === 'firefox', 'Same behavior as ROU-5437');
88+
89+
await selectPopoverPage.setup(config, checkedOptions, false);
90+
91+
await selectPopoverPage.pressEnterOnOption('apple');
92+
await selectPopoverPage.ionPopoverDidDismiss.next();
93+
await expect(selectPopoverPage.popover).not.toBeVisible();
94+
});
6695
});
6796
});
6897
});

‎core/src/components/select-popover/test/fixtures.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ export class SelectPopoverPage {
6363
await option.press('Space');
6464
}
6565

66+
async pressEnterOnOption(value: string) {
67+
const option = this.getOption(value);
68+
await option.press('Enter');
69+
}
70+
6671
private getOption(value: string) {
6772
const { multiple, selectPopover } = this;
6873
const selector = multiple ? 'ion-checkbox' : 'ion-radio';

‎core/src/components/select/test/basic/select.e2e.ts‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,86 @@ configs({ directions: ['ltr'] }).forEach(({ title, config, screenshot }) => {
281281
const popover = page.locator('ion-popover');
282282
await expect(popover).toHaveScreenshot(screenshot(`select-basic-popover-scroll-to-selected`));
283283
});
284+
285+
test('opening a popover with Enter should not immediately dismiss it', async ({ page, skip }, testInfo) => {
286+
// TODO (ROU-5437)
287+
skip.browser('webkit', 'Safari 16 only allows text fields and pop-up menus to be focused.');
288+
289+
testInfo.annotations.push({
290+
type: 'issue',
291+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
292+
});
293+
294+
await page.setContent(
295+
`
296+
<ion-app>
297+
<ion-select aria-label="Fruit" interface="popover">
298+
<ion-select-option value="apple">Apple</ion-select-option>
299+
<ion-select-option value="banana">Banana</ion-select-option>
300+
</ion-select>
301+
</ion-app>
302+
`,
303+
config
304+
);
305+
306+
const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
307+
const ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss');
308+
309+
await page.locator('ion-select button').focus();
310+
await page.keyboard.press('Enter');
311+
await ionPopoverDidPresent.next();
312+
313+
const popover = page.locator('ion-popover');
314+
await expect(popover).toBeVisible();
315+
316+
await page.waitForChanges();
317+
expect(ionPopoverDidDismiss).toHaveReceivedEventTimes(0);
318+
await expect(popover).toBeVisible();
319+
});
320+
321+
test('holding Enter to open a popover should not immediately dismiss it', async ({ page, skip }, testInfo) => {
322+
// TODO (ROU-5437)
323+
skip.browser('webkit', 'Safari 16 only allows text fields and pop-up menus to be focused.');
324+
325+
testInfo.annotations.push({
326+
type: 'issue',
327+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
328+
});
329+
330+
await page.setContent(
331+
`
332+
<ion-app>
333+
<ion-select aria-label="Fruit" interface="popover">
334+
<ion-select-option value="apple">Apple</ion-select-option>
335+
<ion-select-option value="banana">Banana</ion-select-option>
336+
</ion-select>
337+
</ion-app>
338+
`,
339+
config
340+
);
341+
342+
const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
343+
const ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss');
344+
const select = page.locator('ion-select') as E2ELocator;
345+
const ionChange = await select.spyOnEvent('ionChange');
346+
347+
await page.locator('ion-select button').focus();
348+
await page.keyboard.down('Enter');
349+
await ionPopoverDidPresent.next();
350+
351+
const popover = page.locator('ion-popover');
352+
await expect(popover).toBeVisible();
353+
354+
// Second down('Enter') fires a repeat keydown (repeat=true),
355+
// which is the path guarded against in radio-group.
356+
await page.keyboard.down('Enter');
357+
await page.keyboard.up('Enter');
358+
await page.waitForChanges();
359+
360+
expect(ionPopoverDidDismiss).toHaveReceivedEventTimes(0);
361+
expect(ionChange).toHaveReceivedEventTimes(0);
362+
await expect(popover).toBeVisible();
363+
});
284364
});
285365

286366
test.describe('select: modal', () => {
@@ -450,6 +530,49 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {
450530
expect(ionChange).toHaveReceivedEventTimes(1);
451531
});
452532

533+
test('should fire ionChange when confirming a popover value with Enter', async ({ page, skip }, testInfo) => {
534+
// TODO (ROU-5437)
535+
skip.browser('webkit', 'Safari 16 only allows text fields and pop-up menus to be focused.');
536+
537+
testInfo.annotations.push({
538+
type: 'issue',
539+
description: 'https://github.com/ionic-team/ionic-framework/issues/30561',
540+
});
541+
542+
await page.setContent(
543+
`
544+
<ion-app>
545+
<ion-select aria-label="Fruit" interface="popover">
546+
<ion-select-option value="apple">Apple</ion-select-option>
547+
<ion-select-option value="banana">Banana</ion-select-option>
548+
</ion-select>
549+
</ion-app>
550+
`,
551+
config
552+
);
553+
554+
const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
555+
const ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss');
556+
const select = page.locator('ion-select') as E2ELocator;
557+
const ionChange = await select.spyOnEvent('ionChange');
558+
559+
await select.click();
560+
await ionPopoverDidPresent.next();
561+
562+
const popover = page.locator('ion-popover');
563+
const secondRadio = popover.locator('ion-radio').nth(1);
564+
565+
await secondRadio.focus();
566+
await page.keyboard.press('Enter');
567+
568+
await ionChange.next();
569+
await ionPopoverDidDismiss.next();
570+
571+
expect(ionChange).toHaveReceivedEventDetail({ value: 'banana' });
572+
expect(ionChange).toHaveReceivedEventTimes(1);
573+
await expect(popover).not.toBeVisible();
574+
});
575+
453576
test('should fire ionChange when confirming a value from a popover', async ({ page }) => {
454577
await page.setContent(
455578
`

0 commit comments

Comments
 (0)