From c299d3666aae96d0e67ce4d2c70efbe95bee81da Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Mon, 7 Nov 2022 18:03:08 -0500 Subject: [PATCH] feat(radio-group): ionChange will only emit from user committed changes (#26223) --- BREAKING.md | 5 + core/src/components.d.ts | 4 + .../radio-group/radio-group-interface.ts | 1 + .../components/radio-group/radio-group.tsx | 42 ++++++- .../radio-group/test/basic/radio-group.e2e.ts | 32 ++++++ .../radio-group/test/form/radio-group.e2e.ts | 2 +- .../test/radio-group-events.e2e.ts | 104 ++++++++++++++++++ core/src/components/radio/radio.tsx | 4 +- .../select/test/basic/select.e2e.ts | 2 +- packages/vue/src/proxies.ts | 3 +- 10 files changed, 189 insertions(+), 10 deletions(-) create mode 100644 core/src/components/radio-group/test/radio-group-events.e2e.ts diff --git a/BREAKING.md b/BREAKING.md index 9eb81a1721..9a07f10aa2 100644 --- a/BREAKING.md +++ b/BREAKING.md @@ -21,6 +21,7 @@ This is a comprehensive list of the breaking changes introduced in the major ver - [Input](#version-7x-input) - [Modal](#version-7x-modal) - [Overlays](#version-7x-overlays) + - [Radio Group](#version-7x-radio-group) - [Range](#version-7x-range) - [Searchbar](#version-7x-searchbar) - [Segment](#version-7x-segment) @@ -107,6 +108,10 @@ This section details the desktop browser, JavaScript framework, and mobile platf Ionic now listens on the `keydown` event instead of the `keyup` event when determining when to dismiss overlays via the "Escape" key. Any applications that were listening on `keyup` to suppress this behavior should listen on `keydown` instead. +

Radio Group

+ +- `ionChange` is no longer emitted when the `value` of `ion-radio-group` is modified externally. `ionChange` is only emitted from user committed changes, such as clicking or tapping an `ion-radio` in the group. +

Range

- Range is updated to align with the design specification for supported modes. diff --git a/core/src/components.d.ts b/core/src/components.d.ts index 57b4ab7c41..bbcd88d5cd 100644 --- a/core/src/components.d.ts +++ b/core/src/components.d.ts @@ -5908,6 +5908,10 @@ declare namespace LocalJSX { * Emitted when the value has changed. */ "onIonChange"?: (event: IonRadioGroupCustomEvent) => void; + /** + * Emitted when the `value` property has changed. This is used to ensure that `ion-radio` can respond to any value property changes from the group. + */ + "onIonValueChange"?: (event: IonRadioGroupCustomEvent) => void; /** * the value of the radio group. */ diff --git a/core/src/components/radio-group/radio-group-interface.ts b/core/src/components/radio-group/radio-group-interface.ts index 5d2c032532..3aea460288 100644 --- a/core/src/components/radio-group/radio-group-interface.ts +++ b/core/src/components/radio-group/radio-group-interface.ts @@ -1,5 +1,6 @@ export interface RadioGroupChangeEventDetail { value: T; + event?: Event; } export interface RadioGroupCustomEvent extends CustomEvent { diff --git a/core/src/components/radio-group/radio-group.tsx b/core/src/components/radio-group/radio-group.tsx index 8dbdcaba87..58a1e3272a 100644 --- a/core/src/components/radio-group/radio-group.tsx +++ b/core/src/components/radio-group/radio-group.tsx @@ -38,8 +38,7 @@ export class RadioGroup implements ComponentInterface { @Watch('value') valueChanged(value: any | undefined) { this.setRadioTabindex(value); - - this.ionChange.emit({ value }); + this.ionValueChange.emit({ value }); } /** @@ -47,6 +46,15 @@ export class RadioGroup implements ComponentInterface { */ @Event() ionChange!: EventEmitter; + /** + * Emitted when the `value` property has changed. + * This is used to ensure that `ion-radio` can respond + * to any value property changes from the group. + * + * @internal + */ + @Event() ionValueChange!: EventEmitter; + componentDidLoad() { this.setRadioTabindex(this.value); } @@ -88,6 +96,17 @@ export class RadioGroup implements ComponentInterface { return Array.from(this.el.querySelectorAll('ion-radio')); } + /** + * Emits an `ionChange` event. + * + * This API should be called for user committed changes. + * This API should not be used for external value changes. + */ + private emitValueChange(event?: Event) { + const { value } = this; + this.ionChange.emit({ value, event }); + } + private onClick = (ev: Event) => { ev.preventDefault(); @@ -97,17 +116,19 @@ export class RadioGroup implements ComponentInterface { const newValue = selectedRadio.value; if (newValue !== currentValue) { this.value = newValue; + this.emitValueChange(ev); } else if (this.allowEmptySelection) { this.value = undefined; + this.emitValueChange(ev); } } }; @Listen('keydown', { target: 'document' }) - onKeydown(ev: any) { + onKeydown(ev: KeyboardEvent) { const inSelectPopover = !!this.el.closest('ion-select-popover'); - if (ev.target && !this.el.contains(ev.target)) { + if (ev.target && !this.el.contains(ev.target as HTMLElement)) { return; } @@ -116,7 +137,7 @@ export class RadioGroup implements ComponentInterface { const radios = this.getRadios().filter((radio) => !radio.disabled); // Only move the radio if the current focus is in the radio group - if (ev.target && radios.includes(ev.target)) { + if (ev.target && radios.includes(ev.target as HTMLIonRadioElement)) { const index = radios.findIndex((radio) => radio === ev.target); const current = radios[index]; @@ -139,13 +160,24 @@ export class RadioGroup implements ComponentInterface { if (!inSelectPopover) { this.value = next.value; + this.emitValueChange(ev); } } // Update the radio group value when a user presses the // space bar on top of a selected radio if (['Space'].includes(ev.code)) { + const previousValue = this.value; this.value = this.allowEmptySelection && this.value !== undefined ? undefined : current.value; + if (previousValue !== this.value || this.allowEmptySelection) { + /** + * Value change should only be emitted if the value is different, + * such as selecting a new radio with the space bar or if + * the radio group allows for empty selection and the user + * is deselecting a checked radio. + */ + this.emitValueChange(ev); + } // Prevent browsers from jumping // to the bottom of the screen diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts b/core/src/components/radio-group/test/basic/radio-group.e2e.ts index 106ea7dcb2..ee516225b0 100644 --- a/core/src/components/radio-group/test/basic/radio-group.e2e.ts +++ b/core/src/components/radio-group/test/basic/radio-group.e2e.ts @@ -76,6 +76,38 @@ test.describe('radio-group: interaction', () => { await radioFixture.checkRadio('mouse'); await radioFixture.expectChecked(false); }); + + test('programmatically assigning a value should update the checked radio', async ({ page }) => { + await page.setContent(` + + + Item 1 + + + + + Item 2 + + + + + Item 3 + + + + `); + + const radioGroup = page.locator('ion-radio-group'); + const radioOne = page.locator('ion-radio[value="1"]'); + const radioTwo = page.locator('ion-radio[value="2"]'); + + await radioGroup.evaluate((el: HTMLIonRadioGroupElement) => (el.value = '2')); + + await page.waitForChanges(); + + expect(radioOne).not.toHaveClass(/radio-checked/); + expect(radioTwo).toHaveClass(/radio-checked/); + }); }); class RadioFixture { diff --git a/core/src/components/radio-group/test/form/radio-group.e2e.ts b/core/src/components/radio-group/test/form/radio-group.e2e.ts index d6a833e6e6..b563700240 100644 --- a/core/src/components/radio-group/test/form/radio-group.e2e.ts +++ b/core/src/components/radio-group/test/form/radio-group.e2e.ts @@ -16,7 +16,7 @@ test.describe('radio-group: form', () => { await griffRadio.click(); await page.waitForChanges(); - await expect(ionChange).toHaveReceivedEventDetail({ value: 'griff' }); + await expect(ionChange).toHaveReceivedEventDetail({ value: 'griff', event: { isTrusted: true } }); }); test('selecting a disabled option should not update the value', async ({ page }) => { diff --git a/core/src/components/radio-group/test/radio-group-events.e2e.ts b/core/src/components/radio-group/test/radio-group-events.e2e.ts new file mode 100644 index 0000000000..9c004b3162 --- /dev/null +++ b/core/src/components/radio-group/test/radio-group-events.e2e.ts @@ -0,0 +1,104 @@ +import { expect } from '@playwright/test'; +import { test } from '@utils/test/playwright'; + +test.describe('radio group: events: ionChange', () => { + test.beforeEach(({ skip }) => { + skip.rtl(); + skip.mode('md'); + }); + + test('should emit when selecting an unchecked radio', async ({ page }) => { + await page.setContent(` + + + + + + `); + + const ionChangeSpy = await page.spyOnEvent('ionChange'); + + await page.click('ion-radio[value="2"]'); + + await ionChangeSpy.next(); + + expect(ionChangeSpy).toHaveReceivedEventTimes(1); + expect(ionChangeSpy).toHaveReceivedEventDetail({ value: '2', event: { isTrusted: true } }); + }); + + test('should emit when the radio group does not have an initial value', async ({ page }) => { + await page.setContent(` + + + + + + `); + + const ionChangeSpy = await page.spyOnEvent('ionChange'); + + await page.click('ion-radio[value="2"]'); + + await ionChangeSpy.next(); + + expect(ionChangeSpy).toHaveReceivedEventTimes(1); + expect(ionChangeSpy).toHaveReceivedEventDetail({ value: '2', event: { isTrusted: true } }); + }); + + test('should not emit when selecting a checked radio', async ({ page }) => { + await page.setContent(` + + + + + + `); + + const ionChangeSpy = await page.spyOnEvent('ionChange'); + + await page.click('ion-radio[value="1"]'); + + await page.waitForChanges(); + + expect(ionChangeSpy).toHaveReceivedEventTimes(0); + }); + + test('should not emit if the value is set programmatically', async ({ page }) => { + await page.setContent(` + + + + + + `); + + const radioGroup = page.locator('ion-radio-group'); + const ionChangeSpy = await page.spyOnEvent('ionChange'); + + await radioGroup.evaluate((el: HTMLIonRadioGroupElement) => (el.value = '2')); + + expect(ionChangeSpy).toHaveReceivedEventTimes(0); + expect(await radioGroup.evaluate((el: HTMLIonRadioGroupElement) => el.value)).toBe('2'); + }); + + test.describe('allowEmptySelection', () => { + test('should emit when selecting a checked radio', async ({ page }) => { + await page.setContent(` + + + + + + `); + + const ionChangeSpy = await page.spyOnEvent('ionChange'); + + await page.click('ion-radio[value="1"]'); + + await ionChangeSpy.next(); + + expect(ionChangeSpy).toHaveReceivedEventTimes(1); + expect(ionChangeSpy).toHaveReceivedEventDetail({ value: undefined, event: { isTrusted: true } }); + }); + }); +}); diff --git a/core/src/components/radio/radio.tsx b/core/src/components/radio/radio.tsx index 9a0a3d97e5..666fb4732b 100644 --- a/core/src/components/radio/radio.tsx +++ b/core/src/components/radio/radio.tsx @@ -98,14 +98,14 @@ export class Radio implements ComponentInterface { const radioGroup = (this.radioGroup = this.el.closest('ion-radio-group')); if (radioGroup) { this.updateState(); - addEventListener(radioGroup, 'ionChange', this.updateState); + addEventListener(radioGroup, 'ionValueChange', this.updateState); } } disconnectedCallback() { const radioGroup = this.radioGroup; if (radioGroup) { - removeEventListener(radioGroup, 'ionChange', this.updateState); + removeEventListener(radioGroup, 'ionValueChange', this.updateState); this.radioGroup = null; } } diff --git a/core/src/components/select/test/basic/select.e2e.ts b/core/src/components/select/test/basic/select.e2e.ts index 81df203e53..6157b993d4 100644 --- a/core/src/components/select/test/basic/select.e2e.ts +++ b/core/src/components/select/test/basic/select.e2e.ts @@ -149,7 +149,7 @@ test.describe('select: ionChange', () => { await radioButtons.nth(0).click(); await ionChange.next(); - expect(ionChange).toHaveReceivedEventDetail({ value: 'apple' }); + expect(ionChange).toHaveReceivedEventDetail({ value: 'apple', event: { isTrusted: true } }); }); test('should fire ionChange when confirming a value from an action sheet', async ({ page }) => { diff --git a/packages/vue/src/proxies.ts b/packages/vue/src/proxies.ts index 38214f38e7..d8f539a999 100644 --- a/packages/vue/src/proxies.ts +++ b/packages/vue/src/proxies.ts @@ -580,7 +580,8 @@ export const IonRadioGroup = /*@__PURE__*/ defineContainer('i 'allowEmptySelection', 'name', 'value', - 'ionChange' + 'ionChange', + 'ionValueChange' ], 'value', 'v-ion-change', 'ionChange');