feat(tabs): refactored for focusgroup navigation controller - #6494
Conversation
🦋 Changeset detectedLatest commit: 7f4eea4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
📚 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 |
Coverage Report for CI Build 29361234753Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 96.257%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
nikkimk
added
a11y
| * - **`programmatic`** — {@link FocusgroupNavigationController.setActiveItem} or | ||
| * {@link FocusgroupNavigationController.focusFirstItemByTextPrefix} was called directly. | ||
| */ | ||
| export type FocusgroupActiveChangeReason = |
There was a problem hiding this comment.
nit: I like reason, but on the Popover I called it source... for kind of the same thing (when the popover is closed, source values can be programatic, esc, or click-outside). Do you think I should change the one on Popover?
miwha-adobe
left a comment
miwha-adobe
left a comment
There was a problem hiding this comment.
Nice! I like breaking it up.
A few things: it looks like while tabs.selected is fixed, there's still a mismatch on the clicked tab's own .selected inside the handler. This test should surface it:
// ──────────────────────────────────────────────────────────────
// 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');
const tab2 = canvasElement.querySelector('swc-tab[tab-id="2"]') as Tab;
await step(
'clicked tab reflects the new selection inside the change handler',
async () => {
let containerInHandler: string | undefined;
let clickedTabInHandler: boolean | undefined;
tabs.addEventListener(
'change',
() => {
containerInHandler = tabs.selected;
clickedTabInHandler = tab2.selected;
},
{ once: true }
);
tab2.click();
await tabs.updateComplete;
expect(containerInHandler).toBe('2'); // passes today
expect(clickedTabInHandler).toBe(true); // fails today: reads false
}
);
},
};Also added a question regarding the disabled selection!
| if (tab.disabled) { | ||
| return; | ||
| } | ||
| this.selectTarget(tab); |
There was a problem hiding this comment.
Is this intended? Disabled can receive focus, but arrow-keying onto a disabled tab jumps the selection to the first tab (items.find(...) Recording:
disabled.mov
nikkimk
added
the
2nd gen
nikkimk
added
the
Status:Ready for re-review
rubencarvalho
left a comment
rubencarvalho
left a comment
There was a problem hiding this comment.
Looks great! I just left 1 comment, non blocking!
| // When disabled, getItems() returns [] so the controller's cleanup | ||
| // loop in refresh() iterates nothing. Explicitly remove all tabs from | ||
| // the tab order so the tablist is fully inert. | ||
| for (const tab of this._tabs) { |
There was a problem hiding this comment.
I'm not sure if this should or not be responsibility of the controller, but it's probably fine for now!
4 participants
Description
Refactor of 2nd-gen swc-tabs to leverage live focusgroup navigation controller.
Motivation and context
Centralizing roving tabindex/arrow key navigation into a single controller for the components that use it.
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesAccessibility testing checklist
Required: Complete each applicable item and document your testing steps (replace the placeholders with your component-specific instructions).