Tabs: Add @radix-ui/react-tabs-powered version of TabPanel
#51551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
alexstine
reviewed
Jun 16, 2023
There was a problem hiding this comment.
@chad1008 I really wish the title attribute was not used. It can lead to duplicate reading of the tabs based on the context they are interacted with. Anyway to remove it? Not a deal breaker.
There is a tabindex="0" added to the role="tablist" wrapper. This should be removed for sure as it allows you to focus the empty wrapper when focus needs to be placed on the first tab. This is a bug in my opinion.
Also, the same goes for tabindex="0" on the role="tabpanel" for an active tab. This component feels over engineered to me.
Can you also add a Storybook example for manual activation? E.g. You must press enter to select the tab? This is currently supported with the TabPanel here.
Other then those notes, this is really looking pretty good.
Thanks.
|
When using VoiceOver, users can't access the contents of the first tab panels by moving the virtual cursor. This happens because the tabs are automatically selected whenever they receive focus, including the focus triggered by the VO virtual cursor. Here's an issue with more details: radix-ui/primitives#1047 The current TabPanel component doesn't have that issue. Ideally, the trigger event for activating automatic tabs should be arrow keys, not focus. That's what the current TabPanel component does with the NavigableMenu component underneath. A few solutions I can think of:
|
Done! We were setting it explicitly (though I don't recall why) so that was easy to remove. I don't think adding the
Thank you for spotting and pointing all of this out! The first part of this was a little more interesting, as there are some notable differences in Current TabPanel: Radix default: Solution:
Good idea. Done! |
|
@diegohaz found a bug that will likely block this PR. I had no idea the Voiceover virtual cursor would trigger focus events. Oh, Voiceover is so lacking compared to Windows at times. Manual activation for all tab panels is likely an okay solution until the bug is fixed upstream if others think it is okay UX. @chad1008 Thanks for your work on the accessibility fixes. Its testing well for Windows. Let's figure out how to move forward given the above new bug. |
|
@ciampo and I looked at this again today and we (mostly Marco, I just added some type data 😆 ) came up with an internal solution that to prevent assistive-tech focus from selecting a tab to address the bug @diegohaz flagged. Of course, we'd prefer to help address upstream but we also wanted to look at a possible solution for the shorter term. |
|
Size Change: 0 B Total Size: 1.42 MB ℹ️ View Unchanged
|
|
Just sharing some thoughts We may have been able to address the above mentioned issues in the "monolithic" version of the component. Although we should also keep in mind that we plan on exposing the granular version of the components (ie. the tablist, tab, and tabpanel) individually. If we wanted to reimplement the same fixes, we may have to add similar logic to a root parent component and share variables via react context, which is not ideal. |
|
Closing in favor of the Ariakit version we shipped recently. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related: #49874
Part of: #51553
This PR contains a lot of work started by @flootr (see above) with additional contributions from @ciampo and myself. While they both did much of the heavy lifting, I'm now reorganizing the effort into separate PRs.
What?
Introduce a Radix-powered version of
TabPanel, namedTabs, maintaining the same functionality and API surface as the existing component. Eventually this will be used to ensure compatibility for existingTabPanelimplementations as we move fully to a completely new component.Why?
As we experiment with third-party libraries for certain UI patterns,
TabPanelis a good candidate to explore, as there are time where additional functionality could be achieved with a more composable version of the component, which will come in future PRsHow?
Tabsis a wrapper around a composedTabPanelutilizing the variousRadixTabssubcomponents.Tabsis written to accept all the same props asTabPanel, passing them to the right props for the various Radix subcomponents internally.Some key implementation bits:
Props vs children
The current
TabPanelcomponent accepts atabsprop containing an object of tab details, and achildrenprop containing a function responsible for rendering the currently selected tab's content.Radix on the other hand accepts tabs as actual children, and not a prop, so we're mapping over our
tabsarray to render the necessaryRadixTabs.Triggercomponents. We do something similar for the tab contents. Here, while ourTabPanelpasses the selected tab's info to the providedchildrenfunction, Radix expects an individualRadixTabs.Contentcomponent for the contents of each tab.Disabled tab focus
Our current implementation allows disabled tabs to be focused by arrow keys to make their presence known to assistive tech. This isn't something Radix supports out of the box, so we've simulated behavior triggered internally in the
Buttoncomponent by__experimentalIsFocusableasChild
By default, Radix renders the tabs themselves as a
buttonelement. Using theasChildprop allows us to instead render them using our ownButtoncomponent.Note: The new component does not yet have a README, as this isn't form that
Tabswill take long-term. I'll make sure one is added in a future PR when the new component is nearer to completion.Testing Instructions
Tabscomponent.TabPanelcounterpart.