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

Components: Introduce a more composable V2 of the TabPanel #52997

Open
25 of 36 tasks
chad1008 opened this issue Jul 26, 2023 · 21 comments
Open
25 of 36 tasks

Components: Introduce a more composable V2 of the TabPanel #52997

chad1008 opened this issue Jul 26, 2023 · 21 comments
Assignees
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Dev Note Requires a developer note for a major WordPress release cycle [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@chad1008
Copy link
Contributor

chad1008 commented Jul 26, 2023

What?

The new V2 of TabPanel will be a more modular/composable approach to the component, utilizing Ariakit internals. Some of the initial work here has already begun in #52133. That PR focuses on updated the existing component to use Ariakit, while maintaining the same functionality and API surface.

The new component will most likely be named Tabs, as that's a bit more descriptive of the component's function and it will help avoid confusion with the tabpanel role that gets assigned to specific elements within the component.

Requirements of the new component

Some specific features/capabilities this new component must have. A few of these are current blockers of a faux tab replacement

  • Controlled Mode. Specifically, this will allow consumers to set the selected tab when necessary.
  • The ability to place the tablist and tabpanel subcomponents in arbitrary locations within the DOM.
  • tablist design flexibilty
    • Placing an auxiliary button (or other element?) such as a "Close" button beside the tablist
    • Optional full-width tabs
  • Limit onSelect calls to intentional user selections. Currently, the consumer-provided onSelect callback is triggered when the initial tab is selected during the first render. In V2, we intend to only trigger this callback when the user actively selects a tab, not when the component does so automatically.

Nice-to-haves of the new component

tabpanel tab stop/focus behavior

There are no tab stops on the individual tabpanel elements of the current TabPanel component. (...this is when I start looking forward to renaming this component to Tabs in the future 😉). This is something we're changing in the previously mentioned Ariakit migration to better align with ARIA guidance:

When the tab list contains the focus, moves focus to the next element in the page tab sequence outside the tablist, which is the tabpanel unless the first element containing meaningful content inside the tabpanel is focusable.

When that PR merges, all of the tabpanels will get a tab stop, but there's a second part of this guidance for cases dealing with focusable and meaningful content within the tabpanel.

For obvious reasons, our component won't be in a good position to determine which focusable elements contain "meaningful content". We would, however, like to expose a prop (or props, depending on the final approach to this) allowing consumers to specify whether or not focus should go to the tabpanel itself, or to the first focusable element. In cases where the author of a particular implementation knows that the first element in the tabpanel is both focusable and meaningful, this prop will give them the flexibility necessary to streamline the tab flow for end users.

TODOs/Migrations

There may be migrations necessary, if there are implementations that are simulating a tabs pattern without actually using TabPanel. I've included the two of these that I know of, but if anyone is familiar with or comes across more, please let me know!

Related:

@ciampo
Copy link
Contributor

ciampo commented Oct 6, 2023

tabpanel tab stop/focus behavior

This could be definitely done as a follow-up. We could take inspiration from how Modal handles that via the focusOnMount prop.

@afercia
Copy link
Contributor

afercia commented Oct 9, 2023

When that PR merges, all of the tabpanels will get a tab stop

our component won't be in a good position to determine which focusable elements contain "meaningful content". We would, however, like to expose a prop (or props, depending on the final approach to this) allowing consumers to specify whether or not focus should go to the tabpanel itself, or to the first focusable element

Thanks for planning to work on this. A couple quick things:

1
When the tabpanels will get a tab stop, they will need a focus style. Focus indication must always be clear and visible. When tabbing from a Tab to its owned Tabpanel, the Tabpanel must have a focus style. See example at the ARIA Authoring Practices Guide https://www.w3.org/WAI/ARIA/apg/patterns/tabs/examples/tabs-automatic/

2
I'd agree the focus behavior should be consistent with the one recently implementent for the Modal dialog. That is:

  • By default, focus goes to the Tabpanel.
  • AUthors can use a prop to set focus on a focusable element within the Tabpanel.

Assuming the first focusable element within the Tabpanel is always the best place wheere to set focus to may not be a good assumption. For example, a readonly input field is focusable but in some cases may not be the best place where to set focus to.

@jameskoster
Copy link
Contributor

@chad1008 are there any plans to add animation to the new component, perhaps the active indicator, similar to ToggleGroupControl?

@ciampo
Copy link
Contributor

ciampo commented Dec 21, 2023

We initially focused on core functionality, but I think that adding animations should be possible. It's mostly a matter of balancing priorities

@ciampo
Copy link
Contributor

ciampo commented Jan 3, 2024

A couple extra suggestions that came to mind:

  • We should probably use dot notation for the sub components — ie. Tabs.Root, Tabs.TabList, etc..
  • We should rename initialTabId to defaultTabId for better consistency around prop names

@jeryj
Copy link
Contributor

jeryj commented Mar 29, 2024

Limit onSelect calls to intentional user selections. Currently, the consumer-provided onSelect callback is triggered when the initial tab is selected during the first render. In V2, we intend to only trigger this callback when the user actively selects a tab, not when the component does so automatically.

I'm working on switching the Patterns Inserter to a Tab pattern in #60257. I would like the behavior to be:

  • Click block inserter
  • Switch to patterns tab (Blocks, Patterns, Media)

In this state, match the behavior on trunk:

  • Block pattern categories are shown, focus is still on the Patterns tab above the pattern category list
  • Tab to pattern categories
  • Focus is visible on the first pattern category but not selected. (broken)
  • Press Enter to activate the tab

Focus is visible on the first pattern category but not selected is the part that isn't working. When I tab to the list of pattern categories, no active item is set and there is no focus ring visible. I have to press an arrow key to get focus to become visible. Is there a way to have the behavior I'm describing?

Here's the code for Tab implementation I'm working with.

@mirka
Copy link
Member

mirka commented Apr 3, 2024

@jeryj You want Tabs to mount in a state where there is no tab selected (activated), is that correct?

CleanShot.2024-04-04.at.04.19.57.mp4

If so, this video is a modified version of the Controlled Mode story in Storybook with the following arguments:

ControlledMode.args = {
-	selectedTabId: 'tab3',
+	selectedTabId: null,
+	selectOnMove: false,
};

Basically you need to use it in controlled mode, and give it an initial selectedTabId of null.

@jeryj
Copy link
Contributor

jeryj commented Apr 3, 2024

You want Tabs to mount in a state where there is no tab selected (activated), is that correct?

@mirka Kind of. When focus is moved to the tabs, I want the first tab to be focused but not active. Using the example you gave, focus is placed on the tabs wrapper instead of the first tab. You have to press an arrow key to move focus to the tabs, which feels broken to me.

  • Press Tab key
  • Focus moves to tabs wrapper
  • Press arrow key to move focus to first tab
Screen.Recording.2024-04-03.at.3.49.23.PM.mov

I'd like the behavior to be:

  • Press Tab key
  • Focus moves to first tab

@mirka
Copy link
Member

mirka commented Apr 6, 2024

@jeryj I confirmed that this is possible using the raw Ariakit components, by setting a fallback activeId (StackBlitz demo). However I don't think this will work with our current Tabs component setup, so we'll have to make changes. Do you have bandwidth to look into this? Or if not, let us know the importance/urgency of this change so we can prioritize.

@alexstine
Copy link
Contributor

@jeryj I checked the code here and I think I see the issue.

When focusable prop is set, tabindex="0" is set on the button/tabs wrapping div. In your PR, you are not selecting the All tab by default so all the tabindex values for each button are -1 leaving the div as the only element to focus. If you simply set All as the default, I think this problem should go away.

Thanks.

@jeryj
Copy link
Contributor

jeryj commented Apr 8, 2024

If you simply set All as the default, I think this problem should go away.

@alexstine Setting All as the default does fix the focus issue, but it also activates the All tab. I'm trying to recreate the same behavior as trunk by having the All tab focused but not active/open. Then, you can press Enter on the All tab to select it and make the request for all the patterns.

I confirmed that this is possible using the raw Ariakit components, by setting a fallback activeId (StackBlitz demo). However I don't think this will work with our current Tabs component setup, so we'll have to make changes. Do you have bandwidth to look into this?

@mirka Thanks for looking into this! In the demo there's a mystery tab stop for me. Pressing tab again does move focus to the first item. What you made is basically the behavior I'm looking for though. I will try to look into it this week, but as I'm not familiar with the code, I don't expect to make as quick of progress as the components team.

This behavior is blocking a PR to make the patterns inserter much more keyboard friendly, so it's a high priority IMO. In general, I think this behavior would be a great addition to the Tabs component. If I'm not able to find a fix, would two weeks be a reasonable timeline?

@mirka
Copy link
Member

mirka commented Apr 8, 2024

In the demo there's a mystery tab stop for me. Pressing tab again does move focus to the first item.

You're right! Fixed the demo 👍

This behavior is blocking a PR to make the patterns inserter much more keyboard friendly, so it's a high priority IMO. In general, I think this behavior would be a great addition to the Tabs component. If I'm not able to find a fix, would two weeks be a reasonable timeline?

Got it, let us know when you need a review or a handover.

@ciampo
Copy link
Contributor

ciampo commented Jul 19, 2024

@DaniGuardiola I know you have a bunch of Tabs related stuff in your queue, could you please add them to the tracking issue above? 🙏

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 23, 2024
@afercia
Copy link
Contributor

afercia commented Jul 23, 2024

Thinking at this point:

The ability to place the tablist and tabpanel subcomponents in arbitrary locations within the DOM.

I'm not sure this would be any good for semantics, usability, and accessibility. An ARIA tab pattern expects the tab panels to immediately follow the tabs list. Deviating from that would break the pattern.

The Tabs ARIA pattern is really a monolithic pattern where the rendered HTML is supposed to have:

  • The tab list that contains the tabs
  • The tab list is followed by the tab panels.

Allowing to place the various pices of this pattern in arbitrary places in the DOM would be a no go for me. Also, Id like to remind that we already have an issue about extranoue content placed inside the pattern, see #59013

Cc @joedolson for any thoughts and additional feedack.

@joedolson
Copy link
Contributor

What exactly is the reason for wanting tablist/tabpanel subcomponents to be located in arbitrary locations within the DOM? What is the benefit of that requirement for the project?

I struggle to see a way that benefits us other than because it doesn't require existing problem patterns to be redesigned; but I'd appreciate some reasoning for it.

@mirka
Copy link
Member

mirka commented Jul 23, 2024

arbitrary locations within the DOM

I think this meant to say "within the React tree" (correct me if I'm wrong, @ciampo). If I recall correctly, there were use cases where the tabpanel needed to be portaled into a slot, separate from the tablist. They would still be adjacent in the rendered DOM, maintaining appropriate tab order.

@afercia
Copy link
Contributor

afercia commented Jul 24, 2024

They would still be adjacent in the rendered DOM, maintaining appropriate tab order.

Worth reminding it's not just about the tab order. It would be great to build the V2 in a way that it doesn't allow to render extranous content between tabs and tabpabels, see #59013

@ciampo
Copy link
Contributor

ciampo commented Jul 24, 2024

I think this meant to say "within the React tree" (correct me if I'm wrong, @ciampo). If I recall correctly, there were use cases where the tabpanel needed to be portaled into a slot, separate from the tablist. They would still be adjacent in the rendered DOM, maintaining appropriate tab order.

@mirka explained perfectly — allowing consumers of Tabs to render the tablist, tabs and tabpanels via separate react components allows for more flexibility when building UI across complex React trees. For example, this allowed us to use the correct WAI-ARIA pattern instead of a bunch of faux tabs (example).

Worth reminding it's not just about the tab order. It would be great to build the V2 in a way that it doesn't allow to render extranous content between tabs and tabpabels, see #59013

Unfortunately, I don't think that's something we can do at the component source level — it inherently goes against the nature of compound components, like Tabs.

We are definitely committed to enabling high-quality and accessible UIs — that's why we continue to focus on providing the most flexible, compliant, and well-documented set of components.

But if consumers of such components use them in ways that go against best practices, that's out of our control and responsibilities. Testing from the consumer side is, IMO, the most effective way to ensure compliance with such practices.

@afercia
Copy link
Contributor

afercia commented Jul 25, 2024

Unfortunately, I don't think that's something we can do at the component source level — it inherently goes against the nature of compound components, like Tabs.

That's my point. II the compound component pattern doesn't guarantee the quality we want for the markup, than the pattern should not be used. When tools aren't in line with the goals to achieve, other tools should be used instead.

I personally don't mind about the 'composibility' (is that correct English?) of a component if it is at the cost of inaccessible, low quality markup. That would be an approach that is too unbalanced on the engineering side while, to me, a much higher priorit is making sure to provide the highest quality content possible.

As such, I think we should really discuss priorities before even thinking to introduce a composable V2 of the TabPanel.

@mirka
Copy link
Member

mirka commented Jul 25, 2024

Unfortunately there's no silver bullet here, because lack of composability is exactly what motivates a lot of the ad hoc hacks we've been seeing (e.g. faux tabs, fragile style overrides, hacked checkboxes/radios with incorrect labeling).

This composable v2 of TabPanel has already allowed us to eliminate those faux tabs, fixing some major accessibility issues.

There's no single straightforward way to guarantee accessible app code in a project with this many contributors, but I believe having more composable and well-documented components is going to move the needle in the right direction (it already has).

@afercia
Copy link
Contributor

afercia commented Sep 2, 2024

There's no single straightforward way to guarantee accessible app code in a project with this many contributors

I kindly disagree. If the process allows to merge and release non-accessible code, the process is just broken and needs to be fixed. It doesn't relate to individual coding abilities or skills. In an open source project any contribution is blessed and welcom. However, not everything has to be merged.

Regarding the library of components: seven years ago when the editor project started, we as developers and accessibility specialists agreed and were happy to build a standardized library of reusable components. The promise was to build the components in a way that they were accessible sinc ehte beginning, so that was a win for everyone. After seven years, I have to say that promise was not kept. The components still have fundamental problems to be solved and, more importantly, are open to misuse and still can render non-accessible code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Dev Note Requires a developer note for a major WordPress release cycle [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

9 participants