feat(core): selection controller#6402
Conversation
🦋 Changeset detectedLatest commit: eb5a302 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
📚 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 |
…pectrum-web-components into nikkimk/poc-selection-controller
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| <DocsFooter /> | ||
| ``` | ||
|
|
||
| #### Controller MDX template |
There was a problem hiding this comment.
Controllers require different documentation, like how to use them with a host, so they should have different rules.
| 10. **BEHAVIORS STORIES** - Built-in functionality (if applicable) | ||
| 11. **ACCESSIBILITY STORIES** - A11y demonstration | ||
|
|
||
| #### Controller section order |
There was a problem hiding this comment.
Controllers require different documentation, like how to use them with a host, so they should have different rules.
| const items = this.getEligibleItems(); | ||
| if (items.length === 0) { | ||
| for (const el of this.getRawItems()) { | ||
| // getRawItems() calls getItems() which may return [] when the host |
There was a problem hiding this comment.
Needed to make adjustments once the controller was implemented with a host that can be disabled.
| // Fall back to event.code when event.key is empty (synthetic test events). | ||
| // Real browser events always populate event.key, so this never affects | ||
| // numpad disambiguation (e.g. Numpad6 NumLock-off sets key='ArrowRight'). | ||
| const key = event.key || event.code; |
There was a problem hiding this comment.
When used with selection controller, there were issues with conflicts in keyboard events.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ies (#6403) * feat(*): rename /shared to /_lit-styles * docs(css, ai): document process and features of shared style directories
* docs(color-handle): accessbility migration-analysis * docs(color-handle): added notes from adobe a11y * docs(color-loupe): added adaptive border guidance
…pectrum-web-components into nikkimk/poc-selection-controller
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pectrum-web-components into nikkimk/poc-selection-controller
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nikkimk/poc-selection-controller
nikkimk
changed the title
nikkimk
added
the
High priority PR review
|
@nikkimk, Nice work here. These are some of my high level observations.
It would be great if you can split this PR into "lvalidate the controller" and "migrate Tabs". |
Coverage Report for CI Build 28966159336Coverage increased (+0.003%) to 96.257%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
| return true; | ||
| } | ||
|
|
||
| if (this.options.confirmSelectionChange) { |
There was a problem hiding this comment.
Question: in a change handler, should tabs.selected be the newly selected tab or the old one?
Right now if you listen for change and read tabs.selected (or the clicked tab's selected), you get the previous tab, not the one just selected.
This is because change is dispatched from confirmSelectionChange, which runs before the selection is committed. 1st-gen updated selected first, so handlers saw the new value.
There was a problem hiding this comment.
Supporting Test (Not in your PR, just what I ran locally off your branch):
// ──────────────────────────────────────────────────────────────
// TEST: Change event — selected is the new tab inside the handler
// ──────────────────────────────────────────────────────────────
export const ChangeHandlerSelectedTest: Story = {
render: () => html`
<swc-tabs selected="1" accessible-label="Change handler selected test">
<swc-tab tab-id="1">Tab 1</swc-tab>
<swc-tab tab-id="2">Tab 2</swc-tab>
<swc-tab-panel tab-id="1"><p>Panel 1</p></swc-tab-panel>
<swc-tab-panel tab-id="2"><p>Panel 2</p></swc-tab-panel>
</swc-tabs>
`,
play: async ({ canvasElement, step }) => {
const tabs = await getComponent<Tabs>(canvasElement, 'swc-tabs');
await step(
'selected is the new tab inside the change handler',
async () => {
let selectedInHandler: string | undefined;
tabs.addEventListener(
'change',
() => {
selectedInHandler = tabs.selected;
},
{ once: true }
);
(canvasElement.querySelector('swc-tab[tab-id="2"]') as Tab).click();
await tabs.updateComplete;
expect(selectedInHandler).toBe('2'); // fails today: reads '1'
}
);
},
};There was a problem hiding this comment.
Fixed — confirmSelectionChange now runs after mutators and internal state are applied for the candidate transition, and reverts everything if it returns false, matching 1st-gen's apply-then-revert-on-cancel pattern. tabs.selected and the clicked tab's own .selected both read the new value inside a change listener now.
Added ChangeHandlerSelectedTest in tabs.test.ts as given (extended slightly to also assert the clicked tab's own .selected, not just tabs.selected).
| */ | ||
| private focusTabAtIndex(index: number): void { | ||
| if (!this._tabs.length) { | ||
| private readonly _handleNavigationActiveChange = (event: Event): void => { |
There was a problem hiding this comment.
I'm not sure if or how often this would happen, but in _handleNavigationActiveChange it selects whatever the focus controller reports as "active," and refresh() re-parks the roving tab stop on the first tab, which reads as an active change and gets selected as if the user navigated there.
Example: keyboard-activation="automatic", tab 2 selected, disable then re-enable. Selection jumps to tab 1 and change fires. Same thing if a tab was pre-selected on mount. Should selection follow only real focus moves, not refresh()? Manual mode is unaffected.
There was a problem hiding this comment.
Supporting Test (Not in your PR, just what I ran locally off your branch)
// ──────────────────────────────────────────────────────────────
// TEST: Automatic activation — mount keeps the pre-selection
// ──────────────────────────────────────────────────────────────
export const AutomaticActivationMountTest: Story = {
render: () => html`
<swc-tabs
selected="2"
keyboard-activation="automatic"
accessible-label="Automatic activation mount test"
>
<swc-tab tab-id="1">Tab 1</swc-tab>
<swc-tab tab-id="2">Tab 2</swc-tab>
<swc-tab tab-id="3">Tab 3</swc-tab>
<swc-tab-panel tab-id="1"><p>Panel 1</p></swc-tab-panel>
<swc-tab-panel tab-id="2"><p>Panel 2</p></swc-tab-panel>
<swc-tab-panel tab-id="3"><p>Panel 3</p></swc-tab-panel>
</swc-tabs>
`,
play: async ({ canvasElement, step }) => {
const tabs = await getComponent<Tabs>(canvasElement, 'swc-tabs');
await step('mount preserves the pre-selected tab', async () => {
// Expected: the pre-selected tab wins over the default-to-first behavior.
expect(tabs.selected).toBe('2'); // fails today: comes back '1'
const tab2 = canvasElement.querySelector('swc-tab[tab-id="2"]') as Tab;
expect(tab2.selected).toBe(true);
});
},
};
// ──────────────────────────────────────────────────────────────
// TEST: Automatic activation — disable toggle keeps the selection
// ──────────────────────────────────────────────────────────────
export const AutomaticActivationDisableToggleTest: Story = {
render: () => html`
<swc-tabs
selected="2"
keyboard-activation="automatic"
accessible-label="Automatic activation disable test"
>
<swc-tab tab-id="1">Tab 1</swc-tab>
<swc-tab tab-id="2">Tab 2</swc-tab>
<swc-tab tab-id="3">Tab 3</swc-tab>
<swc-tab-panel tab-id="1"><p>Panel 1</p></swc-tab-panel>
<swc-tab-panel tab-id="2"><p>Panel 2</p></swc-tab-panel>
<swc-tab-panel tab-id="3"><p>Panel 3</p></swc-tab-panel>
</swc-tabs>
`,
play: async ({ canvasElement, step }) => {
const tabs = await getComponent<Tabs>(canvasElement, 'swc-tabs');
await step('disabling then re-enabling keeps the selection', async () => {
// Reset in case mount already moved the selection, so this step is
// independent and isolates the disable/enable behavior.
tabs.selected = '2';
await tabs.updateComplete;
let changeCount = 0;
const onChange = (): void => {
changeCount += 1;
};
tabs.addEventListener('change', onChange);
tabs.disabled = true;
await tabs.updateComplete;
tabs.disabled = false;
await tabs.updateComplete;
tabs.removeEventListener('change', onChange);
// Toggling `disabled` is not a user selection.
expect(tabs.selected).toBe('2'); // fails today: comes back '1'
expect(changeCount).toBe(0); // fails today: a spurious change fires
});
},
};There was a problem hiding this comment.
Confirmed this exactly — traced it and _navigation.refresh() (on mount, and on disable/re-enable) was dispatching focusgroupNavigationActiveChange indistinguishably from real keyboard navigation, so automatic mode had no way to tell them apart.
Fixed by adding a reason: 'keyboard' | 'focus' | 'programmatic' | 'refresh' field to FocusgroupNavigationActiveChangeDetail. _handleNavigationActiveChange now only acts on 'keyboard' / 'focus'. Added both AutomaticActivationMountTest and AutomaticActivationDisableToggleTest verbatim, plus controller-level coverage in focusgroup-navigation-controller.test.ts exercising all four reasons directly, including the exact disable/re-enable pattern from your example.
| this._navigation.setActiveItem(selectedTab as HTMLElement); | ||
| } else if (this.selected) { | ||
| // No tab with this id — reset the public property. | ||
| this.selected = ''; |
There was a problem hiding this comment.
Question: should setting selected = '' clear the selection?
Right now setting tabs.selected = '' (or an unknown id) half-clears it: the property reads empty, but the old tab stays highlighted while its panel hides, so the tab looks selected with nothing showing.
1st-gen cleared every tab first, so '' left nothing selected.
There was a problem hiding this comment.
Supporting Test (Not in your PR, just what I ran locally off your branch)
// ──────────────────────────────────────────────────────────────
// TEST: Clearing selection — selected = '' deselects the tab
// ──────────────────────────────────────────────────────────────
export const ClearSelectionTest: Story = {
render: () => html`
<swc-tabs selected="2" accessible-label="Clear selection test">
<swc-tab tab-id="1">Tab 1</swc-tab>
<swc-tab tab-id="2">Tab 2</swc-tab>
<swc-tab-panel tab-id="1"><p>Panel 1</p></swc-tab-panel>
<swc-tab-panel tab-id="2"><p>Panel 2</p></swc-tab-panel>
</swc-tabs>
`,
play: async ({ canvasElement, step }) => {
const tabs = await getComponent<Tabs>(canvasElement, 'swc-tabs');
await step('setting selected to "" deselects the tab', async () => {
const tab2 = canvasElement.querySelector('swc-tab[tab-id="2"]') as Tab;
expect(tab2.selected, 'tab 2 starts selected').toBe(true);
tabs.selected = '';
await tabs.updateComplete;
// Clearing the property should leave no tab looking selected.
expect(tabs.selected).toBe('');
expect(
tab2.selected,
'tab 2 should be deselected when the selection clears'
).toBe(false); // fails today: stays true
});
},
};There was a problem hiding this comment.
Root cause: else if (this.selected) was checking the truthiness of the value that had just been reset — so selected = '' could never enter that branch. Fixed _syncSelectionController to always clear the selection (_selection.clearAll({ silent: true })) whenever no tab matches this.selected, whether it's empty or an unknown id.
That required teaching SelectionController.clearAll() / setSelectedItem(null, ...) to accept { silent: true } as a way to bypass the single-mode "can't be emptied by clicking" restriction, since Tabs' selection controller uses mode: 'single'.
Added ClearSelectionTest verbatim, and strengthened the existing "selecting nonexistent value clears selection" test to also assert the previously-selected tab deselects — that missing assertion is exactly what let this ship originally.
…action, and disabled checking
…pectrum-web-components into nikkimk/poc-selection-controller
Thanks for the thorough review @Rajdeepc — went through all four:
On splitting into two PRs — I didn't do that, and at this point doing it after the fact (rather than before mixing the changes) would add more overhead. |
5 participants
Description
Motivation and context
This controller could handle selection for tabs, accordions, radio groups, swatch groups with selectable swatches, button groups that function as radio groups, menus, and pickers.
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Selection Controller
Tabs
Is the creation and implementation of this controller useful for tabs, accordions, radio groups, swatch groups with selectable swatches, button groups that function as radio groups, menus, and pickers? Or is it easier just to let each custom element handle its own selection management?
Device review