feat(tooltip): simplify implementation#3368
Open
jpzwarte wants to merge 57 commits into
Open
Conversation
jpzwarte
requested review from
Diaan,
a11ymiko,
anna-lach and
michal-sanoma
as code owners
🦋 Changeset detectedLatest commit: 2d1fd1d The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
jpzwarte
changed the title
tooltip property to sl-buttontooltip directly on button
Contributor
🕸 Storybook previewYou can view a preview here (commit |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a tooltip property to @sl-design-system/button (<sl-button>) so consumers can attach an <sl-tooltip> directly to the button and have ARIA relationships applied automatically, improving ergonomics especially for icon-only buttons.
Changes:
- Added a new
tooltipproperty tosl-buttonand render an internal<sl-tooltip>via scoped elements. - Wired tooltip-based ARIA (
aria-describedbyfor text buttons,aria-labelledbyfor icon-only buttons) and added logic to merge forwardedaria-labelledbyelements. - Added Storybook and unit test updates plus a changeset for a minor release.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Updates workspace dependency metadata for the button package (adds tooltip/scoped-elements/lit entries). |
packages/components/button/src/button.ts |
Implements the tooltip property, internal tooltip rendering, and ARIA wiring/merging logic. |
packages/components/button/src/button.stories.ts |
Updates stories to demonstrate tooltip usage, including icon-only example. |
packages/components/button/src/button.spec.ts |
Adds unit tests for tooltip rendering and ARIA behavior. |
packages/components/button/package.json |
Adds @sl-design-system/tooltip dependency and declares scoped-elements/lit peer/dev dependencies. |
.changeset/afraid-parents-nail.md |
Declares a minor release for the new tooltip API. |
Contributor
🕸 Website previewYou can view a preview here (commit |
a11ymiko
reviewed
Jun 3, 2026
a11ymiko
left a comment
a11ymiko
left a comment
Contributor
There was a problem hiding this comment.
For stories in Tooltip2 group (Basic, Hover Bridge, Positions) tooltip is not showed on focus.
Screen.Recording.2026-06-03.at.14.44.36.mov
a11ymiko
approved these changes
Jun 3, 2026
michal-sanoma
approved these changes
Jun 5, 2026
michal-sanoma
left a comment
michal-sanoma
left a comment
Collaborator
There was a problem hiding this comment.
Amazing work with this refactor! 🚀
Small compatibility clarification: do we still want to support shared / multi-anchor tooltips?
The old implementation allowed multiple anchors to point to the same <sl-tooltip> via ARIA attributes, and we had coverage for that in the Shared story and tooltip-shared.spec.ts. The new for API appears to model a single tooltip-to-anchor relationship instead
If that change is intentional, I think it would be helpful for consumers if we mention it as a breaking change and suggest the preferred replacement pattern, wdyt?
Diaan
approved these changes
Jun 8, 2026
|
|
||
| Refactor toggle button to use an internal `<button>` element. This improves accessibility and removes the need for manual keyboard and ARIA handling. | ||
|
|
||
| **Breaking changes** |
Collaborator
There was a problem hiding this comment.
with this many breaking changes, is it still a minor version update?
| }); | ||
|
|
||
| it('should not move focus when the button is clicked without being focused', async () => { | ||
| it('should focus the first menu item when the button is clicked', async () => { |
Collaborator
There was a problem hiding this comment.
I'm not sure this is correct. When i click the button it doesn't show a focusring on the first menu item:
https://storybook-3368.d3c5h2uor7jlkj.amplifyapp.com/?path=/story/actions-menu-menu-button--submenu
| <span>aria-disabled</span> | ||
|
|
||
| <span>md</span> | ||
| <sl-menu-button aria-label="Label"> |
Collaborator
There was a problem hiding this comment.
these need to be updated as well
| expect(el).to.have.attribute('aria-describedby', tooltip?.id); | ||
| }); | ||
|
|
||
| it('should be icon-only even if one icon is missing (Errors variant)', async () => { |
| } | ||
| } | ||
|
|
||
| if (changes.has('disabled')) { |
| description: 'This example shows a tool bar with different tooltip techniques on the buttons.', | ||
| items: () => html` | ||
| <sl-button aria-labelledby="tooltip-bold" fill="outline"> | ||
| <sl-button aria-labelledby="tooltip-bold" fill="outline" id="bold"> |
Collaborator
There was a problem hiding this comment.
Does it still need the aria-labelledby="tooltip-bold"?
Comment on lines
+43
to
47
| // The `tooltip` attribute on sl-button provides the accessible label for icon-only | ||
| // buttons at runtime, so it counts as an accessible name at lint time. | ||
| const hasTooltipAttribute = ((element.attribs ?? {})['tooltip'] ?? '').trim() !== ''; | ||
|
|
||
| if ( |
Comment on lines
224
to
+228
| if (elementsProp.endsWith('Elements')) { | ||
| (targetElement as unknown as Record<string, Element[]>)[elementsProp] = elements; | ||
| const elementsPropValue = | ||
| (targetElement as unknown as Record<string, Element[]>)[elementsProp] ?? []; | ||
|
|
||
| // Make sure we don't override any existing references |
Comment on lines
38
to
42
| this.renderRoot | ||
| .querySelector('slot') | ||
| ?.assignedElements({ flatten: true }) | ||
| .filter((element): element is ToggleButton => element instanceof ToggleButton) ?? [] | ||
| .filter(element => element instanceof ToggleButton) ?? [] | ||
| ); |
Comment on lines
+333
to
+337
| // Use an existing tooltip, or create a new one | ||
| let tooltip = crumb.ariaLabelledByElements?.find( | ||
| (el): el is Tooltip => el instanceof Tooltip && el.for === crumb.id | ||
| ); | ||
| this.#tooltipCleanupFunctions.set(link, cleanup); | ||
| link.dataset['hasTooltip'] = 'true'; | ||
| } | ||
| } else if (link.hasAttribute('data-has-tooltip') && link.hasAttribute('aria-describedby')) { | ||
| const cleanup = this.#tooltipCleanupFunctions.get(link); | ||
| if (cleanup) { | ||
| cleanup(); | ||
| } | ||
| this.#tooltipCleanupFunctions.delete(link); | ||
|
|
||
| const tooltip = tooltipsSlot | ||
| .assignedElements() | ||
| .find(el => el.id === link.getAttribute('aria-describedby')) as Tooltip | undefined; | ||
| tooltip?.remove(); | ||
| link.removeAttribute('data-has-tooltip'); | ||
| link.removeAttribute('aria-describedby'); | ||
| } | ||
| if (!tooltip) { |
Comment on lines
182
to
190
| #onResize(): void { | ||
| const slot = this.renderRoot.querySelector('slot'); | ||
|
|
||
| if (slot && slot.clientWidth < slot.scrollWidth) { | ||
| this.#tooltip ||= Tooltip.lazy( | ||
| this, | ||
| tooltip => { | ||
| this.#tooltip = tooltip; | ||
| tooltip.textContent = this.label; | ||
| }, | ||
| { context: this.shadowRoot! } | ||
| ); | ||
| } else if (this.#tooltip instanceof Tooltip) { | ||
| this.#tooltip.remove(); | ||
| this.#tooltip = undefined; | ||
| } else if (this.#tooltip) { | ||
| this.#tooltip(); | ||
| this.#tooltip = undefined; | ||
| if (typeof this.tooltip === 'string') { | ||
| return; | ||
| } | ||
|
|
||
| // If the contents of the tag overflows, make sure it is keyboard focusable, | ||
| // so the user can tab to it. | ||
| if (!this.disabled && (this.removable || this.#tooltip)) { | ||
| this.setAttribute('tabindex', '0'); | ||
| } else if (!this.hasAttribute('aria-labelledby')) { | ||
| this.removeAttribute('tabindex'); | ||
| } | ||
| const label = this.renderRoot.querySelector('[part="label"]'); | ||
|
|
||
| this.tooltip = !!label && label.clientWidth < label.scrollWidth; | ||
| } |
Comment on lines
+225
to
+232
| const elementsPropValue = | ||
| (targetElement as unknown as Record<string, Element[]>)[elementsProp] ?? []; | ||
|
|
||
| // Make sure we don't override any existing references | ||
| (targetElement as unknown as Record<string, Element[]>)[elementsProp] = [ | ||
| ...elementsPropValue, | ||
| ...elements | ||
| ]; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3344
Closes #3287
Closes #3275
Closes #3255
Closes #3343
Closes #3274
Closes #3049
Closes #3264
Overview
This PR rewrites the
sl-tooltipcomponent and simplifies how tooltips are used across the design system. The new implementation replaces the complex internal machinery with native browser APIs and introduces a first-classtooltipproperty on several components.@sl-design-system/tooltip— major rewriteThe tooltip was the component responsible for the most bug reports and had a complex internal implementation. This rewrite significantly simplifies it.
The
AnchorController,EventsController, andTooltip.lazy()static method have all been removed in favour of native browserpopoverand CSS Anchor Positioning APIs.Note
CSS Anchor Positioning is not yet supported in all browsers. You may need to include the CSS Anchor Positioning polyfill in your application.
Breaking changes
TooltipOptionsinterface andTooltip.lazy()static method have been removed. Use theforattribute to link a tooltip to its anchor instead.position,offset,maxWidth,arrowPadding, andviewportMarginproperties/statics have been removed.hoverShowDelaychanged from500msto150msandhoverHideDelaychanged from200msto0ms.New API
for— links the tooltip to an anchor element by IDtype— controls the ARIA relationship:'label'(ariaLabelledByElements, default) or'description'(ariaDescribedByElements)trigger— space-separated list of triggers:'focus','hover', and/or'click'(default:'focus hover')disabled— prevents the tooltip from showingopen— reflects the current open statetooltipproperty on components — minorPreviously, adding a tooltip to a component required adding a sibling
<sl-tooltip>element manually and wiring up the correctaria-describedbyoraria-labelledbyrelationship by hand. The newtooltipproperty improves the DX by letting you set a tooltip directly on the component, with all accessibility wiring handled automatically:aria-labelledby).aria-describedby).Affected components:
@sl-design-system/button,@sl-design-system/menu,@sl-design-system/tag,@sl-design-system/toggle-button.@sl-design-system/toggle-button— minor refactorRefactored to use an internal
<button>element. This improves accessibility and removes the need for manual keyboard and ARIA handling.Breaking changes
[pressed],[icon-only],[text-only], and[error]attributes have been replaced by CSS custom states (:state(pressed),:state(icon-only),:state(text-only),:state(error)).shapeproperty type changed fromButtonShapetoToggleButtonShape('square' | 'pill').pressedproperty is no longer reflected as an attribute.labelproperty has been removed. Use the newtooltipproperty instead.New
buttonandtooltipfor styling the internal elements.<button>viadelegatesFocus: true.<button>viaForwardAriaMixin.@sl-design-system/shared— minorForwardAriaMixinno longer overrides existingariaDescribedByElementsorariaLabelledByElementsvalues. Components can now maintain their own ARIA relationships without interference.Other components updated to use the new tooltip implementation — minor
@sl-design-system/avatar@sl-design-system/breadcrumbs— tooltips for truncated links now use theforattribute approach, removing the need for manual cleanup functions.@sl-design-system/calendar@sl-design-system/ellipsize-text@sl-design-system/grid@sl-design-system/angular— tooltip directive updated; API unchanged but DOM structure may have changed.Bug fixes
@sl-design-system/menu— Fixed: clicking the menu button a second time did not dismiss the menu. The button's click handler was firing after the popover'stoggleevent and callingtogglePopover()on an already-closed menu. Fixed by tracking a#popoverJustClosedflag via thebeforetoggleevent.@sl-design-system/button-bar— Fixed: buttons were not given enough time to set their icon-only state.