Conversation
Netlify deploy previewBundle size report
|
There was a problem hiding this comment.
Pull request overview
This pull request improves the accessibility of the Stepper component by implementing semantic HTML markup and keyboard navigation following ARIA best practices.
Changes:
- Replaced div elements with semantic ol/li elements for Stepper and Step components
- Implemented roving tabindex focus management with arrow key navigation for step buttons
- Added ARIA attributes (aria-posinset, aria-setsize, aria-orientation) to improve screen reader support
- Refactored StepperContext to export only useStepperContext hook and StepperContextProvider, removing the default export
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mui-material/src/Stepper/utils/useRovingTabIndexFocus.ts | New hook implementing circular keyboard navigation with arrow keys, skipping disabled steps |
| packages/mui-material/src/Stepper/utils/useRovingTabIndexFocus.test.tsx | Comprehensive unit tests for the roving tabindex hook |
| packages/mui-material/src/Stepper/index.js | Removed default export of StepperContext, keeping named exports |
| packages/mui-material/src/Stepper/index.d.ts | Updated TypeScript exports to match JavaScript changes |
| packages/mui-material/src/Stepper/StepperContext.ts | Added new context properties for focus management, marked as @internal, exported StepperContextProvider |
| packages/mui-material/src/Stepper/Stepper.test.tsx | Updated test to expect HTMLOListElement instead of HTMLDivElement |
| packages/mui-material/src/Stepper/Stepper.js | Changed root element from div to ol, integrated roving tabindex, added aria-orientation |
| packages/mui-material/src/StepLabel/StepLabel.js | Updated to use useStepperContext hook instead of direct context import |
| packages/mui-material/src/StepContent/StepContent.js | Updated to use useStepperContext hook instead of direct context import |
| packages/mui-material/src/StepConnector/StepConnector.js | Updated to use useStepperContext hook instead of direct context import |
| packages/mui-material/src/StepButton/StepButton.test.js | Added StepperContextProvider wrapper to all test cases |
| packages/mui-material/src/StepButton/StepButton.js | Integrated roving tabindex, added ARIA attributes, implemented keyboard and click handlers |
| packages/mui-material/src/StepButton/StepButton.d.ts | Added TypeScript definitions for onClick and onKeyDown props |
| packages/mui-material/src/Step/Step.test.js | Updated tests for li element and added StepperContextProvider wrappers |
| packages/mui-material/src/Step/Step.js | Changed root element from div to li, updated to use useStepperContext hook |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/mui-material/src/Stepper/utils/useRovingTabIndexFocus.test.tsx
Outdated
Show resolved
Hide resolved
packages/mui-material/src/Stepper/utils/useRovingTabIndexFocus.ts
Outdated
Show resolved
Hide resolved
2efcaec to
a84bfad
Compare
There was a problem hiding this comment.
@silviuaavram Tested it initially with NVDA screen reader. Nice improvements.
- With NVDA enabled, the Left/Right arrow keys don't work on the non-linear stepper:
https://deploy-preview-47687--material-ui.netlify.app/material-ui/react-stepper/#non-linear and nothing is announced as well. When NVDA is off, arrow-key navigation works as expected. - Is it expected that
Tabkey should move focus to the next step? Since this isn't a tablist, I'm not sure. On the docs site (https://mui.com/material-ui/react-stepper/#non-linear),Tabmoves between steps, but in this PR it jumps directly to the "Next" button. - If
useStepperContextis intended to be a public API, I think we should document how to use it with a custom stepper demo.
|
Hey @ZeeshanTamboli thanks for your input!
|
Yes, it works. I am able to navigate with left/right arrow keys with NVDA enabled in this demo but not in this PR's demo.
Ok, so you mean, pressing Tab should focus only on the last focused step in the Stepper and not move to the next step like in production?
I read this |
packages/mui-material/src/Stepper/utils/useRovingTabIndexFocus.ts
Outdated
Show resolved
Hide resolved
d598657 to
be10951
Compare
1f0ac0d to
353af74
Compare
Fixes #43689.
Fixes #32826.
Technical:
ToDo: update docs / usages to include the
aria-controlsattribute, because the step buttons control the showing of content areas.