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

[Tabs] Focus management on a tab list with no active tabs #21854

Closed
2 tasks done
evenodd opened this issue Jul 20, 2020 · 5 comments · Fixed by #22377
Closed
2 tasks done

[Tabs] Focus management on a tab list with no active tabs #21854

evenodd opened this issue Jul 20, 2020 · 5 comments · Fixed by #22377
Labels
accessibility a11y bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@evenodd
Copy link

evenodd commented Jul 20, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When the there are no active tabs in the tab list the tabs are inaccessible via keyboard navigation.

Expected Behavior 🤔

When focus moves into the tab list and there are no active tabs the focus is placed on the first tab.

Steps to Reproduce 🕹

https://codesandbox.io/s/kind-blackwell-nx37d?file=/src/Demo.js

Steps:

  1. Try to focus on the tabs using keyboard navigation

Context 🔦

I was using tabs as the main navigation system across a site. Some pages in the app would not be associated with a tab (such as an account or settings page), hence none of the tabs would be active. This would make navigating to those tabbed pages impossible or cumbersome for users relying on keyboard navigation.

Your Environment 🌎

Tech Version
Material-UI v4.11.0
React 16.13.1
@evenodd evenodd added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 20, 2020
@eps1lon eps1lon added bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 20, 2020
@eps1lon
Copy link
Member

eps1lon commented Jul 20, 2020

That's likely a hard problem or requires work that isn't maintainable long term. The problem is that we don't necessarily know ahead of time whether the Tabs#value matches a Tab#value.

Do you have an example of a real world interface that has a tablist with not active tab? So far I've only ever seen tablists with at least one active tab (or at least a sensible default).

@evenodd
Copy link
Author

evenodd commented Jul 20, 2020

For a world example l was quickly able to find one in calendar.google.com

On the far right they have a tab list of add-ons with no tabs active by default.
image

For maintainability perhaps the behaviour only occurs when the user explicitly sets Tabs#value to false?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 20, 2020

From what I understand, in the current cloneElement API usage, we can implement this logic without much hassle. We can replicate the same foundMatch logic than SelectInput.js and force a tabIndex={-1} on the first tab is no match is found.

https://github.com/mui-org/material-ui/blob/44db8eee444191ec19caf130d5a0e4b0bdf1755a/packages/material-ui/src/Select/SelectInput.js#L235

However, if we want to move away from cloneElement API, it turns into a hard problem #12921, #14943

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 1, 2020

That's likely a hard problem or requires work that isn't maintainable long term.

I agree with Sebastian, we already warn when the value doesn't match any children, adding the child tab index when the value is outside the set of available value would be a low ROI, better ignore it. Developers should solve the warning instead.

For maintainability perhaps the behavior only occurs when the user explicitly sets Tabs#value to false?

@evenodd However, I had forgotten that false is a valid prop to unselect all the tabs. What do you think about this diff?

diff --git a/packages/material-ui/src/Tabs/Tabs.js b/packages/material-ui/src/Tabs/Tabs.js
index 7fbf9987f..5834c5386 100644
--- a/packages/material-ui/src/Tabs/Tabs.js
+++ b/packages/material-ui/src/Tabs/Tabs.js
@@ -419,6 +419,7 @@ const Tabs = React.forwardRef(function Tabs(props, ref) {
       onChange,
       textColor,
       value: childValue,
+      ...(childIndex === 1 && value === false && !child.props.tabIndex ? { tabIndex: 0 } : {}),
     });
   });

@oliviertassinari oliviertassinari added accessibility a11y good first issue Great for first contributions. Enable to learn the contribution process. labels Aug 1, 2020
@alexmotoc
Copy link
Contributor

Hey, would it be okay for me to submit a PR with this change?

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 component: tabs This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants