[togglebuttongroup] Add roving tabindex keyboard navigation#48849
[togglebuttongroup] Add roving tabindex keyboard navigation#48849silviuaavram wants to merge 5 commits into
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
There was a problem hiding this comment.
Pull request overview
This PR introduces roving tabIndex keyboard navigation for ToggleButtonGroup (arrow keys, Home/End, wrapping, RTL + orientation support) by integrating the shared useRovingTabIndex utilities, and updates other roving-tabindex consumers to the revised container-props API.
Changes:
- Extend
useRovingTabIndexRoot().getContainerPropsto compose consumeronFocus/onKeyDownwith internal roving behavior, allowing consumers to prevent internal navigation. - Add roving-tabindex behavior to
ToggleButtonGroupvia a new internalRovingToggleButtonwrapper while keeping standaloneToggleButtonbehavior unchanged. - Add/adjust regression tests and update Toggle Button keyboard documentation.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/mui-utils/src/useRovingTabIndex/useRovingTabIndex.ts | Updates container props API to accept composed focus/keyboard handlers (but currently breaks the previous positional-ref signature). |
| packages/mui-utils/src/useRovingTabIndex/useRovingTabIndex.test.tsx | Adds coverage for composed onFocus/onKeyDown behavior and prevented-event handling. |
| packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroupContext.ts | Adds context flag to enable roving-tabindex behavior for grouped ToggleButtons. |
| packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroup.test.js | Adds keyboard-navigation regression tests for roving tabindex behavior. |
| packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroup.js | Integrates roving-tabindex root behavior and context provider into ToggleButtonGroup. |
| packages/mui-material/src/ToggleButton/ToggleButton.test.js | Adds test to ensure standalone ToggleButton remains tabbable. |
| packages/mui-material/src/ToggleButton/ToggleButton.js | Wraps grouped ToggleButtons with RovingToggleButton to register/forward ref + tabIndex. |
| packages/mui-material/src/ToggleButton/RovingToggleButton.tsx | New internal wrapper that registers ToggleButtons with useRovingTabIndexItem and forwards refs/tabIndex. |
| packages/mui-material/src/Stepper/Stepper.test.tsx | Adds test ensuring Stepper’s roving container composes handlers correctly. |
| packages/mui-material/src/Stepper/Stepper.js | Updates Stepper to pass { ref, onFocus, onKeyDown } into getContainerProps. |
| packages/mui-material/src/MenuList/MenuList.test.js | Adds test ensuring MenuList composes onFocus/onKeyDown with roving behavior. |
| packages/mui-material/src/MenuList/MenuList.js | Updates MenuList to pass consumer onFocus into getContainerProps and preserve composed behavior. |
| docs/data/material/components/toggle-button/toggle-button.md | Updates docs to describe new keyboard navigation behavior for grouped toggle buttons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
siriwatknp
left a comment
There was a problem hiding this comment.
MenuList and Stepper fixes are independent from ToggleButtonGroup right?
siriwatknp
left a comment
There was a problem hiding this comment.
Nice work on the roving tabindex.
Structure and tests follow the existing Tabs/MenuList/Stepper patterns well.
Two things I would want to fix before merge, plus a few nitpicks. Details inline.
| * and keyboard events. | ||
| */ | ||
| getContainerProps: (ref?: React.Ref<HTMLElement>) => { | ||
| getContainerProps: (params?: GetContainerPropsParams) => { |
There was a problem hiding this comment.
getContainerProps changes from a positional ref to an options object here.
But useRovingTabIndexRoot is reachable from the public @mui/utils/useRovingTabIndex subpath (the "./*" export maps to src/*/index.ts, and index.ts re-exports it), and it already shipped in v9.2.0.
So an external caller doing getContainerProps(ref) now passes the ref into params, params.ref is undefined, the ref gets dropped from handleRefs, and container navigation breaks at runtime. TS users also get a source-breaking error.
Internal callers (Stepper, MenuList, Tabs) are all migrated, so this is external only.
Could we keep the old positional form working? Maybe detect a ref arg vs an options object, or keep ref positional and add the handlers as an optional second arg.
|
|
||
| return React.cloneElement(React.Children.only(children), { | ||
| ...rovingItemProps, | ||
| tabIndex: children.props.tabIndex ?? rovingItemProps.tabIndex, |
There was a problem hiding this comment.
On the first render (SSR and pre-hydration) the item map is empty.
useRovingTabIndexRoot has no getDefaultActiveItemId, so it resolves to null and every button gets tabIndex=-1. So the server-rendered markup has no keyboard tab stop until the client registration effects run.
No-JS and SSR keyboard users cannot tab into the group, which goes against the a11y goal here.
Tab.js handles this with shouldUseSelectedTabStopFallback = getItemMap().size === 0 && selected. Could we mirror that? Fall back to tabIndex=0 for the selected button (or the first enabled one) when the map is empty.
| expect(secondButton).to.have.property('disabled', true); | ||
| }); | ||
|
|
||
| describe('keyboard navigation', () => { |
There was a problem hiding this comment.
The isRtl swap (ArrowLeft/ArrowRight) is not covered. The keyboard nav tests are all LTR.
Please add a case under an RTL theme asserting the arrows are swapped, so the isRtl path is covered.
|
|
||
| return ( | ||
| <ToggleButtonGroupRoot | ||
| role="group" |
There was a problem hiding this comment.
Small a11y note.
The group is arrow-navigable now but stays role="group". aria-orientation is not valid on role="group" (only on toolbar/listbox/menu/radiogroup/tablist etc), so adding it here would do nothing.
The real question is whether this should be role="toolbar". That is a semantic change and maybe out of scope, just flagging it for a decision.
There was a problem hiding this comment.
are we adding the aria-orientation anywhere?
There was a problem hiding this comment.
Leaving it as a group is consistent with base ui
|
|
||
| function RovingToggleButton(props: RovingToggleButtonProps, ref: React.ForwardedRef<HTMLElement>) { | ||
| const { children, disabled = false, selected = false } = props; | ||
| const id = useId(); |
There was a problem hiding this comment.
Minor consistency thing.
ToggleButton already has a required, group-unique value, and Tab uses its value as the roving id. Here we generate a useId() instead.
Using value would be more stable, and avoids the transient undefined that useId returns on the first render under the React 17 fallback. Not blocking.
Closes #17281.
Changes
ToggleButtonGroup.Home/Endnavigation, wrapping, and disabled-item skipping.ToggleButtonbehavior unchanged.RovingToggleButtonwrapper to register grouped buttons and forward refs/tabindex.MenuListandStepperfor the revised container-props API.Validation
ToggleButton,ToggleButtonGroup,MenuList,Stepper, and roving-tabindex tests.