From 1ef8e1cf739017cb3e6c230088f2c9045f3b74b9 Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Thu, 8 Jun 2023 15:45:34 -0400 Subject: [PATCH] fix: radio group race condition (#1364) * fix: radio group race condition * update changelog * prettier * fix changelog --- docs/pages/resources/changelog.md | 1 + .../radio-group/radio-group.test.ts | 31 ++++++++++ src/components/radio-group/radio-group.ts | 61 ++++++++++++------- 3 files changed, 71 insertions(+), 22 deletions(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 1634361b5..0439313f5 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -19,6 +19,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti - Added tests for `` [#1343](https://github.com/shoelace-style/shoelace/pull/1343) - Fixed a bug where changing the size of `` wouldn't update the size of child elements - Fixed a bug in `` and `` where the `size` attribute wasn't being reflected [#1318](https://github.com/shoelace-style/shoelace/issues/1348) +- Fixed a bug in `` where `` would not get checked if `` was defined first. [#1364](https://github.com/shoelace-style/shoelace/pull/1364/) - Improved `` so it can accept children of variable heights [#1317](https://github.com/shoelace-style/shoelace/pull/1317) - Improved the docs to more clearly explain sizing radios and radio buttons - Improved the performance of `` by partially rendering unseen icons [#1310](https://github.com/shoelace-style/shoelace/pull/1310) diff --git a/src/components/radio-group/radio-group.test.ts b/src/components/radio-group/radio-group.test.ts index 760eca0bb..72d5da53b 100644 --- a/src/components/radio-group/radio-group.test.ts +++ b/src/components/radio-group/radio-group.test.ts @@ -365,5 +365,36 @@ describe('when the value changes', () => { await radioGroup.updateComplete; }); + /** + * @see https://github.com/shoelace-style/shoelace/issues/1361 + * This isn't really possible to test right now due to importing "shoelace.js" which + * auto-defines all of our components up front. This should be tested if we ever split + * to non-auto-defining components and not auto-defining for tests. + */ + it.skip('should sync up radios and radio buttons if defined after radio group', async () => { + // customElements.define("sl-radio-group", SlRadioGroup) + // + // const radioGroup = await fixture(html` + // + // + // + // + // `); + // + // await aTimeout(1) + // + // customElements.define("sl-radio-button", SlRadioButton) + // + // expect(radioGroup.querySelector("sl-radio")?.getAttribute("aria-checked")).to.equal("false") + // + // await aTimeout(1) + // + // customElements.define("sl-radio", SlRadio) + // + // await aTimeout(1) + // + // expect(radioGroup.querySelector("sl-radio")?.getAttribute("aria-checked")).to.equal("true") + }); + runFormControlBaseTests('sl-radio-group'); }); diff --git a/src/components/radio-group/radio-group.ts b/src/components/radio-group/radio-group.ts index ca6d9a501..6dbe8a33f 100644 --- a/src/components/radio-group/radio-group.ts +++ b/src/components/radio-group/radio-group.ts @@ -204,40 +204,57 @@ export default class SlRadioGroup extends ShoelaceElement implements ShoelaceFor this.formControlController.emitInvalidEvent(event); } - private syncRadios() { - if (customElements.get('sl-radio') || customElements.get('sl-radio-button')) { - const radios = this.getAllRadios(); + private async syncRadioElements() { + const radios = this.getAllRadios(); + await Promise.all( // Sync the checked state and size - radios.forEach(radio => { + radios.map(async radio => { + await radio.updateComplete; radio.checked = radio.value === this.value; radio.size = this.size; - }); + }) + ); - this.hasButtonGroup = radios.some(radio => radio.tagName.toLowerCase() === 'sl-radio-button'); - - if (!radios.some(radio => radio.checked)) { - if (this.hasButtonGroup) { - const buttonRadio = radios[0].shadowRoot?.querySelector('button'); - - if (buttonRadio) { - buttonRadio.tabIndex = 0; - } - } else { - radios[0].tabIndex = 0; - } - } + this.hasButtonGroup = radios.some(radio => radio.tagName.toLowerCase() === 'sl-radio-button'); + if (!radios.some(radio => radio.checked)) { if (this.hasButtonGroup) { - const buttonGroup = this.shadowRoot?.querySelector('sl-button-group'); + const buttonRadio = radios[0].shadowRoot?.querySelector('button'); - if (buttonGroup) { - buttonGroup.disableRole = true; + if (buttonRadio) { + buttonRadio.tabIndex = 0; } + } else { + radios[0].tabIndex = 0; } + } + + if (this.hasButtonGroup) { + const buttonGroup = this.shadowRoot?.querySelector('sl-button-group'); + + if (buttonGroup) { + buttonGroup.disableRole = true; + } + } + } + + private syncRadios() { + if (customElements.get('sl-radio') && customElements.get('sl-radio-button')) { + this.syncRadioElements(); + return; + } + + if (customElements.get('sl-radio')) { + this.syncRadioElements(); + } else { + customElements.whenDefined('sl-radio').then(() => this.syncRadios()); + } + + if (customElements.get('sl-radio-button')) { + this.syncRadioElements(); } else { // Rerun this handler when or is registered - customElements.whenDefined('sl-radio').then(() => this.syncRadios()); customElements.whenDefined('sl-radio-button').then(() => this.syncRadios()); } }