From f7f1753d0b8179593f08c025c4e8e80117bf1f5d Mon Sep 17 00:00:00 2001 From: Matt Driscoll Date: Fri, 2 Jun 2023 14:04:21 -0700 Subject: [PATCH 1/3] fix(radio-button, radio-button-group): emit change events only when checked is toggled. #6633 --- .../radio-button-group.e2e.ts | 6 ++-- .../radio-button/radio-button.e2e.ts | 28 +++++++++++++++++++ src/components/radio-button/radio-button.tsx | 4 +-- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/components/radio-button-group/radio-button-group.e2e.ts b/src/components/radio-button-group/radio-button-group.e2e.ts index d911653cac1..cc10af40754 100644 --- a/src/components/radio-button-group/radio-button-group.e2e.ts +++ b/src/components/radio-button-group/radio-button-group.e2e.ts @@ -441,15 +441,15 @@ describe("calcite-radio-button-group", () => { expect(changeEvent).toHaveReceivedEventTimes(0); await firstRadio.click(); - expect(changeEvent).toHaveReceivedEventTimes(1); + expect(changeEvent).toHaveReceivedEventTimes(0); expect(await getSelectedItemValue()).toBe("one"); await secondRadio.click(); - expect(changeEvent).toHaveReceivedEventTimes(2); + expect(changeEvent).toHaveReceivedEventTimes(1); expect(await getSelectedItemValue()).toBe("two"); await thirdRadio.click(); - expect(changeEvent).toHaveReceivedEventTimes(3); + expect(changeEvent).toHaveReceivedEventTimes(2); expect(await getSelectedItemValue()).toBe("three"); }); }); diff --git a/src/components/radio-button/radio-button.e2e.ts b/src/components/radio-button/radio-button.e2e.ts index 17e8ba1e9a3..29112c33083 100644 --- a/src/components/radio-button/radio-button.e2e.ts +++ b/src/components/radio-button/radio-button.e2e.ts @@ -10,6 +10,7 @@ import { reflects, renders } from "../../tests/commonTests"; +import { html } from "../../../support/formatting"; describe("calcite-radio-button", () => { describe("renders", () => { @@ -183,6 +184,33 @@ describe("calcite-radio-button", () => { expect(value).toBe("1"); }); + it("should not emit calciteRadioButtonChange when checked already", async () => { + const page = await newE2EPage(); + await page.setContent(html` + + + Trees + + + + Shrubs + + `); + + const checkedRadio = await page.find("calcite-radio-button[checked]"); + expect(await checkedRadio.getProperty("checked")).toBe(true); + + const changeEvent = await checkedRadio.spyOnEvent("calciteRadioButtonChange"); + + expect(changeEvent).toHaveReceivedEventTimes(0); + + await checkedRadio.click(); + await page.waitForChanges(); + expect(await checkedRadio.getProperty("checked")).toBe(true); + + expect(changeEvent).toHaveReceivedEventTimes(0); + }); + it("clicking a radio updates its checked status", async () => { const page = await newE2EPage(); await page.setContent(` diff --git a/src/components/radio-button/radio-button.tsx b/src/components/radio-button/radio-button.tsx index f91f42fa86d..25a31789802 100644 --- a/src/components/radio-button/radio-button.tsx +++ b/src/components/radio-button/radio-button.tsx @@ -185,7 +185,7 @@ export class RadioButton }; check = (): void => { - if (this.disabled) { + if (this.disabled || this.checked) { return; } this.uncheckAllRadioButtonsInGroup(); @@ -204,7 +204,7 @@ export class RadioButton }; onLabelClick(event: CustomEvent): void { - if (!this.disabled && !this.hidden) { + if (!this.disabled && !this.hidden && !this.checked) { this.uncheckOtherRadioButtonsInGroup(); const label = event.currentTarget as HTMLCalciteLabelElement; const radioButton = label.for From 1981ebcb0b7901a5b8a3a6e6d0640e749d39a335 Mon Sep 17 00:00:00 2001 From: Matt Driscoll Date: Fri, 2 Jun 2023 14:13:24 -0700 Subject: [PATCH 2/3] cleanup --- src/components/radio-button/radio-button.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/components/radio-button/radio-button.tsx b/src/components/radio-button/radio-button.tsx index 25a31789802..24acfc56491 100644 --- a/src/components/radio-button/radio-button.tsx +++ b/src/components/radio-button/radio-button.tsx @@ -204,8 +204,7 @@ export class RadioButton }; onLabelClick(event: CustomEvent): void { - if (!this.disabled && !this.hidden && !this.checked) { - this.uncheckOtherRadioButtonsInGroup(); + if (!this.disabled && !this.hidden) { const label = event.currentTarget as HTMLCalciteLabelElement; const radioButton = label.for ? this.rootNode.querySelector( @@ -215,6 +214,12 @@ export class RadioButton `calcite-radio-button[name="${this.name}"]` ); + if (radioButton?.checked) { + return; + } + + this.uncheckOtherRadioButtonsInGroup(); + if (radioButton) { radioButton.checked = true; radioButton.focused = true; From e2d6368c0773773872c382ec368f4d93d898fb78 Mon Sep 17 00:00:00 2001 From: Matt Driscoll Date: Fri, 2 Jun 2023 14:24:03 -0700 Subject: [PATCH 3/3] cleanup/refactoring --- src/components/radio-button/radio-button.tsx | 56 ++++++++++++-------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/components/radio-button/radio-button.tsx b/src/components/radio-button/radio-button.tsx index 24acfc56491..beef04e3f75 100644 --- a/src/components/radio-button/radio-button.tsx +++ b/src/components/radio-button/radio-button.tsx @@ -185,14 +185,20 @@ export class RadioButton }; check = (): void => { - if (this.disabled || this.checked) { + if (this.disabled) { + return; + } + + this.focused = true; + this.setFocus(); + + if (this.checked) { return; } + this.uncheckAllRadioButtonsInGroup(); this.checked = true; - this.focused = true; this.calciteRadioButtonChange.emit(); - this.setFocus(); }; private clickHandler = (): void => { @@ -204,30 +210,34 @@ export class RadioButton }; onLabelClick(event: CustomEvent): void { - if (!this.disabled && !this.hidden) { - const label = event.currentTarget as HTMLCalciteLabelElement; - const radioButton = label.for - ? this.rootNode.querySelector( - `calcite-radio-button[id="${label.for}"]` - ) - : label.querySelector( - `calcite-radio-button[name="${this.name}"]` - ); - - if (radioButton?.checked) { - return; - } + if (this.disabled || this.hidden) { + return; + } - this.uncheckOtherRadioButtonsInGroup(); + const label = event.currentTarget as HTMLCalciteLabelElement; - if (radioButton) { - radioButton.checked = true; - radioButton.focused = true; - } + const radioButton = label.for + ? this.rootNode.querySelector( + `calcite-radio-button[id="${label.for}"]` + ) + : label.querySelector( + `calcite-radio-button[name="${this.name}"]` + ); - this.calciteRadioButtonChange.emit(); - this.setFocus(); + if (!radioButton) { + return; } + + radioButton.focused = true; + this.setFocus(); + + if (radioButton.checked) { + return; + } + + this.uncheckOtherRadioButtonsInGroup(); + radioButton.checked = true; + this.calciteRadioButtonChange.emit(); } private checkLastRadioButton(): void {