Skip to content

fix(checkbox-group): fix the checkbox-group to get it working properly with tooltips#3322

Draft
anna-lach wants to merge 13 commits into
mainfrom
fix/3287-checkbox-tooltip-breaks-sl-checkbox-group
Draft

fix(checkbox-group): fix the checkbox-group to get it working properly with tooltips#3322
anna-lach wants to merge 13 commits into
mainfrom
fix/3287-checkbox-tooltip-breaks-sl-checkbox-group

Conversation

@anna-lach
Copy link
Copy Markdown
Collaborator

@anna-lach anna-lach commented May 14, 2026

Fixes #3287

…y with tooltips, unit tests and WithTooltips story
Copilot AI review requested due to automatic review settings May 14, 2026 11:14
@anna-lach anna-lach linked an issue May 14, 2026 that may be closed by this pull request
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 14, 2026

🦋 Changeset detected

Latest commit: 3bb66db

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

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

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package "@sl-design-system/grid" depends on the ignored package "@sl-design-system/example-data", but "@sl-design-system/grid" is not being ignored. Please add "@sl-design-system/grid" to the `ignore` option.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes sl-checkbox-group behavior when non-checkbox elements (e.g., sl-tooltip) are present in its default slot, ensuring the group only operates on actual checkboxes.

Changes:

  • Restrict the group’s slotted-element query to sl-checkbox elements only.
  • Add a Storybook scenario demonstrating a checkbox group with tooltips in the slot.
  • Add regression tests verifying the group ignores sl-tooltip elements when computing boxes and value.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/components/checkbox/src/checkbox-group.ts Filters @queryAssignedElements to only return sl-checkbox, preventing tooltip elements from being treated as checkboxes.
packages/components/checkbox/src/checkbox-group.stories.ts Adds a WithTooltips story to reproduce/verify checkbox-group behavior with tooltips present.
packages/components/checkbox/src/checkbox-group.spec.ts Adds regression coverage for tooltip-in-slot scenarios and registers the tooltip element for the test environment.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

🕸 Website preview

You can view a preview here (commit 3bb66db7b082156aa5766ca430c849e54331261c).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

🕸 Storybook preview

You can view a preview here (commit 3bb66db7b082156aa5766ca430c849e54331261c).

Copilot AI review requested due to automatic review settings May 14, 2026 12:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@anna-lach anna-lach marked this pull request as ready for review May 14, 2026 13:04
Copy link
Copy Markdown
Contributor

@a11ymiko a11ymiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text from tooltips is not announced by VoiceOver nor NVDA.
Adding aria-describedby=TOOLTIP_ID to input instead of sl-checkbox may fix this issue.
Current behavior:

Screen.Recording.2026-05-14.at.16.48.56.mov
Screen.Recording.2026-05-14.at.16.51.37.mov

Possible solution:

Image

Copy link
Copy Markdown
Contributor

@a11ymiko a11ymiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkbox labels are not announced by NVDA in browse mode (navigation with arrows) for Checkbox With Tooltip story. Checkboxes are announced as 'clicable chechk box required invalid entry' without name, that is provided by label.
This is announced properly in Checkbox group With Tooltips story.

Current behavior:

Screen.Recording.2026-05-14.at.17.42.26.mov

Expected behavior:

Screen.Recording.2026-05-14.at.17.43.06.mov

Copilot AI review requested due to automatic review settings May 15, 2026 10:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings May 18, 2026 06:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings May 19, 2026 09:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment thread packages/components/shared/src/mixins/forward-aria-mixin.ts
Comment thread packages/components/shared/src/mixins/forward-aria-mixin.ts
Copilot AI review requested due to automatic review settings May 19, 2026 10:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comment thread packages/components/tooltip/src/tooltip.ts Outdated
Comment thread packages/components/shared/src/mixins/forward-aria-mixin.ts
Comment thread packages/components/shared/src/mixins/forward-aria-mixin.ts
Comment thread packages/components/shared/src/mixins/forward-aria-mixin.ts
Comment thread packages/components/shared/src/mixins/forward-aria-mixin.ts
@anna-lach anna-lach requested a review from Copilot May 19, 2026 14:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Comment on lines +293 to +297
// If no elements could be resolved at all and the target is in the same
// scope, just remove the attribute from the host — there's nothing useful
// to set as a string attribute. For cross-scope targets, still forward the
// empty array so consumers reading the native property get a deterministic [].
if (elements.length === 0) {
Comment thread .changeset/empty-cougars-cross.md Outdated
Comment thread packages/components/checkbox/src/checkbox.spec.ts Outdated
Comment thread packages/components/checkbox/src/checkbox.spec.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

---

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.
Comment on lines +341 to +342
resolves IDs to element references and forwards them to the focusable input element, which
properly crosses the shadow DOM boundary.
Comment on lines +213 to +220
<sl-checkbox ${tooltip('Newsletter tooltip')} value="newsletter"
>Newsletter</sl-checkbox
>
<sl-checkbox value="promotions" aria-describedby="tooltip1">Promotions</sl-checkbox>
<sl-tooltip id="tooltip1">Promotions tooltip</sl-tooltip>
<sl-checkbox ${tooltip('Product updates tooltip')} value="updates"
>Product updates</sl-checkbox
>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CHECKBOX] tooltip breaks sl-checkbox-group

3 participants