Skip to content

chore(close-button): add structure for s2 migration#6355

Open
TarunAdobe wants to merge 10 commits into
mainfrom
ttomar/migrate-close-button
Open

chore(close-button): add structure for s2 migration#6355
TarunAdobe wants to merge 10 commits into
mainfrom
ttomar/migrate-close-button

Conversation

@TarunAdobe
Copy link
Copy Markdown
Contributor

@TarunAdobe TarunAdobe commented Jun 1, 2026

Description

Phase 2 (Setup) of the swc-close-button 1st-gen → 2nd-gen washing machine migration. This PR scaffolds the component so it builds, registers, and appears in Storybook before API polish and S2 styling land in follow-up PRs.

Core (2nd-gen/packages/core/components/close-button/)

  • CloseButton.types.tsCLOSE_BUTTON_VALID_SIZES, static-color / variant types
  • CloseButton.base.ts — extends ButtonBase with close-button VALID_SIZES, static-color, and legacy variant
    (maps to static-color when set; full deprecation warnings deferred to Phase 3). Size uses shared SizedMixin
    behavior from ButtonBase (default m, reflected on the host).
  • Package export wired in core/package.json
    SWC (2nd-gen/packages/swc/components/close-button/)
  • CloseButton.ts — inner <button type="button">, size-mapped cross icon, visually hidden default slot, aria-label from resolved accessible name
  • close-button.css — stub layout and visually hidden label styles (not S2 visual parity)
  • swc-close-button.ts — element registration
  • Storybook Playground + Overview stories
  • Placeholder unit and a11y spec tests (expanded in Phase 6)

Docs

  • Migration status table: Close Button Setup marked complete

Motivation and context

Close button migration is tracked under the 2nd-gen component workstream. Setup must land before Phase 3 (API / TypeScript) and Phase 5 (S2 CSS). This PR intentionally ships a minimal, buildable skeleton — not visual parity with Spectrum 2 closebutton CSS.

Related issue(s)

  • Epic SWC-2087 — Close button migration
  • Planning (separate PR): SWC-2089 — migration plan
    (#6322)

Screenshots (if appropriate)

N/A — stub styling only; no meaningful visual regression vs Spectrum 2
yet.

Author's checklist

  • I have read the [CONTRIBUTING](https://github.com/adobe/spectru
    m-web-components/blob/main/CONTRIBUTING.md)
    and
    [PULL_REQUESTS](https://github.com/adobe/spectrum-web-components/blob
    /main/PULL_REQUESTS.md)
    documents.
  • I have reviewed the Accessibility Practices for this feature,
    see: Aria Practices
  • I have added automated tests to cover my changes (placeholder
    Storybook + a11y snapshot; full coverage in Phase 6).
  • I have included a well-written changeset if my change needs to be
    published.
  • I have included updated documentation if my change required it
    (status table only).

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

  • Verify close-button scaffold builds and registers
    1. Run yarn build from repo root (or 2nd-gen workspace build for
      core + swc).
    2. Confirm build completes without errors.
  • Verify element registration and Storybook
    1. Open 2nd-gen Storybook → Close ButtonPlayground (or
      branch preview Storybook if available).
    2. Confirm <swc-close-button accessible-label="Close"> renders with
      a cross icon.
    3. In DevTools, confirm shadow DOM contains a real inner <button type="button"> with aria-label="Close".
  • Verify size and static-color attributes (stub visuals)
    1. In Playground, set size to s, m, l, and xl; confirm the
      host updates and the icon mapping changes
    2. Set static-color to white or black; confirm classes
      swc-CloseButton--staticWhite / swc-CloseButton--staticBlack appear
      on the inner button (styling is stub-only).
  • Verify default size is reflected on the host
  1. Render with no author-set size.
  2. Inspect the host; expect size="m" on the host (property default m via SizedMixin).

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 2nd-gen Storybook → Close ButtonPlayground.
    2. Press Tab until focus reaches the close button; confirm
      a visible focus indicator on the inner button (delegated focus).
    3. Press Enter or Space; confirm the button
      activates (click handler / default button behavior).
    4. Set disabled on the control; confirm the button is not reachable
      via Tab and does not activate.
  • Screen reader (required — document steps below)
    1. Open Playground with VoiceOver (macOS) or NVDA (Windows).
    2. Tab to the button; confirm it is announced as a button named
      Close (from accessible-label or slotted text).
    3. With disabled set, confirm the control is announced as
      unavailable/disabled.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 1, 2026

⚠️ No Changeset found

Latest commit: b0368d9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

📚 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-6355

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.

@TarunAdobe TarunAdobe force-pushed the ttomar/migrate-close-button branch from 7e643ab to aa5ff5d Compare June 2, 2026 07:58
@TarunAdobe TarunAdobe marked this pull request as ready for review June 2, 2026 08:02
@TarunAdobe TarunAdobe requested a review from a team as a code owner June 2, 2026 08:02
@TarunAdobe
Copy link
Copy Markdown
Contributor Author

TarunAdobe commented Jun 2, 2026

I implemented this close-button scaffold migration with the Cursor agent, then self-reviewed the diff once before marking it ready for review.

A few things worth calling out for reviewers:

• Cross400 / Cross500 icons — added so l and xl close-button sizes map to the right cross scale (not just
reusing the medium icon).
• Storybook docs — narrowed the autodocs API table so inherited ButtonBase members like pending and
pending-label don’t show up as close-button API (they aren’t part of this component’s public surface). A
post-inheritance CEM plugin strips @internal overrides after inheritance applies.
• 2nd-gen API — swc-close-button does not ship the 1st-gen variant="white|black" alias; use static-color only. Per 2nd-gen convention we drop legacy APIs here rather than deprecating them; variant deprecation stays a 1st-gen (sp-close-button) follow-up.
• Migration plan — updated to match: close-button extends ButtonBase for shared semantics (naming, disabled, sizing), not for swc-button-style pending behavior;

Please flag anything that looks off in the scaffold or wiring

@TarunAdobe TarunAdobe self-assigned this Jun 2, 2026
@TarunAdobe TarunAdobe added the Status:Ready for review PR ready for review or re-review. label Jun 2, 2026
Comment thread 2nd-gen/packages/swc/components/close-button/CloseButton.ts Outdated
@TarunAdobe TarunAdobe force-pushed the ttomar/migrate-close-button branch 2 times, most recently from 2988c74 to d84aed0 Compare June 2, 2026 12:34
Comment on lines +39 to +48
* @internal
* Not part of the close-button public API.
*/
public override pending = false;

/**
* @internal
* Not part of the close-button public API.
*/
public override pendingLabel?: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this work? CEM will scan the Buttonbase properties first. Kindly verify this. Keeping it blocking for now!

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.

works with the plugin that i added. and here's the ticket for broader button base refractor work: https://jira.corp.adobe.com/browse/SWC-2250

Copy link
Copy Markdown
Contributor

@Rajdeepc Rajdeepc Jun 3, 2026

Choose a reason for hiding this comment

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

You can abstract the CloseButton required properties in an abstract class in button.base.ts which ButtonBase and CloseButtonBase should extend. This would keep the API surface cleaner even if future components extends ButtonBase.


// button.base.ts

export abstract class InteractiveButtonbase extends SizedMixin(
  ObserveSlotText(ObserveSlotPresence(SpectrumElement, '[slot="icon"]'), ''),
  { validSizes: BUTTON_VALID_SIZES }
) {
 static override shadowRootOptions: ShadowRootInit = {
    ...SpectrumElement.shadowRootOptions,
    delegatesFocus: true,
  };
  /**
   * Whether the button is disabled. Removes focusability and prevents
   * interaction.
   */
  @property({ type: Boolean, reflect: true })
  public disabled: boolean = false;
  /**
   * Accessible label forwarded to the internal `<button>` element as
   * `aria-label`. Required for icon-only buttons, which have no visible text.
   */
  @property({ type: String, attribute: 'accessible-label' })
  public accessibleLabel?: string;
  protected get hasIcon(): boolean {
    return this.slotContentIsPresent;
  }
 protected get hasLabel(): boolean {
    return this.slotHasContent;
  }
  /**
   * Resolves the accessible name for the button from `accessibleLabel` or
   * visible text content. Returns `null` when no accessible name is
   * determinable.
   *
   * @internal
   */
  protected getResolvedAccessibleName(): string | null {
    return this.accessibleLabel ?? (this.textContent?.trim() || null);
  }
  /**
   * Returns the set of attributes that should be forwarded to the internal
   * semantic `<button>` element, if not otherwise directly managed.
   *
   * @internal
   */
  protected getForwardedButtonAttributes(): Record<
    string,
    string | boolean | undefined
  > {
    return {
      disabled: this.disabled,
    };
  }
  protected override update(changedProperties: PropertyValues): void {
    super.update(changedProperties);
    if (window.__swc?.DEBUG) {
      if (this.hasIcon && !this.hasLabel && !this.accessibleLabel) {
        window.__swc.warn(
          this,
          `<${this.localName}> with an icon and no label must have an "accessible-label" attribute to be accessible.`,
          'https://opensource.adobe.com/spectrum-web-components/components/button/#icon-only',
          { issues: ['accessible-label'] }
        );
      }
    }
  }
  
 export abstract class ButtonBase extends InteractiveButtonbase {
 
   // pending attributes of button stays here
 
  }
  


// CloseButton.base.ts
  
 export abstract class CloseButtonBase extends InteractiveButtonbase { }
  

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.

Done

@TarunAdobe TarunAdobe force-pushed the ttomar/migrate-close-button branch from 6670f9e to 58ebd82 Compare June 3, 2026 06:48
@TarunAdobe
Copy link
Copy Markdown
Contributor Author

CloseButtonBase initially extended ButtonBase, which owns full button semantics including pending /
pending-label. That leaked into:

• CEM / Storybook controls (pending showed up on swc-close-button)
• Docs MDX inherited from the button family

Close-button is not a pending control (unlike swc-button), so that surface was wrong.

First fix: @internal overrides on CloseButtonBase + a custom CEM internal-override-plugin to strip inherited
members after analyze. That worked but was brittle (plugin had to reason about inheritance order;
badge/status-light variant interactions were a footgun).

Final fix (latest commit): structural split instead of doc/CEM patching:

• SharedButtonBase — sizing, slots, disabled, accessible-label, accessible-name helpers (no pending)
• PendingMixin — existing pending behavior moved here unchanged (pendingActive, 1s delay, click suppression,
aria-disabled while pending, etc.)
• ButtonBase — thin layer: PendingMixin(SharedButtonBaseCore) + aria-disabled forwarding

Close-button no longer needs @internal pending overrides or the CEM plugin → plugin removed.

TarunAdobe and others added 10 commits June 3, 2026 17:03
Scaffold 2nd-gen close-button in core and swc: CloseButtonBase with
static-color and sizing, minimal render and CSS, Storybook stories, and
placeholder tests. Wire core package exports and mark Setup complete in
the migration status table.

Co-authored-by: Cursor <cursoragent@cursor.com>
Wire close-button l/xl to spectrum-css UI icon paths instead of reusing Cross300.

Co-authored-by: Cursor <cursoragent@cursor.com>
Rebase onto main with planning docs merged; tick Setup checklist and status table.

Co-authored-by: Cursor <cursoragent@cursor.com>
…isible control

Add size and icon tokens so Playwright can wait for swc-close-button in overview stories.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop pending wiring from the SWC template, mark inherited pending APIs
@internal on CloseButton, hide them in Storybook, and document the scope
in the migration plan.

Co-authored-by: Cursor <cursoragent@cursor.com>
… MDX

Restore public variant in CEM for components that redeclare it on the
concrete class, while keeping pending hidden when only a descendant marks
it @internal (ancestor ButtonBase must not undo CloseButtonBase).

Co-authored-by: Cursor <cursoragent@cursor.com>
2nd-gen removes legacy APIs instead of shipping deprecated aliases.
Use static-color only on swc-close-button; variant deprecation stays
on 1st-gen sp-close-button per migration plan.

Co-authored-by: Cursor <cursoragent@cursor.com>
Remove stray blank line after class opening brace so CI prettier check passes.

Co-authored-by: Cursor <cursoragent@cursor.com>
…utton

Extract shared button semantics into SharedButtonBase and move pending into
PendingMixin on ButtonBase only. Close-button extends SharedButtonBase so
pending stays off its public API without a CEM override plugin.

Co-authored-by: Cursor <cursoragent@cursor.com>
@TarunAdobe TarunAdobe force-pushed the ttomar/migrate-close-button branch from 12446ad to b0368d9 Compare June 3, 2026 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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