Shopify / polaris

Shopify’s design system to help us work together to build a great experience for all of our merchants.
https://polaris.shopify.com
Other
5.8k stars 1.17k forks source link

Selecting a tab changes the Panel id causing missing aria-controls on Tab component #1577

Closed alex-page closed 5 years ago

alex-page commented 5 years ago

Issue summary

In our tabs there is an unordered list of individual Tab components. These have an aria-controls that points to a non existent tabpanel.

We currently have one tabpanel that the content gets rendered into dynamically. We change the ID after the click event happens, this causes problems as you can navigate to the buttons that aria-control tabpanels that do not exist. Other examples of accessible tabs have the child containers pre rendered and hidden:

How we can solve this problem:

Expected behavior

The child container with corresponding id exists and is hidden until the user selects the button or link that controls that area.

Actual behavior

There is one child container that gets its id changed on selection of the individual tab.

ry5n commented 5 years ago

I’m not an expert on the aria tabs mechanic or screen reader users expectations. That said, is there a good reason to use the aria tabs mechanism in a React app? Or could we just have the tabs be regular links and sidestep the issue?

alex-page commented 5 years ago

I am thinking the same thing. We can keep calling them tabs but semantically they are reloading content in a window, like how an application does. So we can probably remove the incorrect role.

We should also change how the focus is handled. Removing the aria role means these should be treated like a navigation. So the focus should move on tab key press.

dpersing commented 5 years ago

@ry5n @alex-page

Tabs do get used for navigation sometimes, which confuses this issue, I think (https://github.com/Shopify/polaris-ux/issues/198). Changing the tab behavior to be a standard on-page link that manages focus would also get complicated for pages like indexes. Focus would need to get managed in different ways for different "tab" usage patterns.

alex-page commented 5 years ago

Reopening this issue as the list item has not been removed: https://github.com/Shopify/polaris-react/blob/72f00417c710a816bb703fb553c1fbad1e2e8709/a11y_shitlist.json#L134-L154