diff --git a/.changeset/empty-cougars-cross.md b/.changeset/empty-cougars-cross.md new file mode 100644 index 0000000000..c123591880 --- /dev/null +++ b/.changeset/empty-cougars-cross.md @@ -0,0 +1,6 @@ +--- +'@sl-design-system/checkbox': patch +--- + +Fix checkbox group not working with tooltips (only `sl-checkbox` elements are tracked by `@queryAssignedElements`). +Use `ForwardAriaMixin` to forward `aria-describedby` as element references to the inner input element, enabling screen reader support. diff --git a/packages/components/checkbox/src/checkbox-group.spec.ts b/packages/components/checkbox/src/checkbox-group.spec.ts index cad3f96582..399ea6aa05 100644 --- a/packages/components/checkbox/src/checkbox-group.spec.ts +++ b/packages/components/checkbox/src/checkbox-group.spec.ts @@ -1,5 +1,6 @@ import { type SlFormControlEvent } from '@sl-design-system/form'; import '@sl-design-system/form/register.js'; +import '@sl-design-system/tooltip/register.js'; import { fixture } from '@sl-design-system/vitest-browser-lit'; import { LitElement, type TemplateResult, html } from 'lit'; import { spy } from 'sinon'; @@ -437,4 +438,29 @@ describe('sl-checkbox-group', () => { expect(fitc.shadowRoot!.activeElement).to.equal(input); }); }); + + describe('with tooltips', () => { + beforeEach(async () => { + el = await fixture(html` + + A + B + Tooltip + C + + `); + }); + + it('should only contain checkbox elements in the boxes property', () => { + expect(el.boxes).to.have.lengthOf(3); + expect(el.boxes?.every(box => box.tagName.toLowerCase() === 'sl-checkbox')).to.be.true; + }); + + it('should correctly reflect only checkbox values', async () => { + await userEvent.click(el.querySelector('sl-checkbox[value="b"]')!); + await new Promise(resolve => setTimeout(resolve, 50)); + + expect(el.value).to.deep.equal([null, 'b', null]); + }); + }); }); diff --git a/packages/components/checkbox/src/checkbox-group.stories.ts b/packages/components/checkbox/src/checkbox-group.stories.ts index 8fc5f1e8ed..de866ac7ad 100644 --- a/packages/components/checkbox/src/checkbox-group.stories.ts +++ b/packages/components/checkbox/src/checkbox-group.stories.ts @@ -1,6 +1,8 @@ import '@sl-design-system/button/register.js'; import '@sl-design-system/button-bar/register.js'; import '@sl-design-system/form/register.js'; +import { tooltip } from '@sl-design-system/tooltip'; +import '@sl-design-system/tooltip/register.js'; import { type Meta, type StoryObj } from '@storybook/web-components-vite'; import { type TemplateResult, html } from 'lit'; import '../register.js'; @@ -191,3 +193,37 @@ export const CustomAsyncValidity: Story = { } } }; + +export const WithTooltips: Story = { + render: () => { + const onClick = (event: Event & { target: HTMLElement }): void => { + event.target.closest('sl-form')?.reportValidity(); + }; + + return html` +

+ This story demonstrates how to use tooltips with checkboxes inside a checkbox group using + both the tooltip() directive and the manual + aria-describedby approach with a separate sl-tooltip element. +

+ + + + + Newsletter + Promotions + Promotions tooltip + Product updates + + + + Report validity + + + `; + } +}; diff --git a/packages/components/checkbox/src/checkbox-group.ts b/packages/components/checkbox/src/checkbox-group.ts index 3d5b6db374..57b5b3b9be 100644 --- a/packages/components/checkbox/src/checkbox-group.ts +++ b/packages/components/checkbox/src/checkbox-group.ts @@ -75,7 +75,7 @@ export class CheckboxGroup extends FormControlMixin(LitElement) { readonly internals = this.attachInternals(); /** @internal The slotted checkboxes. */ - @queryAssignedElements() boxes?: Array>; + @queryAssignedElements({ selector: 'sl-checkbox' }) boxes?: Array>; /** @internal Emits when the component loses focus. */ @event({ name: 'sl-blur' }) blurEvent!: EventEmitter; diff --git a/packages/components/checkbox/src/checkbox.spec.ts b/packages/components/checkbox/src/checkbox.spec.ts index ffd7c7e591..f9af794cff 100644 --- a/packages/components/checkbox/src/checkbox.spec.ts +++ b/packages/components/checkbox/src/checkbox.spec.ts @@ -13,7 +13,9 @@ describe('sl-checkbox', () => { describe('defaults', () => { beforeEach(async () => { - el = await fixture(html`Hello world`); + el = await fixture(html` + Hello world + `); input = el.querySelector('input')!; }); @@ -23,6 +25,10 @@ describe('sl-checkbox', () => { expect(input.type).to.equal('checkbox'); }); + it('should have aria-describedby in observedAttributes', () => { + expect((el.constructor as typeof Checkbox).observedAttributes).to.include('aria-describedby'); + }); + it('should not be checked', () => { expect(el.checked).not.to.be.true; expect(input.checked).not.to.be.true; @@ -121,11 +127,43 @@ describe('sl-checkbox', () => { }); it('should proxy the aria-labelledby attribute to the input element', async () => { - el.setAttribute('aria-labelledby', 'id'); + const label = document.createElement('span'); + label.id = 'my-label'; + label.textContent = 'My label'; + el.parentElement!.appendChild(label); + + el.setAttribute('aria-labelledby', 'my-label'); await new Promise(resolve => setTimeout(resolve, 50)); expect(el).to.not.have.attribute('aria-labelledby'); - expect(el.input).to.have.attribute('aria-labelledby', 'id'); + + // In same-scope mode, ForwardAriaMixin uses string attributes and + // #onLabelSlotChange appends the checkbox's own label IDs. + const ids = el.input.getAttribute('aria-labelledby')!.split(/\s+/); + expect(ids).to.include('my-label'); + + label.remove(); + }); + + it('should proxy the aria-describedby attribute to the input element', async () => { + const tooltip = document.createElement('span'); + tooltip.id = 'my-tooltip'; + tooltip.textContent = 'My tooltip'; + el.parentElement!.appendChild(tooltip); + + el.setAttribute('aria-describedby', 'my-tooltip'); + await new Promise(resolve => setTimeout(resolve, 50)); + + expect(el).to.not.have.attribute('aria-describedby'); + + // In same-scope mode, ForwardAriaMixin uses string attributes. + expect(el.input.getAttribute('aria-describedby')).to.equal('my-tooltip'); + + tooltip.remove(); + }); + + it('should return the input element from getProxyTarget', () => { + expect(el.getProxyTarget()).to.equal(el.input); }); it('should be pristine', () => { @@ -284,7 +322,9 @@ describe('sl-checkbox', () => { describe('disabled', () => { beforeEach(async () => { - el = await fixture(html`Hello world`); + el = await fixture(html` + Hello world + `); input = el.querySelector('input')!; }); @@ -323,7 +363,9 @@ describe('sl-checkbox', () => { describe('validation', () => { beforeEach(async () => { - el = await fixture(html`Hello world`); + el = await fixture(html` + Hello world + `); }); it('should be invalid when required and no option is selected', async () => { @@ -434,7 +476,9 @@ describe('sl-checkbox', () => { // empty } - el = await fixture(html``); + el = await fixture(html` + + `); }); it('should emit an sl-form-control event after first render', () => { diff --git a/packages/components/checkbox/src/checkbox.stories.ts b/packages/components/checkbox/src/checkbox.stories.ts index fd7fe9a664..2f99f58c75 100644 --- a/packages/components/checkbox/src/checkbox.stories.ts +++ b/packages/components/checkbox/src/checkbox.stories.ts @@ -1,6 +1,8 @@ import '@sl-design-system/button/register.js'; import '@sl-design-system/button-bar/register.js'; import '@sl-design-system/form/register.js'; +import { tooltip } from '@sl-design-system/tooltip'; +import '@sl-design-system/tooltip/register.js'; import { type Meta, type StoryObj } from '@storybook/web-components-vite'; import { type TemplateResult, html, nothing } from 'lit'; import { ifDefined } from 'lit/directives/if-defined.js'; @@ -85,8 +87,9 @@ export default { .showValid=${showValid} .value=${value} size=${ifDefined(size)} - >${text} + ${text} + `} ${reportValidity @@ -193,21 +196,21 @@ export const Indeterminate: StoryObj = {
  • - Andre + + Andre +
  • - Paul Bunyan + + Paul Bunyan +
  • - Two sandwiches + + Two sandwiches +
    • @@ -218,14 +221,14 @@ export const Indeterminate: StoryObj = { Smurfs
  • - Mushrooms + + Mushrooms +
  • - One Sandwich + + One Sandwich +
    • @@ -239,8 +242,10 @@ export const NoVisibleLabel: StoryObj = { return html`

    This checkbox has no internal or external label. It only has an - aria-label attribute. That attribute is automatically applied to the - input element. + aria-label + attribute. That attribute is automatically applied to the + input + element.

    `; @@ -281,9 +286,9 @@ export const CustomValidity: Story = { }; return html` - I agree to all terms & conditions + + I agree to all terms & conditions + `; } } @@ -307,10 +312,73 @@ export const CustomAsyncValidity: Story = { }; return html` - I agree to all terms & conditions + + I agree to all terms & conditions + `; } } }; + +export const WithTooltip: Story = { + render: () => { + const onClick = (event: Event & { target: HTMLElement }): void => { + event.target.closest('sl-form')?.reportValidity(); + }; + + return html` +

    + This story demonstrates how to use tooltips with checkboxes using the + tooltip() + directive, + aria-describedby + , and + aria-labelledby + with a separate + sl-tooltip + element. The + ForwardAriaMixin + resolves IDs to element references and forwards them to the focusable input element, which + properly crosses the shadow DOM boundary. +

    + +

    Using the tooltip directive

    + + + + Newsletter + + + + Report validity + + + +

    Using aria-describedby

    + + + + Promotions + + Promotions tooltip + + + Report validity + + + +

    Using aria-labelledby

    + + + + Updates + + Updates tooltip + + + Report validity + + + `; + } +}; diff --git a/packages/components/checkbox/src/checkbox.ts b/packages/components/checkbox/src/checkbox.ts index bc2d9528cf..609c5ab30d 100644 --- a/packages/components/checkbox/src/checkbox.ts +++ b/packages/components/checkbox/src/checkbox.ts @@ -3,7 +3,7 @@ import { FormControlMixin } from '@sl-design-system/form'; import { type EventEmitter, EventsController, - ObserveAttributesMixin, + ForwardAriaMixin, event } from '@sl-design-system/shared'; import { @@ -45,7 +45,8 @@ let nextUniqueId = 0; */ @localized() // eslint-disable-next-line @typescript-eslint/no-explicit-any -export class Checkbox extends ObserveAttributesMixin(FormControlMixin(LitElement), [ +export class Checkbox extends ForwardAriaMixin(FormControlMixin(LitElement), [ + 'aria-describedby', 'aria-disabled', 'aria-label', 'aria-labelledby' @@ -166,6 +167,7 @@ export class Checkbox extends ObserveAttributesMixin(FormControlMixin(L } this.setFormControlElement(this.input); + this.setProxyTarget(this.input); this.#onLabelSlotChange(); } @@ -283,6 +285,7 @@ export class Checkbox extends ObserveAttributesMixin(FormControlMixin(L this.#syncInput(this.input); this.setFormControlElement(this.input); + this.setProxyTarget(this.input); } } @@ -315,13 +318,32 @@ export class Checkbox extends ObserveAttributesMixin(FormControlMixin(L } requestAnimationFrame(() => { - if (!this.input.hasAttribute('aria-labelledby') && this.input.labels?.length) { - this.input.setAttribute( - 'aria-labelledby', - Array.from(this.input.labels) - .map(label => label.id) - .join(' ') - ); + if ( + (this.hasAttribute('aria-labelledby') || this.ariaLabelledByElements?.length) && + !this.input.labels?.length + ) { + // ForwardAriaMixin will handle it; no labels to append. + return; + } + + if (this.input.labels?.length) { + const labelIds = Array.from(this.input.labels) + .map(label => label.id) + .filter(Boolean) + .join(' '); + + const existing = this.input.getAttribute('aria-labelledby') ?? ''; + const existingIds = existing.split(/\s+/).filter(Boolean); + + // Append label IDs that aren't already present + const newIds = labelIds.split(/\s+/).filter(id => id && !existingIds.includes(id)); + + if (newIds.length) { + const combined = [...existingIds, ...newIds].join(' '); + this.input.setAttribute('aria-labelledby', combined); + } else if (!existing && labelIds) { + this.input.setAttribute('aria-labelledby', labelIds); + } } }); @@ -336,7 +358,5 @@ export class Checkbox extends ObserveAttributesMixin(FormControlMixin(L input.checked = !!this.checked; input.indeterminate = !!this.indeterminate; - - this.setAttributesTarget(input); } } diff --git a/packages/components/shared/src/mixins/forward-aria-mixin.ts b/packages/components/shared/src/mixins/forward-aria-mixin.ts index 0fa2ba6dac..1d3271cea3 100644 --- a/packages/components/shared/src/mixins/forward-aria-mixin.ts +++ b/packages/components/shared/src/mixins/forward-aria-mixin.ts @@ -1,6 +1,17 @@ import { type Constructor } from '@open-wc/dedupe-mixin'; import { type ReactiveElement } from 'lit'; +/** Counter for generating stable unique IDs for elements that lack one. */ +let nextAutoId = 0; + +/** Ensures the element has an `id` attribute, auto-assigning one if missing. */ +function ensureId(el: HTMLElement): string { + if (!el.id) { + el.id = `sl-aria-ref-${nextAutoId++}`; + } + return el.id; +} + export interface ForwardAriaMixinInterface { getProxyTarget(): HTMLElement | undefined; setProxyTarget(target: HTMLElement): void; @@ -77,9 +88,21 @@ export function ForwardAriaMixin< ariaDisabledStorage = new WeakMap(); class ForwardAriaImpl extends constructor { + /** Maximum number of deferred retries before giving up on unresolved IDs. */ + static readonly #MAX_RETRIES = 3; + #observer?: MutationObserver; #pendingAttributes = new Set(); + /** Tracks the number of deferred resolution attempts per attribute. */ + #retryCounts = new Map(); + + /** + * Stores attribute values at the time they were observed, as a fallback when Lit's ARIA + * reflection removes the attribute before resolution. + */ + #storedValues = new Map(); + static override get observedAttributes(): string[] { return [...(super.observedAttributes ?? []), ...(observedAttributes ?? [])]; } @@ -93,11 +116,36 @@ export function ForwardAriaMixin< setProxyTarget(target: HTMLElement): void { targetElements.set(this, target); - // Forward any element reference properties that were set before the target was available + // Forward any element reference properties that were set before the target was available. + // Apply the same scope-aware logic as #forwardAttributes: when the target shares the + // same root as the host, use string attributes (IDs) instead of element references, + // because Chrome clears the content attribute when ariaLabelledByElements is assigned. const stored = propertyStorage.get(this); if (stored) { + const root = this.getRootNode(); + const targetRoot = target.getRootNode(); + for (const [prop, value] of stored) { - (target as unknown as Record)[prop] = value; + // Skip null/empty values to avoid adding empty aria-* content attributes + const isEmpty = value === null || (Array.isArray(value) && value.length === 0); + if (isEmpty) { + continue; + } + + if (targetRoot === root) { + // Same scope: convert element references to a string ID attribute. + const attrName = Object.entries(ELEMENT_REFERENCES).find(([, p]) => p === prop)?.[0]; + if (attrName) { + const elements = Array.isArray(value) ? value : [value]; + const ids = elements.map(el => ensureId(el as HTMLElement)).join(' '); + if (ids) { + target.setAttribute(attrName, ids); + } + } + } else { + // Cross-scope: element references are required. + (target as unknown as Record)[prop] = value; + } } } @@ -123,16 +171,21 @@ export function ForwardAriaMixin< // before the element upgraded or before the target is available). if (!observedAttributes) { // Collect any aria-* attributes already present on the element - for (const { name } of this.attributes) { + for (const { name, value } of this.attributes) { if (name.startsWith('aria-')) { this.#pendingAttributes.add(name); + this.#storedValues.set(name, value); } } this.#observer = new MutationObserver(mutations => { for (const { attributeName } of mutations) { if (attributeName?.startsWith('aria-')) { - this.#pendingAttributes.add(attributeName); + const value = this.getAttribute(attributeName); + if (value !== null) { + this.#pendingAttributes.add(attributeName); + this.#storedValues.set(attributeName, value); + } } } @@ -154,12 +207,23 @@ export function ForwardAriaMixin< oldValue: string | null, newValue: string | null ): void { + // Store the value BEFORE calling super, since Lit's ARIA reflection may + // remove or empty the attribute during super.attributeChangedCallback. + if (observedAttributes?.includes(name) && newValue) { + this.#pendingAttributes.add(name); + this.#storedValues.set(name, newValue); + } + super.attributeChangedCallback(name, oldValue, newValue); // Only intercept attributes from our explicit list (the MutationObserver // path handles the "observe everything" case in connectedCallback). - if (observedAttributes?.includes(name) && newValue !== null) { - this.#pendingAttributes.add(name); + if (observedAttributes?.includes(name) && newValue) { + // Attempt to forward; if #pendingAttributes was already consumed (e.g. by a + // re-entrant call triggered from super), re-add and retry. + if (!this.#pendingAttributes.has(name)) { + this.#pendingAttributes.add(name); + } this.#forwardAttributes(); } } @@ -174,9 +238,13 @@ export function ForwardAriaMixin< // Resolve IDs from the host's root (light DOM document or parent shadow root), // not from our own shadow root. const root = this.getRootNode() as Document | ShadowRoot; + const unresolved = new Set(); for (const name of this.#pendingAttributes) { - const value = this.getAttribute(name); + // Read from the host attribute first; fall back to stored value if Lit's + // built-in ARIA reflection removed the attribute (or set it to empty string) + // before we could process it. + const value = this.getAttribute(name) || this.#storedValues.get(name); if (!value) { continue; } @@ -185,25 +253,96 @@ export function ForwardAriaMixin< if (elementsProp) { // Reference attribute: resolve space-separated IDs to DOM elements and // assign via the element reference property (singular or plural). - const elements = value - .split(/\s+/) - .map(id => root.querySelector(`#${CSS.escape(id)}`)) - .filter((el): el is HTMLElement => el !== null); + const ids = value.split(/\s+/), + elements = ids + .map(id => root.querySelector(`#${CSS.escape(id)}`)) + .filter((el): el is HTMLElement => el !== null); + + // If not all IDs could be resolved, defer until the referenced elements + // are in the DOM (e.g. when a sibling is connected after this element). + // Only defer when at least one element was found (partial resolution), + // indicating siblings are still rendering. When nothing resolves at all, + // forward immediately so consumers get a deterministic empty result. + // After MAX_RETRIES attempts, give up and forward whatever was resolved + // so permanently-missing IDs don't block indefinitely. + if (elements.length < ids.length && elements.length > 0) { + const retries = this.#retryCounts.get(name) ?? 0; + if (retries < ForwardAriaImpl.#MAX_RETRIES) { + this.#retryCounts.set(name, retries + 1); + unresolved.add(name); + continue; + } + // Fallback: forward partially-resolved elements. + // Clean up retry state since we're done trying. + this.#retryCounts.delete(name); + } + + // Clean up retry count on successful resolution. + this.#retryCounts.delete(name); - if (elementsProp.endsWith('Elements')) { - (targetElement as unknown as Record)[elementsProp] = elements; + // Mirror the element reference in propertyStorage so that external code + // (e.g. tooltip anchor matching) can discover the relation via the host. + let stored = propertyStorage.get(this); + if (!stored) { + stored = new Map(); + propertyStorage.set(this, stored); + } + const refValue = elementsProp.endsWith('Elements') ? elements : (elements[0] ?? null); + stored.set(elementsProp, refValue); + + // If no elements could be resolved at all, handle based on scope: + // - Same scope: forward the string IDREF attribute anyway — the browser's + // native resolution works dynamically and will pick up elements that + // appear in the DOM later. + // - Cross scope: forward an empty array so consumers reading the native + // property get a deterministic []. Element references can't resolve + // natively across shadow boundaries. + if (elements.length === 0) { + const targetRoot = targetElement.getRootNode(); + if (targetRoot === root) { + targetElement.setAttribute(name, value); + } else { + (targetElement as unknown as Record)[ + elementsProp + ] = refValue; + } + this.removeAttribute(name); + this.#storedValues.delete(name); + continue; + } + + // Determine the target's root to decide how to forward the relationship. + const targetRoot = targetElement.getRootNode(); + + if (targetRoot === root) { + // Same scope: the string attribute reliably resolves IDs. Do NOT set + // element reference properties here — Chrome replaces the content attribute + // with an empty marker when ariaLabelledByElements/ariaDescribedByElements + // is assigned, which breaks the accessibility tree. + targetElement.setAttribute(name, value); } else { - (targetElement as unknown as Record)[elementsProp] = - elements[0] ?? null; + // Cross-scope (target is inside a shadow root): string IDs cannot resolve + // across the shadow boundary, so element references are required. + (targetElement as unknown as Record)[elementsProp] = + refValue; } } else { targetElement.setAttribute(name, value); } this.removeAttribute(name); + this.#storedValues.delete(name); } - this.#pendingAttributes.clear(); + this.#pendingAttributes = unresolved; + + // If some IDs could not be resolved, retry to give sibling elements time to + // connect to the DOM. Use both setTimeout (new macrotask, after all synchronous + // rendering) and rAF (next frame) to cover different rendering pipelines. + if (unresolved.size > 0) { + setTimeout(() => this.#forwardAttributes(), 0); + requestAnimationFrame(() => this.#forwardAttributes()); + } } } @@ -252,7 +391,29 @@ export function ForwardAriaMixin< const target = targetElements.get(this); if (target) { - (target as unknown as Record)[prop] = value; + // Only forward non-empty values to the target. Forwarding null or empty arrays + // can cause the browser to add an empty aria-describedby/aria-labelledby content + // attribute on the target, which interferes with deferred ID resolution. + const isEmpty = value === null || (Array.isArray(value) && value.length === 0); + if (!isEmpty) { + const root = (this as unknown as Element).getRootNode(); + const targetRoot = target.getRootNode(); + + if (targetRoot === root) { + // Same scope: use string attribute to avoid Chrome clearing it. + const attrName = Object.entries(ELEMENT_REFERENCES).find(([, p]) => p === prop)?.[0]; + if (attrName) { + const elements = Array.isArray(value) ? value : [value]; + const ids = elements.map(el => ensureId(el as HTMLElement)).join(' '); + if (ids) { + target.setAttribute(attrName, ids); + } + } + } else { + // Cross-scope: element references required. + (target as unknown as Record)[prop] = value; + } + } } } }); diff --git a/packages/components/tooltip/src/tooltip.ts b/packages/components/tooltip/src/tooltip.ts index 7e093cda5f..90e09c65ab 100644 --- a/packages/components/tooltip/src/tooltip.ts +++ b/packages/components/tooltip/src/tooltip.ts @@ -118,7 +118,7 @@ export class Tooltip extends LitElement { } }; - const createTooltip = (): void => { + const createTooltip = (event?: Event): void => { if (created) { return; } @@ -166,6 +166,18 @@ export class Tooltip extends LitElement { // We only need to create the tooltip once, so ignore all future events. removeListeners(); + + // When the tooltip is created in response to a focusin event, the tooltip's own + // document-level focusin listener missed the original event. Dispatch a synthetic + // focusin on the owning document so the tooltip's normal show logic kicks in + // without re-triggering focusin listeners on the anchor or its ancestors. + if (event?.type === 'focusin') { + requestAnimationFrame(() => { + (target.ownerDocument ?? document).dispatchEvent( + new FocusEvent('focusin', { bubbles: true, composed: true }) + ); + }); + } }; const cleanup = () => { @@ -1313,6 +1325,20 @@ export class Tooltip extends LitElement { while (true) { const rootNode = this.#getShadowRoot(normalized.getRootNode()); if (!rootNode) { + // For light DOM proxy targets: if the element's parent has getProxyTarget() + // pointing to this element, normalize to the parent (e.g. an in the + // light DOM of sl-checkbox where sl-checkbox is the actual anchor). + const parent = normalized.parentElement; + if (parent instanceof HTMLElement) { + const parentProxy = ( + parent as HTMLElement & { getProxyTarget?(): Element | null } + ).getProxyTarget?.(); + + if (parentProxy === normalized && this.#matchesAnchor(parent)) { + return parent; + } + } + return normalized; }