-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[docs-infra] Fix keyboard navigation on page tabs #42152
[docs-infra] Fix keyboard navigation on page tabs #42152
Conversation
Netlify deploy previewhttps://deploy-preview-42152--material-ui.netlify.app/ Bundle size report |
A few issues with this UI:
So it looks like this UI is trying to be tabs, but it's still a long way off tabs. We need to either convert it into tabs properly (I'd recommend using the newly merged Base UI tabs component) or convert it into simple links, wrapped in a Toolbar component. |
Uhm... Good points! I reckon all of these should also come in with the Material UI Tabs component, which it's what this is using, but given the whole layout is not structured in a way you'd usually do with Tabs (e.g., as you mentioned, having each tab content within a tab panel), we might be better off for now going for you latter suggestion to avoid a more complex change right now — just wanted to improve keyboard navigation slightly but it seems incomplete if we try to fix this problem and not the others circulating the Tabs context. |
@danilo-leal If we convert to simple links, we can even omit the |
@colmtuite check again now? I was unsure what you were referring to with |
I was referring to the Toolbar ARIA pattern. This row of links should be wrapped in a Toolbar component that has focus management with roving tab index, an aria-role, and an aria-label. For now we could just wrap the links in a |
@colmtuite check again now? A brief rundown of the changes: The links wrapper is a nav element; each link element has |
@alexfauquette & @bharatkashyap — hey! Directly tagging you here to get your review in case there's any code-specific concerns with the way I pulled off this PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just proposing an update do don't lose a fix
Old description
After multiple trials and errors + looking around, I realized that something (which I couldn't quite figure out) is preventing the `Link` component from `@mui/docs` from rendering with the `role="tab"` attribute. That's ultimately what's breaking the arrow keys keyboard navigation. My goal with this PR is purely to fix that rather than discuss just yet whether this is a desirable pattern (although I did find [a cool Ariakit example](https://ariakit.org/examples/tab-next-router) using it, which is reassuring!).So, just using the
Link
component straight from Next.js seems to nicely fix the issue — try focusing on the first active tab and pressing the right key to navigate; you should expect normal Tabs behavior now.Edit: The merged version of this PR is different from its initial iteration, so I kept the original description stored up there. In any case, here's a run-down of what was ultimately pushed:
aria-selected="false"
—that's fixed herehttps://deploy-preview-42152--material-ui.netlify.app/base-ui/react-button/