feat(checkbox radio-group): add description and tooltip support - #3605
feat(checkbox radio-group): add description and tooltip support#3605michal-sanoma wants to merge 27 commits into
Conversation
🦋 Changeset detectedLatest commit: 016e659 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🕸 Preview deploys |
There was a problem hiding this comment.
Pull request overview
Adds description and tooltip support to checkbox and radio components.
Changes:
- Adds description properties, slots, styling, and ARIA relationships.
- Adds scoped tooltip rendering.
- Adds tests, stories, dependencies, and release metadata.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.changeset/social-lies-tell.md |
Documents minor feature releases. |
packages/components/checkbox/package.json |
Adds tooltip and scoped-elements dependencies. |
packages/components/checkbox/src/checkbox-group.stories.ts |
Demonstrates group descriptions and tooltips. |
packages/components/checkbox/src/checkbox.scss |
Styles description content. |
packages/components/checkbox/src/checkbox.spec.ts |
Tests descriptions and tooltips. |
packages/components/checkbox/src/checkbox.stories.ts |
Adds feature examples. |
packages/components/checkbox/src/checkbox.ts |
Implements checkbox descriptions and tooltips. |
packages/components/radio-group/package.json |
Adds tooltip and scoped-elements dependencies. |
packages/components/radio-group/src/radio-group.stories.ts |
Demonstrates radio descriptions and tooltips. |
packages/components/radio-group/src/radio.scss |
Styles description content. |
packages/components/radio-group/src/radio.spec.ts |
Tests descriptions and tooltips. |
packages/components/radio-group/src/radio.ts |
Implements radio descriptions and tooltips. |
yarn.lock |
Locks added dependencies. |
Suppressed comments (3)
packages/components/radio-group/src/radio.ts:342
- This IDREF is set on an element in the shadow root while
#descriptionis in the host's light DOM, so attribute-based ID resolution cannot establish the intended accessible description across the tree boundary. It also only appends IDs, leaving stale references when descriptions are removed or replaced. Usewrapper.ariaDescribedByElementswith the description element and track/remove the component-owned reference while preserving other descriptions.
const describedBy = this.wrapper.getAttribute('aria-describedby');
const ids = new Set(describedBy ? describedBy.split(' ') : []);
ids.add(this.#description.id);
this.wrapper.setAttribute('aria-describedby', Array.from(ids).join(' '));
packages/components/checkbox/src/checkbox.ts:456
- If both the property and a slotted description are present, then the slotted element is removed, this branch is skipped because
this.descriptionis still truthy.#descriptionconsequently keeps pointing at the detached slotted element: the property fallback becomes visible, but the input is not validly described by it. Recreate the synthesized property-backed description when no assigned description remains.
} else if (!this.description && this.#description) {
packages/components/checkbox/src/checkbox.ts:502
- When a description is removed or replaced, this logic only adds the current ID and never removes the previous component-owned ID. Toggling
descriptionrepeatedly therefore accumulates references to detached elements in the input'saria-describedby. Track the previously owned ID/reference and remove it during synchronization, without removing consumer-provided descriptions.
const describedBy = this.input.getAttribute('aria-describedby');
const ids = new Set(describedBy ? describedBy.split(' ') : []);
ids.add(this.#description.id);
this.input.setAttribute('aria-describedby', Array.from(ids).join(' '));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/components/checkbox/src/checkbox.ts:545
- This attribute write replaces the
ariaDescribedByElementslist assembled above. As a result, consumer-provided descriptions preserved byForwardAriaMixinand the shadow-root tooltip reference are discarded whenever an internal description is present. Keep these relationships inariaDescribedByElementsand remove only references owned by this component.
if (this.#description?.id && this.input) {
const describedBy = this.input.getAttribute('aria-describedby');
const ids = new Set(describedBy ? describedBy.split(' ') : []);
ids.add(this.#description.id);
this.input.setAttribute('aria-describedby', Array.from(ids).join(' '));
packages/components/checkbox/src/checkbox.ts:556
- A string
aria-describedbyID on the light-DOM input cannot resolve this tooltip inside the checkbox shadow root, so the tooltip is not exposed as the input's accessible description. Add it throughinput.ariaDescribedByElementsand remove that owned reference whentooltipis cleared; checking only for a current tooltip also leaves stale references behind.
tooltip.id ||= `sl-checkbox-tooltip-${nextUniqueId++}`;
const describedBy = this.input.getAttribute('aria-describedby');
const ids = new Set(describedBy ? describedBy.split(' ') : []);
ids.add(tooltip.id);
this.input.setAttribute('aria-describedby', Array.from(ids).join(' '));
packages/components/radio-group/src/radio.ts:362
- The radio wrapper is in the shadow root, while
#descriptionis a light-DOM node, so this string IDREF cannot resolve across the shadow boundary. CallingsetAttributealso replaces anyariaDescribedByElementsrelation installed by the tooltip. Link the description throughwrapper.ariaDescribedByElementsinstead, preserving the tooltip reference and removing the previously managed description when it changes.
const describedBy = this.wrapper.getAttribute('aria-describedby');
const ids = new Set(describedBy ? describedBy.split(' ') : []);
ids.add(this.#description.id);
this.wrapper.setAttribute('aria-describedby', Array.from(ids).join(' '));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/components/checkbox/src/checkbox.ts:531
#syncAria()replaces the input's entire described-by list with only the component description. This drops consumer-providedaria-describedbyreferences forwarded byForwardAriaMixin, and it never associates the new tooltip with the actual checkbox input—the tooltip only describes the non-semantic wrapper. Manage the component-owned description/tooltip references while preserving existing input references, and assert the tooltip relation onel.input.
this.input.ariaDescribedByElements = elements.length > 0 ? elements : null;
packages/components/checkbox/src/checkbox.ts:476
- This permanently sets
aria-hiddenon a caller-owned slotted element. If the consumer later removes or changes itsslotattribute, the element remains hidden from assistive technology in its new context. Preserve the prior value and restore it when this element stops being the active description.
slottedDescription.id ||= `sl-checkbox-description-${nextUniqueId++}`;
slottedDescription.setAttribute('aria-hidden', 'true');
packages/components/radio-group/src/radio.ts:316
- This permanently sets
aria-hiddenon a caller-owned slotted element. If the consumer later removes or changes itsslotattribute, the element remains hidden from assistive technology in its new context. Preserve the prior value and restore it when this element stops being the active description.
slottedDescription.id ||= `sl-radio-description-${nextUniqueId++}`;
slottedDescription.setAttribute('aria-hidden', 'true');
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (7)
packages/components/checkbox/src/checkbox.scss:203
- These custom properties are not generated by the repository's token set, so
font-sizeis discarded and the description inherits the label size whileline-heightalways falls back to 16px. Use the available composite body-small typography token instead.
font-size: var(--sl-text-size-body-sm);
line-height: var(--sl-text-typeset-lineHeight-200, 16px);
packages/components/radio-group/src/radio.scss:152
- These custom properties are not generated by the repository's token set, so
font-sizeis discarded and the description inherits the label size whileline-heightalways falls back to 16px. Use the available composite body-small typography token instead.
font-size: var(--sl-text-size-body-sm);
line-height: var(--sl-text-typeset-lineHeight-200, 16px);
packages/components/checkbox/src/checkbox.scss:138
--sl-text-size-body-mdis not a generated token, so large checkboxes always use the hard-coded 14px fallback rather than the theme's body-medium size. Use the generated font-size token.
font-size: var(--sl-text-size-body-md, 14px);
packages/components/radio-group/src/radio.scss:110
--sl-text-size-body-mdis not a generated token, so large radios always use the hard-coded 14px fallback rather than the theme's body-medium size. Use the generated font-size token.
font-size: var(--sl-text-size-body-md, 14px);
packages/components/checkbox/src/checkbox.ts:285
slotchangeonly fires when assigned nodes change, not when the text or descendants of an already assigned element mutate. A slotted description initialized empty and populated reactively therefore remains hidden by thehas-descriptionCSS (and clearing existing text leaves stale state). Observe assigned-content mutations and resync this attribute, including observer cleanup on disconnect.
<slot name="description" @slotchange=${() => this.#onDescriptionSlotChange()}
packages/components/radio-group/src/radio.ts:218
slotchangeonly fires when assigned nodes change, not when the text or descendants of an already assigned element mutate. A slotted description initialized empty and populated reactively therefore remains hidden by thehas-descriptionCSS (and clearing existing text leaves stale state). Observe assigned-content mutations and resync this attribute, including observer cleanup on disconnect.
<slot name="description" @slotchange=${() => this.#onDescriptionSlotChange()}
packages/components/checkbox/src/checkbox.spec.ts:220
- This assertion only checks the wrapper relation that
<sl-tooltip for="wrapper">creates itself, so it does not exercise the new manual relationship to the actual checkbox input and would pass if that accessibility fix regressed. Assert the input relationship here instead.
expect(wrapper?.ariaDescribedByElements).to.include(tooltip as HTMLElement);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
packages/components/checkbox/src/checkbox.ts:602
- This relationship is not reapplied when
#onInputSlotChangereplacesthis.input. The component explicitly supports a custom input being slotted after connection, but if a description or tooltip already exists, the new control receives neither reference and is announced without that description. Re-run this synchronization after switching the input target.
this.input.ariaDescribedByElements = nextRefs.length > 0 ? nextRefs : null;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (5)
packages/components/checkbox/src/checkbox.ts:268
- A whitespace-only property is truthy here, so
has-descriptionremains set even though#descriptionText()trims it to empty. This displays an empty description row and differs from the empty slotted-description behavior. Base the state solely on normalized text.
this.toggleAttribute(
'has-description',
!!this.description || this.#descriptionText().length > 0
);
packages/components/radio-group/src/radio.ts:212
- A whitespace-only property is truthy here, so
has-descriptionremains set even though#descriptionText()trims it to empty. This displays an empty description row and differs from the empty slotted-description behavior. Base the state solely on normalized text.
this.toggleAttribute(
'has-description',
!!this.description || this.#descriptionText().length > 0
);
packages/components/checkbox/src/checkbox.ts:232
- Disconnecting stops the observer but leaves a consumer-provided description with the component-owned
aria-hidden="true". If that node is later moved out while the checkbox remains disconnected, it stays hidden from assistive technology. Restore its recorded value before disconnecting; reconnection will apply the component state again.
This issue also appears on line 265 of the same file.
override disconnectedCallback(): void {
this.#mutationObserver.disconnect();
super.disconnectedCallback();
packages/components/radio-group/src/radio.ts:174
- Disconnecting stops the observer but leaves a consumer-provided description with the component-owned
aria-hidden="true". If that node is later moved out while the radio remains disconnected, it stays hidden from assistive technology. Restore its recorded value before disconnecting; reconnection will apply the component state again.
This issue also appears on line 209 of the same file.
override disconnectedCallback(): void {
this.#mutationObserver.disconnect();
super.disconnectedCallback();
packages/components/checkbox/src/checkbox.spec.ts:245
- This only verifies the tooltip's built-in
for="wrapper"relation, so it would still pass if the checkbox input—the actual control—lost its accessible description. Assertel.input.ariaDescribedByElementsto cover the new#syncAria()behavior.
const wrapper = el.renderRoot.querySelector<HTMLElement>('#wrapper');
expect(wrapper?.ariaDescribedByElements).to.include(tooltip as HTMLElement);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/components/checkbox/src/checkbox.ts:764
- This unconditionally copies every ARIA property from the old input and then clears it. When a late custom input already has its own
aria-label,aria-controls, or element-reference properties, null/empty values from the synthesized input overwrite them; when a custom input is removed for reuse, its own ARIA state is likewise stripped. Preserve consumer-owned ARIA on both inputs and transfer only state known to have been forwarded or generated by this component.
forwardedAriaValueProperties.forEach(prop => {
to[prop] = from[prop];
from[prop] = null;
});
forwardedAriaElementProperties.forEach(prop => {
packages/components/checkbox/src/checkbox.ts:536
- Switching the active input here leaves the FormControlMixin
invalidlistener attached to every previous input:setFormControlElement()only adds the listener, while disconnection removes it only from the current element (form-control-mixin.ts:303,519-522). If a removed custom input is reused, its invalid events are still prevented and update this checkbox's validity state. Detach the old form-control element before registering the replacement, preferably insetFormControlElement()itself.
This issue also appears on line 760 of the same file.
this.setFormControlElement(this.input);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/components/checkbox/src/checkbox.ts:561
- Restoring the fallback with
captureFromOldInputset tofalseloses host-forwarded ARIA that was applied while the custom input was active (and all forwarded ARIA when the component started with a custom input). Those host attributes have already been removed byForwardAriaMixin, while#forwardedAria*Stateonly captures the synthesized input during the earlier switch, sosetProxyTarget()cannot replay the current label/description/controls onto the fallback. Track the ARIA owned by the forwarding path as it changes and migrate that subset when restoring the fallback; simply capturing every property here would incorrectly steal consumer-owned ARIA from the custom input.
this.#syncForwardedAria(this.input, input, false);
packages/components/form/src/form-control-mixin.ts:524
disconnectedCallback()removes theinvalidlistener but retains#formControlElement. When a control is reconnected, it calls this method with the same element, so this early return prevents the listener from being registered again and subsequent nativeinvalidevents no longer update the component's validity UI. Remove the shortcut (removing and re-adding the same listener is safe), or clear the stored element during disconnect.
if (this.#formControlElement === element) {
return;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/components/shared/src/mixins/forward-aria-mixin.ts:145
- Retargeting preserves an attribute already owned by the new target, but the storage still treats that attribute as forwarded. For example, if the old target received
aria-label="host"and the new target already hasaria-label="custom", this branch preservescustom; a laterhost.removeAttribute('aria-label')nevertheless removescustomvia the cleanup path. Element-reference properties have the same ownership loss when the new target already contains one ofrefs. Track only the values/references actually contributed to each target so later cleanup cannot remove consumer-owned ARIA.
forwardedAttributesStorage.get(this)?.forEach((value, name) => {
if (!to.hasAttribute(name)) {
to.setAttribute(name, value);
}
packages/components/checkbox/src/checkbox.ts:825
- This independently cached ARIA state becomes stale while a custom input is active. For example: forward
aria-label="A", switch to a custom input (capturingA), remove the host ARIA label, then remove the custom input;#applyForwardedAriareplaysAonto the fallback even though the host cleared it. Changed element references similarly accumulate obsolete values. UseForwardAriaMixin's proxy retargeting as the source of truth, or update this cache whenever forwarded state changes or is removed.
#applyForwardedAria(input: HTMLInputElement): void {
forwardedAriaValueProperties.forEach(prop => {
const value = this.#forwardedAriaValueState.get(prop);
if (value !== undefined && input[prop] === null) {
input[prop] = value;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/components/shared/src/mixins/forward-aria-mixin.ts:430
- When the forwarded IDs resolve to no elements, assigning
[]replaces the target's entire reference list and deletes references owned by the target or another feature. This is a regression from the merge behavior: after removing this mixin's prior contribution, preserve any nonempty references that remain and only normalize to[]when the target has none.
if (!elements.length) {
(targetElement as unknown as Record<string, Element[] | null>)[elementsProp] = [];
forwarded.set(elementsProp, elements);
elementsForTarget(this, targetElement).delete(elementsProp);
continue;
packages/components/shared/src/mixins/forward-aria-mixin.ts:246
- This migration only replays
forwardedElementsStorage, but attribute forwarding populates that storage only for plural properties. Consequently anaria-activedescendantforwarded throughariaActiveDescendantElementis neither copied to the new proxy nor removed from the old one. Track and migrate singular attribute-owned references as well.
forwardedElementsStorage.get(this)?.forEach((refs, prop) => {
const contributed = applyElementReference(to, prop, refs);
packages/components/shared/src/mixins/forward-aria-mixin.ts:281
- Equality with the stored value does not establish ownership here. If the previous proxy already had consumer-owned
aria-disabled="true"before the host'sariaDisabledproperty was forwarded, retargeting clears that consumer value. Record per-target ownership forariaDisabled, as is done for other forwarded attributes, and only clean up values actually contributed by this mixin.
if (ariaDisabledStorage.has(this)) {
const value = ariaDisabledStorage.get(this) ?? null;
if (to.ariaDisabled === null) {
setAriaDisabled(to, value);
}
if (from.ariaDisabled === value) {
setAriaDisabled(from, null);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/components/checkbox/src/checkbox.ts:491
- Restoring the fallback input drops scalar ARIA attributes that were already forwarded to the removed custom input.
ForwardAriaMixinremoves attributes such asaria-labelfrom the host after forwarding them, andsetProxyTarget()cannot replay those plain attributes to this new input. For example, removing the input from<sl-checkbox aria-label="Terms"><input slot="input"></sl-checkbox>leaves the synthesized checkbox unnamed. Preserve/replay the forwarded ARIA state when changing proxy targets.
} else if (!this.#isSynthesizedInput) {
const customValidity = this.input.validity.customError ? this.input.validationMessage : '';
const input = this.#synthesizedInput ?? this.#createInput();
this.input = input;
this.#syncInput(this.input, customValidity);
packages/components/checkbox/src/checkbox.ts:496
- The removed custom input still retains the
invalidlistener installed byFormControlMixin.setFormControlElement()only adds the listener to the replacement and never detaches it from the previous control, so reusing that custom input elsewhere will still suppress its native validation UI and update this checkbox's validity state. Detach the old form-control listener when replacing the target.
this.setFormControlElement(this.input);
Should be better now
Fixed |
I don't have idea why, because when I add reference to different text as I didn't find any active issue for problems of reading |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/components/radio-group/src/radio.ts:99
- If the synthesized tooltip-description node is removed while
tooltipremains unchanged, the mutation observer only resyncs labels/descriptions, so the visual tooltip survives while the radio's accessible description points at a detached element. Recreate the tooltip description during external child-mutation handling as well.
this.#onLabelSlotChange();
this.#onDescriptionSlotChange();
packages/components/checkbox/src/checkbox.ts:133
- If the synthesized tooltip-description node is removed while
tooltipremains unchanged (for example, by light-DOM reconciliation), this observer never recreates it. The visual tooltip continues to render, but the input keeps a detached description reference and loses its accessible tooltip text. Resync the tooltip node when external child mutations are handled, just as is done for the property-backed description.
this.#onLabelSlotChange();
this.#onDescriptionSlotChange();
Summary
This PR extends both
sl-checkboxandsl-radiocomponents withdescriptionandtooltipfunctionality matching the Figma design specifications.Changes
descriptionSupport (sl-checkbox&sl-radio):@property() description?: stringand<slot name="description">to allow providing helper/description text either via property or slot.part="content"container groupingpart="label"andpart="description".has-descriptionhost attribute reflecting whether a description is present.--sl-color-foreground-subtlestand--sl-text-size-body-sm(with size-adjusted variants).aria-describedby.tooltipSupport (sl-checkbox&sl-radio):sl-tooltipviaScopedElementsMixin.@property() tooltip?: stringrendering<sl-tooltip for="wrapper" part="tooltip" type="description">AFTER