feat(actiongroup): migrate action group - #6444
Conversation
🦋 Changeset detectedLatest commit: 4782780 The changes in this PR will be included in the next version bump. This PR includes changesets to release 85 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 |
rise-erpelding
added
Status:WIP
Coverage Report for CI Build 32280033838Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 96.248%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
f169a5d to
da08b85
Compare
📚 Branch Preview Links🔍 Gen1 Visual Regression Test ResultsWhen 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: If the changes are expected, update the |
49fb3e7 to
827a2a4
Compare
827a2a4 to
a969bf6
Compare
91f0ed3 to
e109f49
Compare
2022776 to
8abe6f3
Compare
fca3b28 to
a443606
Compare
a443606 to
3bee943
Compare
99d0559 to
23e2fca
Compare
Co-authored-by: Rise Erpelding <for27065@adobe.com> Co-authored-by: Stephanie Eckles <seckles@adobe.com>
f8f749c to
e18e15b
Compare
rise-erpelding
added
Status:Ready for review
| text-decoration: none; | ||
| background-color: var(--swc-action-button-background-color-default, token("gray-100")); | ||
| border: var(--_swc-action-button-border-width) solid var(--swc-action-button-border-color-default, transparent); | ||
| border-radius: var(--swc-action-button-border-radius, token("corner-radius-medium-size-medium")); |
There was a problem hiding this comment.
Adjusting border radii in action button in order to restyle action buttons in compact action group
There was a problem hiding this comment.
[nit] could hold in a private var vs repeating:
---_swc-action-button-border-radius: var(--swc-action-button-border-radius, token("corner-radius-medium-size-medium"));
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
A few final things for polish, and one found bug with the global styles conversion that I've filed separately but you might want to exclude for now.
| * An action group clusters related actions together with consistent | ||
| * spacing, sizing, and orientation. | ||
| * | ||
| * Unlike [button group](../?path=/docs/components-button-group--docs), where |
There was a problem hiding this comment.
Should we add a similar cross-reference to button group?
|
|
||
| <Canvas of={ActionGroupStories.Orientations} /> | ||
|
|
||
| ### Quiet |
There was a problem hiding this comment.
We could probably reasonably remove the compact example since that's mostly useful for VRTs so is covered separately.
| text-decoration: none; | ||
| background-color: var(--swc-action-button-background-color-default, token("gray-100")); | ||
| border: var(--_swc-action-button-border-width) solid var(--swc-action-button-border-color-default, transparent); | ||
| border-radius: var(--swc-action-button-border-radius, token("corner-radius-medium-size-medium")); |
There was a problem hiding this comment.
[nit] could hold in a private var vs repeating:
---_swc-action-button-border-radius: var(--swc-action-button-border-radius, token("corner-radius-medium-size-medium"));| /* Same inflated specificity — matches when aria-disabled is set externally | ||
| (e.g. by a disabled swc-action-group). Kept separate from :disabled so the | ||
| button stays focusable and shows a focus ring for keyboard users. */ | ||
| :host([aria-disabled="true"]) .swc-ActionButton:is(*, :hover) { |
There was a problem hiding this comment.
Was there an issue in combining selectors to merge this with the previous existing disabled styles rule (as in a comma to include both selector clauses)?
| * @cssprop --swc-action-button-min-block-size - Minimum block size. Defaults to the medium component height token. | ||
| * @cssprop --swc-action-button-border-radius - Corner radius. Defaults to `corner-radius-medium-size-medium`. | ||
| * @cssprop --swc-action-button-border-radius - Corner radius applied to all four corners. Defaults to `corner-radius-medium-size-medium`. | ||
| * @cssprop --swc-action-button-border-start-start-radius - Start-start corner radius override. Defaults to `--swc-action-button-border-radius`. Used by `swc-action-group` compact mode to reset interior corners. |
There was a problem hiding this comment.
Do you think we need the implementation detail of "Used by..." since this is also consumer facing docs?
| } | ||
|
|
||
| :host([compact]:not([quiet])[orientation="vertical"]) ::slotted(:not(:first-child, :last-child)) { | ||
| --swc-action-button-border-start-start-radius: 0; |
There was a problem hiding this comment.
[nit] since all are reset, could pass to --swc-action-button-border-radius instead (same for line 111)
| * Propagates visual attributes (`quiet`, `size`, `staticColor`) to slotted | ||
| * children. Called on slot change and when any of these properties update. | ||
| * | ||
| * Note: `compact` affects children via CSS custom property cascade |
There was a problem hiding this comment.
[nit] cleanup comment to remove "Phase 5" mention
|
|
||
| ## Accessibility | ||
|
|
||
| - **Host role** no longer switches between `toolbar`, `radiogroup`, and `group`. It is always `role="group"`. Update any code or tests that asserted a specific host role. If you relied on the toolbar role for a landmark, wrap the group in your own `role="toolbar"` element instead. |
There was a problem hiding this comment.
Maybe add a link to the main docs page toolbar example since it covers aria-orientation too?
| .swc-ActionButton { | ||
| border-color: ButtonBorder; | ||
| } | ||
| .swc-ActionButton--true { |
There was a problem hiding this comment.
Whoops! Looks like a bug with the global styles conversion (also affecting line 86 here). I've filed the details (SWC-2511), but up to you if you want to exclude the aria-disabled styles for now.
| - [x] Status table in workstream doc updated | ||
| - [x] PR created with description referencing Epic SWC-2212 | ||
| - [ ] Peer engineer sign-off | ||
| - [ ] All `TODO` comments added to code during implementation are audited and filed as follow-up Jira tickets under Epic SWC-2212 (see Deferred implementation tickets below) — do this once, at the end, after all `TODO`s for the migration are in |
There was a problem hiding this comment.
I noted a couple of these, maybe double-check?
|
|
||
| public override connectedCallback(): void { | ||
| super.connectedCallback(); | ||
| this.childObserver = new MutationObserver(() => { |
There was a problem hiding this comment.
Low priority / a11y edge: swc-action-group can lose its keyboard tab stop after being detached and re-attached with unchanged children (Vue , drag-drop between containers, virtualized rows).
childObserver only starts observing in handleSlotchange, but slotchange doesn't re-fire on reconnect, so the rebuilt observer never re-arms.
Not a blocker. Suggested Fix: pull the observe loop out of handleSlotchange into a helper and also call it on reconnect (connectedCallback → this.updateComplete.then(() => this.observeChildren())).
There was a problem hiding this comment.
This test can surface the issue
export const ReconnectionObserverTest: Story = {
render: () => html`
<swc-action-group accessible-label="Edit actions">
<swc-action-button>Cut</swc-action-button>
<swc-action-button>Copy</swc-action-button>
<swc-action-button>Paste</swc-action-button>
</swc-action-group>
`,
play: async ({ canvasElement, step }) => {
const group = await getComponent<ActionGroup>(
canvasElement,
'swc-action-group'
);
const buttons = Array.from(
canvasElement.querySelectorAll('swc-action-button')
) as (HTMLElement & {
disabled: boolean;
updateComplete: Promise<boolean>;
})[];
await step('first child is the initial tab stop', async () => {
expect(buttons[0].tabIndex).toBe(0);
});
await step('detach and re-attach the group', async () => {
const parent = group.parentElement as HTMLElement;
parent.removeChild(group);
parent.appendChild(group); // unchanged children → no slotchange fires
await group.updateComplete;
await new Promise((r) => requestAnimationFrame(r));
});
await step(
'disabling the active child after reconnect still moves the tab stop',
async () => {
buttons[0].disabled = true;
await buttons[0].updateComplete;
await new Promise((r) => requestAnimationFrame(r));
expect(buttons[0].tabIndex, 'disabled child is no longer the tab stop').not.toBe(0);
expect(buttons.filter((b) => b.tabIndex === 0).length, 'one tab stop remains').toBe(1);
expect(buttons[1].tabIndex, 'next child becomes the tab stop').toBe(0);
}
);
await step('strip stays keyboard-operable', async () => {
buttons[1].focus();
await userEvent.keyboard('{ArrowRight}');
expect(document.activeElement).toBe(buttons[2]);
});
},
};| changed.has('staticColor') || | ||
| // `compact` is pre-wired here so children re-render when the attribute | ||
| // changes, but the attribute is not propagated via JS — it affects layout | ||
| // via CSS cascade on ::slotted() selectors (wired in Phase 5). |
There was a problem hiding this comment.
We can probably remove any references to Phases, since it doesn't have much value post migration.
| * children. Called on slot change and when any of these properties update. | ||
| * | ||
| * Note: `compact` affects children via CSS custom property cascade | ||
| * (`::slotted(:first-child)` / `::slotted(:last-child)` in Phase 5) rather |
There was a problem hiding this comment.
Same thought: We can probably remove any references to Phases, since it doesn't have much value post migration.
| * they can be forwarded to the inner `<button>`, preventing duplicate ARIA | ||
| * state from appearing on both the host and the native element. The | ||
| * `_ariaForwardingInProgress` guard stops the re-entrant callback triggered | ||
| * by `removeAttribute` from re-entering this branch. |
There was a problem hiding this comment.
This comment is now stale with the addition of aria-disabled and it not stripping as the other two do. Can we update the JSDoc?
4 participants
Description
Migrates the action group component to gen 2.
Motivation and context
Part of the broader 2nd-gen migration (epic SWC-2212).
swc-action-groupgroups relatedswc-action-button/swc-action-menucontrols with roving-tabindex keyboard navigation, replacingsp-action-group. Theselects/selectedexclusive-choice behavior from 1st-gen is intentionally dropped here; that will live in the upcomingswc-segmented-controlinstead.Related issue(s)
Child issues (check off as they're merged)
Related bugs resolved by Gen2 (check off as addressed)
selects/selectedon this component. We can re-examine whether this is addressed when we create swc-segemented control and swc-toggle-button-groupselects/selected. The component that will cover that behavior will be swc-segmented-control.Follow up tickets created related to this work
Screenshots
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Roving tabindex and arrow-key navigation
Action Group→AnatomystoryOrientationvertical example) to move between children, including wrapping from the last child back to the firstGroup vs. child disabled state
Action Group→Disabledstorydisabledon the group (all children getaria-disabledbut stay in the arrow-key sequence) against disabling oneswc-action-buttonchild directly (that child drops out of focus/arrow-key navigation, the rest of the group is unaffected)Toolbar composition and icon-only labeling
Action Group→Toolbar Compositionstory and theAnatomystory's icon-only childtoolbarlandmark wrapping namedgroup-role clusters, and the icon-only button'saccessible-labelexposed as its accessible nameDevice review
Accessibility testing checklist
Required: Complete each applicable item and document your testing steps (replace the placeholders with your component-specific instructions).
Keyboard (required — document steps below) — What to test for: Focus order is logical; Tab reaches the component and all interactive descendants; Enter/Space activate where appropriate; arrow keys work for tabs, menus, sliders, etc.; no focus traps; Escape dismisses when applicable; focus indicator is visible.
Anatomystory, Tab in (focus should land on the first enabled child, or the last-focused child on a repeat visit)Orientation) to move among children, and Home/End to jump to the first/last childScreen reader (required — document steps below) — What to test for: Role and name are announced correctly; state changes (e.g. expanded, selected) are announced; labels and relationships are clear; no unnecessary or duplicate announcements.
AnatomyandToolbar Compositionstorieslabel(when set) as its name, the icon-only child announces itsaccessible-label, and the toolbar wrapper is announced as a toolbar landmark containing named groupsdisabledon the group and confirm each child announces a disabled state while remaining discoverable during navigation (not removed from the accessibility tree)