refactor(v2): update tabs to follow WAI-ARIA spec #4193
Conversation
lex111
added
the
pr: polish
lex111
force-pushed
the
lex111/tabs-impr
branch
from
e964e08 to
9cd9e72
Compare
|
Deploy preview for docusaurus-2 failed. Built with commit e964e08 https://app.netlify.com/sites/docusaurus-2/deploys/602125eedc037800078667c1 |
|
[V1] Deploy preview failure Built with commit e964e08 https://app.netlify.com/sites/docusaurus-1/deploys/602125eed5c0a2000786173b |
|
Size Change: +16 B (0%) Total Size: 156 kB ℹ️ View Unchanged
|
|
[V1] Deploy preview success Built with commit 9cd9e72 |
|
Deploy preview for docusaurus-2 ready! Built with commit 9cd9e72 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4193--docusaurus-2.netlify.app/classic/ |
|
As looks fine to me, also see this behavior on ReachUI demo. @Mythra do you want to give us some thoughts? |
What do you have in mind? Reakit makes the tab panel always focusable, it seems unreasonable to me, so I did not adopt this behavior. |
I don't say Reakit does the right thing, just highlighting that some of your refs are doing things differently 😅 Wouldn't we benefit from including comps like ReachUI tabs directly instead of implementing our own? As they are small and isolated that may be useful? |
|
This is definitely a good change, I didn't have any tabs on the site I was testing, but yes this is the way tabs are expected to work. One note I may mention is usually if a contents of the tab list are lazy which you seem to have a prop for, it's recommended a user manually hit "space", or "enter" to activate the tab as to not potentially slow down a user. A user who may be using the arrow keys to scroll, don't want them accidentally getting stuck on a tab list if there's going to be a delay. |
|
Dah, you caught me 😄, but actually this is just a list of examples that follow the pattern noted above. Since we already have a large JS bundle, and we are not using tree-shaking correctly, for now wise to use our own component implementations. Maybe after migrating to webpack 5 and loadable components, we should switch to using third-party UI components, but no sure about this. |
|
thanks for the review let's merge this for now
I don't think it's related, we don't need treeshaking for libs that have multiple packages like But yeah we can postpone that choice for now, just think that it could save us some time in the future |
Motivation
After some research, I realized that the current implementation of the Tabs component is not entirely correct. We should better follow the WAI-ARIA Tabs Pattern, which is described at https://www.w3.org/TR/wai-aria-practices/#tabpanel.
The main difference is that pressing on tabs list with Tab key will not switch tab, but instead move focus to the corresponding tabpanel (if necessary). This is useful if the tab content contains a focusable element like a link.
References (with automatic activation):
In addition, I also decided to refactor the code - when rendering each tab item, we use the same functions instead of creating new ones. It seems to me to improve performance a little.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Now the link in the tab content is available for navigating from the keyboard by pressing the
Tabbutton (previously there was a switch to the next tab).Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)