feat(checkbox): replace `RovingTabIndexController` with `focusgroup` by jpzwarte · Pull Request #3566 · sl-design-system/components

jpzwarte · 2026-07-31T09:35:04Z

Replaces the custom RovingTabindexController keyboard navigation with the native focusgroup HTML attribute (via @microsoft/focusgroup-polyfill) in the checkbox-group component, as part of the investigation for issue #3187. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-07-31T09:38:10Z

🕸 Preview deploys

Storybook: https://storybook-3566.d3c5h2uor7jlkj.amplifyapp.com
Website: https://website-3566.d3c5h2uor7jlkj.amplifyapp.com

Copilot

Pull request overview

This PR migrates sl-checkbox-group away from the custom RovingTabindexController toward the emerging platform focusgroup behavior, using @microsoft/focusgroup-polyfill to keep behavior working in non-supporting environments.

Changes:

Added @microsoft/focusgroup-polyfill and loaded it in Vitest and Storybook environments.
Refactored sl-checkbox-group to use a focusgroup wrapper instead of RovingTabindexController, plus updated styling and stories.
Updated checkbox-group tests for the new behavior (but currently includes focused-test/debug artifacts).

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`vitest.setup.ts`	Loads `focusGroup` polyfill during tests when needed.
`.storybook/preview.ts`	Loads `focusGroup` polyfill for Storybook rendering.
`packages/components/checkbox/src/checkbox-group.ts`	Replaces roving tabindex controller usage with `focusgroup` wrapper and refactors event wiring.
`packages/components/checkbox/src/checkbox-group.scss`	Moves layout styling to the new wrapper part.
`packages/components/checkbox/src/checkbox-group.stories.ts`	Adds a tooltip-focused story to exercise navigation with extra focusable content.
`packages/components/checkbox/src/checkbox-group.spec.ts`	Adjusts assertions and navigation test for the new focus behavior.
`package.json`	Adds `@microsoft/focusgroup-polyfill` dependency.

Suppressed comments (1)

packages/components/checkbox/src/checkbox-group.spec.ts:237

Remove debug console.log statements from tests to keep test output clean and avoid noise in CI logs.

      console.log(el.renderRoot.querySelector('[part="wrapper"]')?.getAttribute('focusgroup')); // toolbar block wrap
      // console.log(el.renderRoot.querySelector('[part="wrapper"]')?.focusGroup); // undefined

    this.#observer.observe(this, OBSERVER_OPTIONS);

-    this.internals.role = 'group';
    this.setFormControlElement(this);


+    requestAnimationFrame(() =>
+      this.setProxyTarget(this.renderRoot.querySelector('[part="wrapper"]')!)
+    );


+   *
+   * @default false
+   */
+  @property({ type: Boolean }) override disabled?: boolean;


+   *
+   * @default false
+   */
+  @property({ type: Boolean }) override required?: boolean;


changeset-bot · 2026-07-31T11:06:21Z

🦋 Changeset detected

Latest commit: 5806141

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@sl-design-system/checkbox	Major
@sl-design-system/angular	Patch
@sl-design-system/grid	Patch
@sl-design-system/tree	Patch

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

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (9)

packages/components/checkbox/src/checkbox-group.ts:88

required no longer reflects to an attribute. This diverges from the established pattern for form controls in this repo (e.g. radio-group/src/radio-group.ts:140, checkbox/src/checkbox.ts:113, select/src/select.ts:223) and is a breaking API change for attribute-based styling/testing.

  @property({ type: Boolean }) override required?: boolean;

.storybook/preview.ts:28

The console.log inside the Storybook preview polyfill block is leftover debug output and will spam the browser console for all stories.

if (!('focusGroup' in HTMLElement.prototype)) {
  const { polyfillBodyAndObserve } = await import('@microsoft/focusgroup-polyfill');

  console.log('Applying focusGroup polyfill', polyfillBodyAndObserve);
  polyfillBodyAndObserve();
}

packages/components/checkbox/src/checkbox-group.ts:128

requestAnimationFrame(() => this.setProxyTarget(this.renderRoot.querySelector(... )!)) can run before the wrapper is rendered; if querySelector returns null this will throw at runtime. ForwardAriaMixin also documents calling setProxyTarget typically in firstUpdated.

    requestAnimationFrame(() =>
      this.setProxyTarget(this.renderRoot.querySelector('[part="wrapper"]')!)
    );

packages/components/checkbox/src/checkbox-group.ts:114

internals.role is no longer set. Other form-associated group controls (e.g. sl-radio-group sets internals.role = 'radiogroup') use ElementInternals to expose the correct ARIA role on the host; relying only on a shadow child role="group" can reduce AT interoperability.

    this.#observer.observe(this, OBSERVER_OPTIONS);

    this.setFormControlElement(this);

packages/components/checkbox/src/checkbox-group.ts:81

disabled no longer reflects to an attribute. In this codebase, form control disabled is consistently reflect: true (e.g. radio-group/src/radio-group.ts:130, checkbox/src/checkbox.ts:96, select/src/select.ts:184), and removing reflection is a breaking change for users relying on attribute selectors and SSR.

  @property({ type: Boolean }) override disabled?: boolean;

packages/components/checkbox/src/checkbox-group.ts:200

sl-checkbox-group no longer overrides focus(). A number of existing tests/usages call el.focus() expecting focus to move into the first focusable checkbox; without an override, HTMLElement.focus() on a custom element is typically a no-op unless it is made focusable via tabindex. This is a behavioral breaking change.

  override reportValidity(): boolean {
    this.boxes?.forEach(box => box.reportValidity());

    return super.reportValidity();
  }

packages/components/checkbox/src/checkbox-group.spec.ts:232

it.only(...) will cause Vitest to run only this single test and skip the rest of the suite, which can mask failures in other tests.

    it.only('should handle navigating between options correctly', async () => {

packages/components/checkbox/src/checkbox-group.spec.ts:237

Leftover debug logging in a test will add noise to CI output and can slow down runs. This should be removed before merging.

      console.log(el.renderRoot.querySelector('[part="wrapper"]')?.getAttribute('focusgroup')); // toolbar block wrap
      // console.log(el.renderRoot.querySelector('[part="wrapper"]')?.focusGroup); // undefined

packages/components/checkbox/src/checkbox-group.ts:209

Clicking the host (e.g. via an associated <label>) currently focuses boxes?.at(0), even if the first checkbox is disabled. This can leave focus on a non-interactive control; the old roving logic focused the first enabled option.

  #onClick = (event: Event): void => {
    if (event.target === this) {
      this.boxes?.at(0)?.focus();
    }

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

packages/components/checkbox/src/checkbox-group.ts:210

sl-checkbox-group no longer overrides focus(), so calling el.focus() won’t move focus into the group (the existing tests expect focus to land on the first checkbox input). Also, clicking an associated <label> currently focuses boxes.at(0) even if it’s disabled; the previous behavior focused the first enabled option.

  #onClick = (event: Event): void => {
    if (event.target === this) {
      this.boxes?.at(0)?.focus();
    }
  };

packages/components/checkbox/src/checkbox-group.ts:128

setProxyTarget() is invoked via requestAnimationFrame() in connectedCallback() with a non-null assertion. If the first render hasn’t happened yet, querySelector('[part="wrapper"]') can be null and throw. ForwardAriaMixin also documents calling setProxyTarget() in firstUpdated (or after the initial render).

    requestAnimationFrame(() =>
      this.setProxyTarget(this.renderRoot.querySelector('[part="wrapper"]')!)
    );

packages/components/checkbox/src/checkbox-group.ts:81

disabled no longer reflects to an attribute. Most form controls in this repo reflect disabled (e.g. radio-group.ts:131), and removing reflection can be a breaking API change for styling and consumers that observe attributes.

  @property({ type: Boolean }) override disabled?: boolean;

packages/components/checkbox/src/checkbox-group.ts:88

required no longer reflects to an attribute. Most form controls in this repo reflect required (e.g. radio-group.ts:140), and removing reflection can be a breaking API change for styling and consumers that observe attributes.

  @property({ type: Boolean }) override required?: boolean;

packages/components/checkbox/src/checkbox-group.spec.ts:244

After {ArrowDown}, focus will be inside the second checkbox’s internal <input> (or its proxy), not on the sl-checkbox host itself. Also, the :state(checked) assertions won’t match because sl-checkbox doesn’t use custom states. Use .checked and assert focus against the corresponding input element.

      expect(document.activeElement).to.equal(el.boxes?.at(1));

packages/components/checkbox/src/checkbox-group.ts:184

focusgroup keyboard navigation requires either native support or the polyfill, but this component doesn’t load the polyfill at runtime (it’s only loaded in Storybook/Vitest setup). In browsers without native focusgroup, arrow-key roving behavior will regress unless consumers are instructed to include the polyfill.

      <div focusgroup="toolbar block wrap" part="wrapper" role="group">

vitest.setup.ts:13

Feature-detecting focusgroup support via 'focusGroup' in HTMLElement.prototype is unreliable for this polyfill (it does not guarantee adding a focusGroup property). This can cause the polyfill to be loaded unnecessarily or repeatedly. Consider guarding with a one-time global flag instead.

if (!('focusGroup' in HTMLElement.prototype)) {
  const { polyfillBodyAndObserve } = await import('@microsoft/focusgroup-polyfill');

  polyfillBodyAndObserve();
}

.storybook/preview.ts:27

Feature-detecting focusgroup support via 'focusGroup' in HTMLElement.prototype is unreliable for this polyfill (it does not guarantee adding a focusGroup property). This can cause the polyfill to be loaded unnecessarily or repeatedly. Consider guarding with a one-time global flag instead.

if (!('focusGroup' in HTMLElement.prototype)) {
  const { polyfillBodyAndObserve } = await import('@microsoft/focusgroup-polyfill');

  polyfillBodyAndObserve();
}

+      expect(boxes[0]).to.match(':state(checked)');
+      expect(boxes[1]).to.match(':state(checked)');
+      expect(boxes[2]).not.to.match(':state(checked)');


… from focusgroup Adds a `checked` custom state to `sl-checkbox` so `:state(checked)` can be used, and opts the infotip slot out of the focusgroup when the checkbox is disabled. Removes the checkbox group tests that relied on the removed `focus()` override. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (6)

packages/components/checkbox/src/checkbox-group.ts:196

sl-checkbox-group no longer overrides focus(). Since the host isn’t focusable (no tabindex), calling .focus() won’t move focus into the group, which is inconsistent with other composite controls (e.g. radio-group overrides focus() to focus the first item).

  }

  override reportValidity(): boolean {

packages/components/checkbox/src/checkbox-group.ts:81

disabled is no longer reflected to an attribute (@property({ reflect: true })), so el.disabled = true won’t produce a [disabled] attribute. Most other form controls in this repo reflect disabled (e.g. radio-group/src/radio-group.ts:131), and consumers often rely on [disabled] for styling and state introspection.

  @property({ type: Boolean }) override disabled?: boolean;

packages/components/checkbox/src/checkbox-group.ts:88

required is no longer reflected to an attribute, so el.required = true won’t produce a [required] attribute. Other comparable controls reflect required (e.g. radio-group/src/radio-group.ts:140), and dropping reflection is a breaking DOM/API behavior change for consumers using attribute selectors.

  @property({ type: Boolean }) override required?: boolean;

packages/components/checkbox/src/checkbox-group.ts:184

role="group" is applied to the internal [part="wrapper"], but the group’s required state is set on the host via this.internals.ariaRequired (see willUpdate). That means the element with role="group" won’t reflect aria-required, so assistive tech may not announce the group as required.

      <div focusgroup="toolbar block wrap" part="wrapper" role="group">

packages/components/checkbox/src/checkbox.ts:258

toggle() (and therefore click/keyboard activation) never clears indeterminate. With role="checkbox", a user activation should move from mixed to a determinate checked/unchecked state; otherwise aria-checked stays mixed indefinitely and the checkbox can’t be cleared from indeterminate by user interaction.

  toggle(force?: boolean): void {
    // Changing `checked` schedules an update, and `willUpdate` syncs the form value and validity;
    // doing that here as well would emit `sl-validate` twice per toggle.
    this.checked = force ?? !this.checked;

packages/components/checkbox/src/checkbox-group.ts:209

When the host is clicked (e.g. via an associated <label>), the group focuses boxes.at(0) even if that checkbox is disabled. Previously the roving tabindex logic skipped disabled items; this should focus the first enabled checkbox.

  #onClick = (event: Event): void => {
    if (event.target === this) {
      this.boxes?.at(0)?.focus();
    }

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (4)

packages/components/checkbox/src/checkbox-group.ts:128

setProxyTarget() is scheduled via requestAnimationFrame() in connectedCallback(). This is a bit racy because it assumes the first Lit render has completed by the next frame; the ForwardAriaMixin docs/tests consistently set the proxy target in firstUpdated() after the shadow DOM exists.

Using updateComplete here avoids timing issues without relying on an extra frame.

    requestAnimationFrame(() =>
      this.setProxyTarget(this.renderRoot.querySelector('[part="wrapper"]')!)
    );

vitest.setup.ts:13

The focusgroup polyfill feature-detection checks for focusGroup on HTMLElement.prototype, but focusgroup is an HTML attribute and this check is unlikely to ever become true. This will effectively always load the polyfill and could also conflict with future native implementations if the detection never flips.

Consider either loading the polyfill unconditionally in the test environment (for consistency), or switching to the polyfill package’s recommended detection mechanism instead of focusGroup.

// Load the polyfill for the focusGroup API if needed
if (!('focusGroup' in HTMLElement.prototype)) {
  const { polyfillBodyAndObserve } = await import('@microsoft/focusgroup-polyfill');

  polyfillBodyAndObserve();
}

.storybook/preview.ts:27

The focusgroup polyfill feature-detection checks for focusGroup on HTMLElement.prototype, but focusgroup is an HTML attribute and this check is unlikely to ever become true. This will effectively always load the polyfill and could also conflict with future native implementations if the detection never flips.

Consider either loading the polyfill unconditionally in Storybook (for consistency), or switching to the polyfill package’s recommended detection mechanism instead of focusGroup.

// Load the polyfill for the focusGroup API if needed
if (!('focusGroup' in HTMLElement.prototype)) {
  const { polyfillBodyAndObserve } = await import('@microsoft/focusgroup-polyfill');

  polyfillBodyAndObserve();
}

packages/components/checkbox/src/checkbox.ts:269

toggle(force?) currently emits sl-change and marks the control dirty even if force matches the current checked state (e.g. toggle(true) when already checked). Other components’ toggle(force) helpers (e.g. sl-accordion-item) no-op in that case to avoid redundant events/state churn.

  toggle(force?: boolean): void {
    // Changing `checked` schedules an update, and `willUpdate` syncs the form value and validity;
    // doing that here as well would emit `sl-validate` twice per toggle.
    this.checked = force ?? !this.checked;

jpzwarte and others added 2 commits June 7, 2026 13:58

Merge branch 'main' into maintenance/3187-focusgroup

5d593c1

Copilot AI review requested due to automatic review settings July 31, 2026 09:35

Copilot started reviewing on behalf of jpzwarte July 31, 2026 09:35 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

🍤

e11102a

Copilot AI review requested due to automatic review settings July 31, 2026 11:06

Copilot started reviewing on behalf of jpzwarte July 31, 2026 11:06 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

🚛

d30ad84

Copilot AI review requested due to automatic review settings July 31, 2026 11:17

Copilot started reviewing on behalf of jpzwarte July 31, 2026 11:18 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Comment thread packages/components/checkbox/src/checkbox-group.spec.ts

Comment on lines +85 to +87

expect(boxes[0]).to.match(':state(checked)');

expect(boxes[1]).to.match(':state(checked)');

expect(boxes[2]).not.to.match(':state(checked)');

jpzwarte and others added 2 commits July 31, 2026 13:40

🌴

b53097f

Copilot AI review requested due to automatic review settings July 31, 2026 12:06

Copilot started reviewing on behalf of jpzwarte July 31, 2026 12:07 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

🌴

5806141

Copilot AI review requested due to automatic review settings August 3, 2026 07:25

Copilot started reviewing on behalf of jpzwarte August 3, 2026 07:26 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat(checkbox): replace `RovingTabIndexController` with `focusgroup` - #3566

feat(checkbox): replace `RovingTabIndexController` with `focusgroup`#3566
jpzwarte wants to merge 7 commits into
mainfrom
maintenance/3187-focusgroup

jpzwarte commented Jul 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

changeset-bot Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jpzwarte commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🕸 Preview deploys

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

changeset-bot Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jpzwarte commented Jul 31, 2026 •

edited

Loading

github-actions Bot commented Jul 31, 2026 •

edited

Loading

changeset-bot Bot commented Jul 31, 2026 •

edited

Loading