fix: Overflow Tabs (3558) included in collection system#3790
fix: Overflow Tabs (3558) included in collection system#3790williamjstanton wants to merge 2 commits intoWorkday:masterfrom
Conversation
Workday/canvas-kit
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
william-3558-overflow-tabs-fixes
|
| Run status |
|
| Run duration | 02m 36s |
| Commit |
|
| Committer | William Stanton |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
86
|
Skipped
|
0
|
Passing
|
850
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.71%
|
|
|---|---|
|
Untested elements
|
1505
|
|
Tested elements
|
367
|
Accessibility
99.37%
|
|
|---|---|
|
Failed rules
|
6 critical
5 serious
0 moderate
2 minor
|
|
Failed elements
|
76
|
|
This PR can also apply to this feature issue: |
There was a problem hiding this comment.
Pull request overview
This PR addresses accessibility and keyboard navigation issues with the Tabs overflow pattern (issue #3558). The "More" overflow button is transformed from a standard button outside the tablist into a first-class tab within the collection system. It now uses role="tab", participates in roving tabindex, and is navigable via Left/Right arrow keys instead of the Tab key. When no tabs are hidden, the overflow button is not rendered, ensuring accurate tab counts for assistive technologies.
Changes:
- The overflow button is now included in the tabs collection as a synthetic item with role="tab" and roving focus behavior, navigable via arrow keys
- Overflow calculation triggers immediately when the overflow button size is measured, ensuring responsive layout updates
- Tab panels can now receive focus via ArrowDown key from the selected tab (panels set
tabIndex={-1})
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
modules/react/tabs/lib/useTabsModel.tsx |
Adds synthetic overflow button item to collection; manages nonInteractive state and cursor position; implements focus management after menu selection |
modules/react/tabs/lib/TabsOverflowButton.tsx |
Changes overflow button to use role="tab" with roving focus hooks; implements conditional rendering based on hidden items; adds custom measurement hook |
modules/react/tabs/lib/TabsList.tsx |
Implements custom render function to filter synthetic overflow item; adds custom cursor reset logic for tabs; adds minWidth style for flex layout |
modules/react/tabs/lib/TabsItem.tsx |
Adds ArrowDown handler to focus tab panel from selected tab |
modules/react/collection/lib/useOverflowListModel.tsx |
Triggers immediate overflow recalculation when overflow target size changes |
modules/react/tabs/stories/examples/OverflowTabs.tsx |
Updates example to use design system components and tokens; sets panel tabIndex={-1} for ArrowDown focus behavior |
cypress/component/Tabs.spec.tsx |
Updates tests to reflect overflow button as tab with role="tab" instead of button; adjusts keyboard navigation and focus expectations |
cypress/support/component.ts |
Changes Cypress mount import from cypress/react18 to cypress/react |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (panel) { | ||
| (panel as HTMLElement).focus(); | ||
| } | ||
| return null as unknown as void; // prevent roving focus from handling this key |
There was a problem hiding this comment.
The return statement return null as unknown as void is a type system workaround that obscures the intent. This is being used to signal to the roving focus handler to not process this key event. Consider using a more explicit mechanism such as calling event.stopPropagation() or documenting this pattern more clearly, or returning undefined which is the expected return type for event handlers. The type assertion undermines TypeScript's type safety without clear benefit.
| model.events.goTo({id: targetId}); | ||
| } | ||
| } | ||
| }, [model.state, model.events]); |
There was a problem hiding this comment.
The useLayoutEffect depends on model.state and model.events, which are objects that may be recreated on every render. This could cause the effect to run more frequently than intended. Since the effect conditionally calls model.events.goTo() which updates state, there's a risk of infinite render loops if the conditions aren't carefully managed. Consider using more specific dependencies like model.state.cursorId, model.state.hiddenIds.length, model.state.items.length, and model.state.selectedIds instead of the entire model.state object. This would make the dependencies more explicit and reduce the risk of unintended re-runs.
| }, [model.state, model.events]); | |
| }, [ | |
| model.state.cursorId, | |
| model.state.hiddenIds.length, | |
| model.state.items.length, | |
| model.state.selectedIds, | |
| model.events.goTo, | |
| ]); |
| onBlur() { | ||
| if (!programmaticFocusRef.current) { | ||
| requestAnimationFrameRef.current = requestAnimationFrame(() => { | ||
| requestAnimationFrameRef.current = 0; |
There was a problem hiding this comment.
The cursor reset logic assigns requestAnimationFrameRef.current = 0 after the requestAnimationFrame callback runs (line 135), but this value is never checked elsewhere. If the intent is to track whether there's a pending animation frame for cleanup purposes, the value should be stored when the rAF is scheduled and the cleanup in useMountLayout should check if it's non-zero before canceling. However, if the goal is simply to clean up the reference, setting it to 0 is redundant since the cleanup function will cancel any pending frame on unmount. Consider removing this line or documenting why it's needed.
| requestAnimationFrameRef.current = 0; |
| requestAnimationFrame(() => { | ||
| requestAnimationFrame(() => { | ||
| const tabId = pendingFocusTabIdRef.current; | ||
| if (tabId) { | ||
| const tabElement = document.querySelector<HTMLElement>( | ||
| `[data-focus-id="${slugify(`${model.state.id}-${tabId}`)}"]` | ||
| ); | ||
| if (tabElement) { | ||
| tabElement.focus(); | ||
| } | ||
| pendingFocusTabIdRef.current = null; | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The double requestAnimationFrame scheduled in the onSelect handler (lines 230-243) is not canceled if the component unmounts before the callbacks execute. This could lead to attempted DOM queries after the component is unmounted, though it's handled safely by the null check. Consider storing the rAF IDs and canceling them in a cleanup effect or when the model is destroyed to prevent unnecessary work and potential memory leaks in complex applications where many tab instances are created and destroyed.
| // require('./commands'); | ||
|
|
||
| import {mount} from 'cypress/react18'; | ||
| import {mount} from 'cypress/react'; |
There was a problem hiding this comment.
The import path has been changed from cypress/react18 to cypress/react. This change is not mentioned in the PR description or breaking changes section. If this is intentional, it should be documented as it may affect how Cypress tests are run, especially if the project specifically requires React 18 support or if there are differences in how the mount function behaves between these imports.
| // Focus the selected tab AFTER React has re-rendered. | ||
| // We use double requestAnimationFrame to ensure: | ||
| // 1. First rAF: React commits DOM changes from state updates | ||
| // 2. Second rAF: We focus after the popup's useReturnFocus has run | ||
| // This overrides the default behavior of returning focus to the overflow button. | ||
| requestAnimationFrame(() => { | ||
| requestAnimationFrame(() => { | ||
| const tabId = pendingFocusTabIdRef.current; | ||
| if (tabId) { | ||
| const tabElement = document.querySelector<HTMLElement>( | ||
| `[data-focus-id="${slugify(`${model.state.id}-${tabId}`)}"]` | ||
| ); | ||
| if (tabElement) { | ||
| tabElement.focus(); | ||
| } | ||
| pendingFocusTabIdRef.current = null; | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The use of double requestAnimationFrame to focus the selected tab after menu closes is fragile and may not work reliably across different browsers or performance conditions. The comment states it's needed to run after useReturnFocus, but this creates a timing dependency on the internal implementation of the popup component. Consider using a more robust approach such as a callback from the menu model's onHide event, or using useLayoutEffect with proper dependencies to handle focus management. This approach is susceptible to race conditions if React's rendering timing changes or if the popup's focus restoration logic is modified.
Summary
Fixes: #3558
This PR improves the Tabs overflow pattern for accessibility and keyboard navigation. The "More" overflow button is now a first-class member of the tablist: it uses
role="tab", participates in the collection's roving tabindex, and is reached with Left/Right arrow keys (same as other tabs) instead of the Tab key. When there are no hidden items, the overflow button is not rendered so it does not appear in the tab count or focus order.Motivation: After receiving usability feedback about our Tabs overflow pattern, we revisited the implementation:
Keyboard discoverability: Users were expected to use the Tab key to focus the "More" overflow button, even though it is visually styled like the other tab elements. This was inconsistent with how tabs are normally navigated (arrow keys within a tablist).
ARIA semantics: The overflow control was a standard
buttonelement and did not use therole="tab"required for children of arole="tablist"container, which could confuse assistive technologies and automated checks.Release Category
Components
Release Note
Optional release note message. Changelog and release summaries will contain a pull request title. This section will add additional notes under that title. This section is not a summary, but something extra to point out in release notes. An example might be calling out breaking changes in a labs component or minor visual changes that need visual regression updates. Remove this section if no additional release notes are required.
The Tabs "More" overflow button is now part of the tablist for accessibility: it has
role="tab", is included in the roving tabindex, and is reached with Left/Right arrow keys instead of Tab. Automation or tests that relied on the overflow control being abuttonor focused only via Tab may need to be updated.BREAKING CHANGES
Optional breaking changes message. If your PR includes breaking changes. It is extremely rare to put breaking changes outside a
prerelease/majorbranch. Anything in this section will show up in release notes. Remove this section if no breaking changes are present.role="tab"(notrole="button").Automation or accessibility tests that target the overflow control by role (
button) or by Tab-key focus order may need to be updated to use arrow-key navigation androle="tab"(e.g. within the tablist).Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
modules/react/tabs/lib/useTabsModel.tsx— Synthetic overflow item and non-interactive handling.modules/react/tabs/lib/TabsOverflowButton.tsx— Overflow button as a tab (role, roving focus, conditional render).modules/react/tabs/lib/TabsList.tsx— Filtering the synthetic item in the render prop and cursor reset on blur.Areas for Feedback? (optional)
Testing Manually
OverflowTabsor the overflow example in the Tabs docs).role="tab"andaria-selected="false". Confirm the tablist has the expected number of tabs (visible tabs + one for "More" when overflow is present).Screenshots or GIFs (if applicable)
No UI visuals changed; behavior and DOM/ARIA semantics were updated. A short GIF of arrow-key navigation to/from the "More" tab could be added if desired.
Thank You Gif (optional)