refactor radio logic

docs/resources/changelog.md

@@ -11,6 +11,7 @@ _During the beta period, these restrictions may be relaxed in the event of a mis
 - Fixed a bug that prevented form submission from working as expected in some cases
 - Fixed a bug that prevented `<sl-split-panel>` from toggling `vertical` properly [#703](https://github.com/shoelace-style/shoelace/issues/703)
 - Fixed a bug that prevented `<sl-color-picker>` from rendering a color initially [#704](https://github.com/shoelace-style/shoelace/issues/704)
+- Refactored `<sl-radio>` to move selection logic into `<sl-radio-group>`
 - Upgraded the status of `<sl-visually-hidden>` from experimental to stable
 
 ## 2.0.0-beta.71

src/components/radio-group/radio-group.ts

@@ -2,6 +2,7 @@ import { html, LitElement } from 'lit';
 import { customElement, property, query } from 'lit/decorators.js';
 import { classMap } from 'lit/directives/class-map.js';
 import type SlRadio from '~/components/radio/radio';
+import { emit } from '~/internal/event';
 import styles from './radio-group.styles';
 
 /**
@@ -26,15 +27,72 @@ export default class SlRadioGroup extends LitElement {
   /** Shows the fieldset and legend that surrounds the radio group. */
   @property({ type: Boolean, attribute: 'fieldset' }) fieldset = false;
 
-  handleFocusIn() {
-    // When tabbing into the fieldset, make sure it lands on the checked radio
-    requestAnimationFrame(() => {
-      const checkedRadio = [...(this.defaultSlot.assignedElements({ flatten: true }) as SlRadio[])].find(
-        el => el.tagName.toLowerCase() === 'sl-radio' && el.checked
-      );
+  connectedCallback() {
+    super.connectedCallback();
+    this.setAttribute('role', 'radiogroup');
+  }
 
-      checkedRadio?.focus();
+  getAllRadios() {
+    return this.defaultSlot
+      .assignedElements({ flatten: true })
+      .filter(el => el.tagName.toLowerCase() === 'sl-radio') as SlRadio[];
+  }
+
+  handleRadioClick(event: MouseEvent) {
+    const target = event.target as HTMLElement;
+    const checkedRadio = target.closest('sl-radio');
+
+    if (checkedRadio) {
+      const radios = this.getAllRadios();
+      radios.forEach(radio => {
+        radio.checked = radio === checkedRadio;
+        radio.input.tabIndex = radio === checkedRadio ? 0 : -1;
+      });
+    }
+  }
+
+  handleKeyDown(event: KeyboardEvent) {
+    if (['ArrowUp', 'ArrowDown', 'ArrowLeft', 'ArrowRight'].includes(event.key)) {
+      const radios = this.getAllRadios().filter(radio => !radio.disabled);
+      const checkedRadio = radios.find(radio => radio.checked) ?? radios[0];
+      const incr = ['ArrowUp', 'ArrowLeft'].includes(event.key) ? -1 : 1;
+      let index = radios.indexOf(checkedRadio) + incr;
+      if (index < 0) {
+        index = radios.length - 1;
+      }
+      if (index > radios.length - 1) {
+        index = 0;
+      }
+
+      this.getAllRadios().forEach(radio => {
+        radio.checked = false;
+        radio.input.tabIndex = -1;
+      });
+
+      radios[index].focus();
+      radios[index].checked = true;
+      radios[index].input.tabIndex = 0;
+
+      emit(radios[index], 'sl-change');
+
+      event.preventDefault();
+    }
+  }
+
+  handleSlotChange() {
+    const radios = this.getAllRadios();
+    const checkedRadio = radios.find(radio => radio.checked);
+
+    radios.forEach(radio => {
+      radio.setAttribute('role', 'radio');
+      radio.input.tabIndex = -1;
     });
+
+    if (checkedRadio) {
+      checkedRadio.input.tabIndex = 0;
+    } else if (radios.length > 0) {
+      radios[0].input.tabIndex = 0;
+    }
   }
 
   render() {
@@ -45,13 +103,15 @@ export default class SlRadioGroup extends LitElement {
           'radio-group': true,
           'radio-group--has-fieldset': this.fieldset
         })}
-        role="radiogroup"
-        @focusin=${this.handleFocusIn}
       >
         <legend part="label" class="radio-group__label">
           <slot name="label">${this.label}</slot>
         </legend>
-        <slot></slot>
+        <slot
+          @click=${this.handleRadioClick}
+          @keydown=${this.handleKeyDown}
+          @slotchange=${this.handleSlotChange}
+        ></slot>
       </fieldset>
     `;
   }

src/components/radio/radio.test.ts

@@ -1,5 +1,6 @@
-import { expect, fixture, html, oneEvent } from '@open-wc/testing';
+import { expect, fixture, html, oneEvent, waitUntil } from '@open-wc/testing';
 import { sendKeys } from '@web/test-runner-commands';
+import sinon from 'sinon';
 import type SlRadioGroup from '~/components/radio-group/radio-group';
 import type SlRadio from './radio';
 
@@ -52,12 +53,33 @@ describe('<sl-radio>', () => {
     expect(radio2.checked).to.be.true;
   });
 
-  it('should not fire sl-change when checked is set by javascript', async () => {
-    const el = await fixture<SlRadio>(html` <sl-radio></sl-radio> `);
-    el.addEventListener('sl-change', () => expect.fail('event fired'));
-    el.checked = true;
-    await el.updateComplete;
-    el.checked = false;
-    await el.updateComplete;
+  describe('when submitting a form', () => {
+    it('should submit the correct value', async () => {
+      const form = await fixture<HTMLFormElement>(html`
+        <form>
+          <sl-radio-group>
+            <sl-radio id="radio-1" name="a" value="1" checked></sl-radio>
+            <sl-radio id="radio-2" name="a" value="2"></sl-radio>
+            <sl-radio id="radio-2" name="a" value="3"></sl-radio>
+          </sl-radio-group>
+          <sl-button type="submit">Submit</sl-button>
+        </form>
+      `);
+      const button = form.querySelector('sl-button')!;
+      const radio = form.querySelectorAll('sl-radio')[1]!;
+      const submitHandler = sinon.spy((event: SubmitEvent) => {
+        formData = new FormData(form);
+        event.preventDefault();
+      });
+      let formData: FormData;
+
+      form.addEventListener('submit', submitHandler);
+      radio.click();
+      button.click();
+
+      await waitUntil(() => submitHandler.calledOnce);
+
+      expect(formData!.get('a')).to.equal('2');
+    });
   });
 });

src/components/radio/radio.ts

@@ -54,20 +54,9 @@ export default class SlRadio extends LitElement {
    */
   @property({ type: Boolean, reflect: true }) invalid = false;
 
-  firstUpdated() {
-    this.updateComplete.then(() => {
-      const radios = this.getAllRadios();
-      const checkedRadio = radios.find(radio => radio.checked);
-      radios.forEach(radio => {
-        radio.input.tabIndex = -1;
-      });
-
-      if (checkedRadio) {
-        checkedRadio.input.tabIndex = 0;
-      } else if (radios.length > 0) {
-        radios[0].input.tabIndex = 0;
-      }
-    });
+  connectedCallback(): void {
+    super.connectedCallback();
+    this.setAttribute('role', 'radio');
   }
 
   /** Simulates a click on the radio. */
@@ -107,37 +96,33 @@ export default class SlRadio extends LitElement {
     return [...radioGroup.querySelectorAll<SlRadio>('sl-radio')].filter((radio: this) => radio.name === this.name);
   }
 
-  getSiblingRadios() {
-    return this.getAllRadios().filter(radio => radio !== this);
-  }
-
   handleBlur() {
     this.hasFocus = false;
     emit(this, 'sl-blur');
   }
 
-  @watch('checked', { waitUntilFirstUpdate: true })
+  @watch('checked')
   handleCheckedChange() {
-    if (this.checked) {
-      this.input.tabIndex = 0;
+    this.setAttribute('aria-checked', this.checked ? 'true' : 'false');
 
-      this.getSiblingRadios().forEach(radio => {
-        radio.input.tabIndex = -1;
-        radio.checked = false;
-      });
+    if (this.hasUpdated) {
+      emit(this, 'sl-change');
     }
   }
 
   handleClick() {
     this.checked = true;
-    emit(this, 'sl-change');
   }
 
   @watch('disabled', { waitUntilFirstUpdate: true })
   handleDisabledChange() {
+    this.setAttribute('aria-disabled', this.disabled ? 'true' : 'false');
+
     // Disabled form controls are always valid, so we need to recheck validity when the state changes
-    this.input.disabled = this.disabled;
-    this.invalid = !this.input.checkValidity();
+    if (this.hasUpdated) {
+      this.input.disabled = this.disabled;
+      this.invalid = !this.input.checkValidity();
+    }
   }
 
   handleFocus() {
@@ -145,38 +130,7 @@ export default class SlRadio extends LitElement {
     emit(this, 'sl-focus');
   }
 
-  handleKeyDown(event: KeyboardEvent) {
-    if (['ArrowUp', 'ArrowDown', 'ArrowLeft', 'ArrowRight'].includes(event.key)) {
-      const radios = this.getAllRadios().filter(radio => !radio.disabled);
-      const incr = ['ArrowUp', 'ArrowLeft'].includes(event.key) ? -1 : 1;
-      let index = radios.indexOf(this) + incr;
-      if (index < 0) {
-        index = radios.length - 1;
-      }
-      if (index > radios.length - 1) {
-        index = 0;
-      }
-
-      this.getAllRadios().forEach(radio => {
-        radio.checked = false;
-        radio.input.tabIndex = -1;
-      });
-
-      radios[index].focus();
-      radios[index].checked = true;
-      radios[index].input.tabIndex = 0;
-
-      emit(radios[index], 'sl-change');
-
-      event.preventDefault();
-    }
-  }
-
   render() {
-    this.setAttribute('role', 'radio');
-    this.setAttribute('aria-checked', this.checked ? 'true' : 'false');
-    this.setAttribute('aria-disabled', this.disabled ? 'true' : 'false');
-
     return html`
       <label
         part="base"
@@ -186,7 +140,6 @@ export default class SlRadio extends LitElement {
           'radio--disabled': this.disabled,
           'radio--focused': this.hasFocus
         })}
-        @keydown=${this.handleKeyDown}
       >
         <input
           class="radio__input"
@@ -195,7 +148,6 @@ export default class SlRadio extends LitElement {
           value=${ifDefined(this.value)}
           .checked=${live(this.checked)}
           .disabled=${this.disabled}
-          aria-hidden="true"
           @click=${this.handleClick}
           @blur=${this.handleBlur}
           @focus=${this.handleFocus}