Skip to content

feat(button): extract pending state into reusable core primitives#6439

Open
caseyisonit wants to merge 5 commits into
mainfrom
caseyisonit/refactor-pending-primitives-swc-2296
Open

feat(button): extract pending state into reusable core primitives#6439
caseyisonit wants to merge 5 commits into
mainfrom
caseyisonit/refactor-pending-primitives-swc-2296

Conversation

@caseyisonit

Copy link
Copy Markdown
Contributor

Description

Extracts the pending (busy) state out of ButtonBase into reusable, decoupled 2nd-gen core primitives so any pending-capable component can adopt it, then refactors swc-button and swc-action-button to consume them. Builds on main's current implementation (shared renderPendingSpinner + base-owned state) and supersedes the earlier exploration in #6393.

  • PendingController (@spectrum-web-components/core/controllers/pending-controller) — state only: the 1-second-delayed pendingActive flag, inline-size freeze via --swc-pending-inline-size, and the derived busy accessible name.
  • renderPendingSpinner (@spectrum-web-components/core/directives/pending-spinner) — render-only, token-free directive, relocated out of the swc button package so non-button components can reuse it. The token-based pending-spinner.css fragment stays in swc _lit-styles. (Wires directives in as a new core entry-point category.)
  • PendingMixin (@spectrum-web-components/core/mixins) — declares pending / pending-label, instantiates the controller, and suppresses activation while pending.
  • ButtonBase is now fully pending-agnostic. swc-button and swc-action-button apply PendingMixin; non-pending ButtonBase subclasses (e.g. CloseButton) are unaffected.

Behavior parity

aria-disabled, the derived busy aria-label, and click suppression stay synchronous on pending; the visual treatment (disabled palette, spinner, frozen size) keys off the delayed pendingActive. No public API change to swc-button / swc-action-button. The generated global-*.css is unchanged by the refactor (pending is @global-exclude); the small diff is generator de-staling.

Motivation and context

Pending was inheritance-locked to ButtonBase and the spinner render lived inside the button package, so non-button busy controls couldn't reuse either. This lifts both into dedicated core primitives alongside the other reactive controllers, directives, and mixins.

Planning doc: CONTRIBUTOR-DOCS/03_project-planning/03_components/button/pending-reusable-primitives-plan.md.

Related issue(s)

  • SWC-2296

Author's checklist

  • I have read the CONTRIBUTING and PULL_REQUESTS documents.
  • I have reviewed 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.
  • 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

  • Button + action-button pending visual is unchanged

    1. Open Storybook → Components → Button (and Action Button).
    2. Toggle pending on a labeled instance.
    3. Expect: after ~1s the label/icon hide, width is preserved, disabled colors apply, and the animated spinner appears — identical to before.
  • Pending is announced and non-activatable while focusable

    1. On a pending button/action-button, confirm the internal <button> has aria-disabled="true", no native disabled, and aria-label = the derived "<name>, busy" (or explicit pending-label).
    2. Click/Enter/Space do not activate; Tab still focuses it.
  • Pending controller docs render

    1. Open Storybook → Controllers → Pending controller.
    2. Expect the Docs page, demo-host stories, and hand-authored API table to render with no console errors.

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. Open Storybook → Components → Button / Action Button and Tab to a pending instance.
    2. Confirm it stays in the tab order with a visible focus indicator while pending.
    3. Press Enter and Space; expect no activation while pending. Clear pending and confirm activation works. Confirm a disabled instance is skipped in the tab order (distinct from pending).
  • Screen reader (required — document steps below)

    1. With VoiceOver/NVDA, focus a non-pending instance; confirm name + button role announce.
    2. Set pending; confirm it is announced as unavailable (aria-disabled) and busy by name ("<name>, busy", or the explicit pending-label).
    3. Confirm the spinner is not separately announced (aria-hidden). Clear pending and confirm the name returns to normal.

caseyisonit and others added 3 commits June 22, 2026 14:51
Rescoped plan to extract pending (busy) state into reusable, decoupled
2nd-gen primitives, built on top of main's current implementation
(state in ButtonBase + shared renderPendingSpinner used by button and
action-button).

Confirmed direction:
- PendingController (state) + thin PendingMixin (opt-in) in core
- pendingSpinner directive in core/directives (render-only, token-free)
- ButtonBase stays pending-agnostic; swc-button and swc-action-button
  apply the mixin; non-pending subclasses (e.g. CloseButton) unaffected
- single PR

Supersedes the exploration on caseyisonit/pending-controller (PR #6393).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…C-2296)

Lift the pending (busy) state off ButtonBase into reusable, decoupled
2nd-gen core primitives so any pending-capable component can adopt it,
and refactor button + action-button to consume them.

- PendingController (core/controllers/pending-controller): state only —
  the 1s-delayed pendingActive flag, inline-size freeze via
  --swc-pending-inline-size, and the derived busy accessible name.
- renderPendingSpinner (core/directives/pending-spinner): render-only,
  token-free directive; relocated out of the swc button package. The
  token-based pending-spinner.css fragment stays in swc _lit-styles.
- PendingMixin (core/mixins): declares pending / pending-label, wires the
  controller, and suppresses activation while pending.
- ButtonBase is now fully pending-agnostic; swc-button and
  swc-action-button apply PendingMixin. Non-pending ButtonBase subclasses
  (e.g. CloseButton) are unaffected.
- Generalized the inline-size freeze custom property from the
  button-specific --_swc-button-pending-inline-size to --swc-pending-inline-size.
- Controller stories + per-unit MDX; regenerated global-*.css.

Wires directives as a new core entry-point category (vite.config +
package.json exports/typesVersions). Behavior parity: aria-disabled, the
derived busy aria-label, and click suppression stay synchronous on
`pending`; the visual treatment keys off the delayed `pendingActive`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 83dacab

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

This PR includes changesets to release 2 packages
Name Type
@spectrum-web-components/core Patch
@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 Jun 22, 2026

Copy link
Copy Markdown
Contributor

📚 Branch Preview Links

🔍 First Generation 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-6439

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.

@coveralls

coveralls commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 28112418066

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Warning

No base build found for commit 53f5752 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.246%

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: 39173
Covered Lines: 37902
Line Coverage: 96.76%
Relevant Branches: 6459
Covered Branches: 6017
Branch Coverage: 93.16%
Branches in Coverage %: Yes
Coverage Strength: 458.39 hits per line

💛 - Coveralls

…r (SWC-2296)

Ports the focus-retention coverage from #6411 (authored by Nikki Massaro),
which was merged onto the abandoned caseyisonit/pending-controller branch
and not carried into this rescoped branch.

Adapted to the new primitives design:
- demo-pending-host gains `click-pending` (click starts pending, focusing
  the button, auto-clears after 2s) to demonstrate focus retention across
  the controller-triggered re-render.
- FocusRetained story + per-unit MDX section.
- pending-controller.test.ts play test verifies aria is applied immediately
  on `pending`, the spinner + busy class appear after the delay, and
  shadowRoot.activeElement stays on the same internal button throughout
  (class assertion updated to the demo host's `is-pendingActive`).

Co-Authored-By: Nikki Massaro <5090492+nikkimk@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@caseyisonit caseyisonit added Status:Ready for review PR ready for review or re-review. High priority PR review PR is a high priority and should be reviewed ASAP labels Jun 22, 2026
@caseyisonit caseyisonit marked this pull request as ready for review June 22, 2026 23:02
@caseyisonit caseyisonit requested a review from a team as a code owner June 22, 2026 23:02

@cdransf cdransf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few small things. Looks rad! ✨

// ─────────────────────────
// LIFECYCLE
// ─────────────────────────

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hostDisconnected resets _wasPending = false. On reconnect, hostUpdate is supposed to see _wasPending = false vs host.pending = true and restart the timer — but in Lit 3, reconnect doesn't schedule an update, so hostUpdate never runs.

Suggested change
public hostConnected(): void {
if (this._host.pending) {
this._wasPending = true;
this._activateAfterDelay();
}
}

When the element reconnects while still pending, this restarts the timer immediately. The _wasPending = true prevents hostUpdate from starting a second timer if an update does happen to fire afterward.

import { classMap } from 'lit/directives/class-map.js';
import { ifDefined } from 'lit/directives/if-defined.js';

import { renderPendingSpinner } from '../../../directives/pending-spinner/index.js';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be moved out of directives if it's a plain render function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you say more about this question?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, yeah! It's a plain function that returns HTML and doesn't use the Lit directive func or Directive class. I think it can stay here but the semantics don't necessarily line up with Lit directives.

@property({ type: String, attribute: 'pending-label' })
public pendingLabel?: string;

private _pendingController = new PendingController(this, {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be readonly too? ✨

Rajdeepc

This comment was marked as outdated.

@Rajdeepc

Copy link
Copy Markdown
Contributor

@caseyisonit I think the direction here is right. Decoupling pending state behavior from ButtonBase and moving renderPendingSpinner into core/directives both make sense. The behavior parity, focus-retention handling, tests, and docs all look solid.

But my main concern is that PendingController feels like an extra abstraction layer without providing much additional value. Today, PendingMixin mostly delegates directly to the controller, making the controller the real implementation and the mixin a thin wrapper around it.

The rationale for exposing the controller as an escape hatch also seems a bit weak. Components using the controller still need to satisfy PendingControllerHost (pending, pendingLabel, etc.), which are the same pieces the mixin provides. In practice, it doesn't appear to reduce much integration work compared to simply applying the mixin.

I wonder if a simpler approach would be to move the controller logic directly into PendingMixin. I created one for you here: #6440

A few smaller observations in your PR:

  • The changeset feels more like a minor than a patch since we're adding new public exports.
  • pendingActive moving from @state() to manual requestUpdate() feels like a step away from standard Lit patterns.
  • The disabled cast in PendingMixin.update() is a little fragile; constraining the type or guarding with 'disabled' in this would be safer.
  • The warning URL is button-specific and may become misleading if the mixin is adopted elsewhere.

Overall, I like the goal and most of the implementation. My suggestion would be to collapse PendingController into PendingMixin, keep renderPendingSpinner where it is, and simplify the architecture while preserving the same behavior.

@rubencarvalho

Copy link
Copy Markdown
Contributor

Overall, I agree this is the most idiomatic approach, but I also agree with Rajdeep that we could simplify it a bit 😄 RN, I can’t really see a strong reason to introduce a controller here, especially since we know this exact behaviour is only needed by two components. LMK what you think!

@5t3ph 5t3ph added Status:Addressing feedback PR owner is addressing review comments and will change label back to "Ready for review" when ready. and removed Status:Ready for review PR ready for review or re-review. labels Jun 24, 2026
…-2296)

Have PendingController own and expose the spinner render through a new
renderPendingState() method (delegating to the renderPendingSpinner
directive), and re-expose it from PendingMixin. swc-button,
swc-action-button, and the controller demo host now call
this.renderPendingState() instead of importing the directive directly —
one less import per consumer. The directive remains a standalone export
for stateless use.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@caseyisonit

Copy link
Copy Markdown
Contributor Author

After several discussion we are going to keep the controller and directive and its now wired up through the mixin and the controller owns the directive

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

Labels

High priority PR review PR is a high priority and should be reviewed ASAP Status:Addressing feedback PR owner is addressing review comments and will change label back to "Ready for review" when ready.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants