Skip to content
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

Merged
merged 8 commits into from
May 21, 2024

Conversation

danilo-leal
Copy link
Contributor

@danilo-leal danilo-leal commented May 7, 2024

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:

  • Remove the Material UI tabs component and use a plain Link instead. The reasoning here is that the whole page is not set up for proper Tabs usage, particularly because we weren't toggling the content with Tab Panels, so the accessible pattern was not implemented correctly, and that resulted in the keyboard navigation getting broken
  • Add styles and logic so that the design looks like a Tab component (e.g., active indicator), even with a Link. The difference as far as keyboard navigation here is that you can circle around the different links via the tab key, as opposed to arrow keys, as you'd do in an actual Tab component
  • Cross-off the last item in [docs-infra] Fix Base UI API tabs layout regresions #41122 regarding aria-selected="false"—that's fixed here

https://deploy-preview-42152--material-ui.netlify.app/base-ui/react-button/

@danilo-leal danilo-leal added bug 🐛 Something doesn't work accessibility a11y scope: docs-infra Specific to the docs-infra product labels May 7, 2024
@danilo-leal danilo-leal requested review from colmtuite, mnajdova and a team May 7, 2024 04:06
@danilo-leal danilo-leal self-assigned this May 7, 2024
@mui-bot
Copy link

mui-bot commented May 7, 2024

Netlify deploy preview

https://deploy-preview-42152--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 4982b27

@colmtuite
Copy link
Contributor

A few issues with this UI:

  • It looks like there are no role="tabpanel" elements? As per the spec, you can't have tabs with no tab panels.
  • You can't place focus on the tablist element using the Tab key. You should be able to.
  • If you activate the second tab, but focus the first tab, then press the Tab key, it places focus on the second tab. The tab key should move focus from tablist to tabpanel, you shouldn't be able to focus individual tabs using the Tab key.
  • Space should activate the focused tab, but it doesn't, it scrolls the page instead.
  • As you mentioned, focus is lost when you activate a new tab.

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.

@danilo-leal
Copy link
Contributor Author

danilo-leal commented May 7, 2024

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.

@colmtuite
Copy link
Contributor

@danilo-leal If we convert to simple links, we can even omit the Toolbar component for now since it's not necessarily a requirement. Just mentioning that in case it makes it any easier.

@danilo-leal
Copy link
Contributor Author

@colmtuite check again now? I was unsure what you were referring to with Toolbar (and whether we should've put role="toolbar" here). These last two commits, before I change the PR title/description, go towards the simple links approach, but in such a way that there's little to no visual diff. That required me to do some value mapping here and there for the active indicator's width and position, as well as inserting aria-current, which seems appropriate here.

@colmtuite
Copy link
Contributor

I was unsure what you were referring to with Toolbar

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 <nav> element.

@danilo-leal
Copy link
Contributor Author

danilo-leal commented May 8, 2024

@colmtuite check again now? A brief rundown of the changes: The links wrapper is a nav element; each link element has aria-current so we can warn which one is currently active; links are navigatable via the tab key; visually, it looks like a tab (minimal to no changes from the previous design)

@danilo-leal
Copy link
Contributor Author

@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 :)

Copy link
Member

@alexfauquette alexfauquette left a 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

@danilo-leal danilo-leal merged commit 0af958f into mui:next May 21, 2024
19 checks passed
@danilo-leal danilo-leal deleted the fix-docs-page-tabs-keyboard-nav branch May 21, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y bug 🐛 Something doesn't work scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants