search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

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

Open chad1008 opened 1 year ago

chad1008 commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 11 months ago

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

ciampo commented 11 months ago

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

ciampo commented 10 months ago

A couple extra suggestions that came to mind:

jeryj commented 7 months ago

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 https://github.com/WordPress/gutenberg/pull/60257. I would like the behavior to be:

In this state, match the behavior on trunk:

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 commented 7 months ago

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

https://github.com/WordPress/gutenberg/assets/555336/6dfd8646-6cd4-47dd-8348-1901b48e0997

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 commented 7 months ago

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.

https://github.com/WordPress/gutenberg/assets/967608/e2d07036-4b1e-4231-a419-525545a65eff

I'd like the behavior to be:

mirka commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 4 months ago

@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 commented 3 months ago

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:

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 https://github.com/WordPress/gutenberg/issues/59013

Cc @joedolson for any thoughts and additional feedack.

joedolson commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 https://github.com/WordPress/gutenberg/issues/59013

ciampo commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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.