Skip to content

fix(action-button): track label content changes after first render - #6586

Open
rubencarvalho wants to merge 3 commits into
mainfrom
rubencarvalho/fix-action-button-label-slotchange
Open

fix(action-button): track label content changes after first render#6586
rubencarvalho wants to merge 3 commits into
mainfrom
rubencarvalho/fix-action-button-label-slotchange

Conversation

@rubencarvalho

Copy link
Copy Markdown
Contributor

Description

<swc-action-button>'s default <slot> wasn't bound to SlotTextController's handleSlotChange. hasLabel (and therefore the swc-ActionButton--iconOnly class) was only correct for content present at connect; label text added or removed afterward wasn't tracked.

Motivation and context

SlotTextController requires the observed slot to bind @slotchange for post-first-render changes to be tracked (its MutationObserver only watches characterData, not childList). swc-button, swc-badge, swc-illustrated-message, and swc-card all bind this correctly; swc-action-button was the one outlier, so a consumer app that dynamically clears or sets an action button's label (e.g. a framework re-render) would see the button stay icon-only-styled (or vice versa) with stale spacing until an unrelated property change forced a re-render.

I audited every consumer of SlotTextController (Button.base.ts, Badge.base.ts, IllustratedMessage.base.ts, Card.base.ts) and their concrete render templates; ActionButton.ts was the only one missing the binding. CloseButton overrides hasIcon/hasLabel directly and doesn't use the controller, so it's unaffected.

Related issue(s)

  • fixes N/A (found via code review)

Author's checklist

  • I have read the CONTRIBUTING and PULL_REQUESTS documents.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
  • I have added automated tests to cover my changes.
  • I have included a well-written changeset if my change needs to be published.
  • I have included updated documentation if my change required it.

Reviewer's checklist

  • Includes a Github Issue with appropriate flag or Jira ticket number without a link
  • Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
  • Automated tests cover all use cases and follow best practices for writing
  • Validated on all supported browsers
  • All VRTs are approved before the author can update Golden Hash

Manual review test cases

  • Dynamic label removal collapses to icon-only

    1. Open Storybook, Action button > Tests > Dynamic label behavior
    2. Run the play function (or manually: render <swc-action-button><svg slot="icon">...</svg>Edit</swc-action-button>, then remove the text node)
    3. Expect the internal <button> to gain swc-ActionButton--iconOnly and tighten its padding
  • Dynamic label addition restores label layout

    1. Start from an icon-only action button
    2. Append a text label at runtime
    3. Expect swc-ActionButton--iconOnly to be removed and label spacing to return

Device review

  • Did it pass in Desktop?
  • Did it pass in (emulated) Mobile?
  • Did it pass in (emulated) iPad?

Accessibility testing checklist

  • Keyboard (required — document steps below)

    1. No new focusable parts are introduced by this change.
    2. Tab to the action button in the "Dynamic label behavior" story; confirm the focus ring renders identically before and after the label mutation.
    3. Expect no change in tab order or focus behavior.
  • Screen reader (required — document steps below)

    1. With VoiceOver/NVDA running, focus the action button in the "Dynamic label behavior" story before the label is removed.
    2. Trigger the label removal (via the play function or devtools), then re-focus or re-announce the button.
    3. Expect the accessible name to still reflect the current accessible-label/visible text combination — the fix only corrects the internal --iconOnly styling class, not aria-label derivation, which was already correct.

ActionButton's default slot wasn't bound to the SlotTextController's
handleSlotChange, so hasLabel only reflected content present at
connect. Dynamically adding/removing the label in a consumer app left
the --iconOnly class stale. Button, Badge, IllustratedMessage, and
Card already bind this correctly; ActionButton was the outlier.
@rubencarvalho
rubencarvalho requested a review from a team as a code owner August 6, 2026 14:36
@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: e37c893

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

This PR includes changesets to release 1 package
Name Type
@adobe/spectrum-wc 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

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

📚 Branch Preview Links

🔍 Gen1 Visual Regression Test Results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Deployed to Azure Blob Storage: pr-6586

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

@rubencarvalho rubencarvalho added Component:Action button Status:Ready for review PR ready for review or re-review. run_vrt Triggers the Chromatic VRT run for 2nd-gen labels Aug 6, 2026
@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 31117883006

Warning

No base build found for commit 8c19c55 on main.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 96.239%

Details

  • Patch coverage: No coverable lines changed in this PR.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 39268
Covered Lines: 37993
Line Coverage: 96.75%
Relevant Branches: 6464
Covered Branches: 6019
Branch Coverage: 93.12%
Branches in Coverage %: Yes
Coverage Strength: 460.21 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component:Action button run_vrt Triggers the Chromatic VRT run for 2nd-gen Status:Ready for review PR ready for review or re-review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants