TabPanel: add tabName prop (controlled component)

[madhusudhand]

What?

Add new prop selectedTabName to control the component tab selection programatically.

Why?

Currently it is not possible to activate a tab using component props. This allows to activate a tab programatically.
The first usecase for this is to auto select the tab in style book based on the selected block in global styles.

[mirka]

Nice enhancement! I just wanted to mention as a heads-up: The disabled tab feature will be landing soon (#46471), so controlled mode also needs to take that into account.

[madhusudhand]

The disabled tab feature will be landing soon (#46471), so controlled mode also needs to take that into account.

Thanks for the mention. Now that the PR is merged, updated the code to consider disabled tab scenario.

[ciampo]

Just linking to this comment that I had made a while ago about TabPanel and its controlled mode. A few notes in particular:

• prop name: in my comment I proposed tabName, I thought it made more sense (usually inputs have a value prop, not a selectedValue prop)
• controlled/uncontrolled: have we considered using the useControlledValue hook?
• have we checked that these API changes are still compatible with our plans of migrating this component to use ariakit under the hood ? (I personally don't think there should be any problems, but it would be good to double-check)
• we need to make sure that current uncontrolled usages of the component still work as expected (we should be adding specific unit tests)

[github-actions]

Flaky tests detected in 4bfaeb9.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6271247960
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

[ciampo]

This PR needs a rebase after the fix in #47100 was merged. I can take another look at the code once the rebase has happened :)

[ciampo]

Hey @madhusudhand , are you still able to work on this PR ?

[madhusudhand]

Hey @ciampo

Updated the component and following are the test scenarios.

[ciampo]

Thank you @madhusudhand for your work here!

It actually looks like there's quite a few scenarios that we'd need to test, especially around:

• the initial tab selection
• the currently selected tab becoming disabled
• the component in general working as expected in controlled and uncontrolled mode alike

Since you've added also a bunch of unit tests (which is great!), I suggest we do the following:

1. We open a new PR, where we add the new unit tests to the current implementation of TabPanel. Of course, we'll be able only to test the component in uncontrolled mode, but adding those tests separately can help us to certify the behaviour of the component on trunk
2. Once the PR with the unit tests is merged, we can rebase this PR and just make the necessary changes to include the new tabName prop and enable controlled mode. We will just need to run the existing tests on the controlled version of the component, and make adjustments if necessary.

I believe that the suggested approach will enable us to move faster and will give us more confidence when merging such changes.

What do folks think?

[ciampo]

We've been recently working across the codebase to refactor unit tests away from using take timers and act() call.

That would mean removing the jest.useFakeTimers(); call and the advanceTimers: jest.advanceTimersByTime, option when setting userEvent up.

cc @tyxla

[tyxla]

Yes, we use fake timers only when we really have to. That means removing jest.useFakeTimers() and calling const user = userEvent.setup(); without the advanceTimers setting.

Here are some PRs where we refactored from fake to real timers if that will help:

• Components: Refactor more tests to use real timers #47318
• core-data: Refactor tests to use real timers #47218
• Tests: Use real timers where fake timers are unnecessary #47144
• Components: Use real timers where possible #47056
• Jest config: use real timers by default #46714

[ciampo]

A pattern that we've started to adopt when writing unit tests for components that can be used both in controlled and uncontrolled mode, is to use describe.each and run the same suite against the controlled and uncontrolled version.

You can see it in action for the ComboboxControl component:

gutenberg/packages/components/src/combobox-control/test/index.js

Lines 80 to 83 in 56c33cf

	describe.each( [
	[ 'uncontrolled', ComboboxControl ],
	[ 'controlled', ControlledComboboxControl ],
	] )( 'ComboboxControl %s', ( ...modeAndComponent ) => {

Of course, if there are certain tests that are specifically aimed at one version of the component, they can be added outside of the describe.each call.

[ciampo]

Hey @madhusudhand , do you still have the capacity to work on this?

[madhusudhand]

@ciampo I would be able to continue the work after 16th Feb. If you have capacity feel free to take it from me. Thanks.

[aaronrobertshaw]

We open a new PR, where we add the new unit tests to the current implementation of TabPanel. Of course, we'll be able only to test the component in uncontrolled mode, but adding those tests separately can help us to certify the behaviour of the component on trunk

I put together a draft PR (#48086) that you might use as a base for verifying the current component behaviour on trunk. I'll be AFK for the next week so feel free to pick it up or close it if it doesn't help.

[madhusudhand]

Update: Rebased the PR with unit testes from #48086 and they all are running great. Will push more tests for controlled mode.

[ciampo]

Update: Rebased the PR with unit testes from #48086 and they all are running great. Will push more tests for controlled mode.

Nice! You can take inspiration from ComboboxControl's tests on how to add the controlled version of the component to the tests:

gutenberg/packages/components/src/combobox-control/test/index.tsx

Lines 61 to 85 in dc32380

	const ControlledComboboxControl = ( {
	value: valueProp,
	onChange,
	...props
	}: ComboboxControlProps ) => {
	const [ value, setValue ] = useState( valueProp );
	const handleOnChange: ComboboxControlProps[ 'onChange' ] = ( newValue ) => {
	setValue( newValue );
	onChange?.( newValue );
	};
	return (
	<>
	<ComboboxControl
	{ ...props }
	value={ value }
	onChange={ handleOnChange }
	/>
	</>
	);
	};
	describe.each( [
	[ 'uncontrolled', ComboboxControl ],
	[ 'controlled', ControlledComboboxControl ],
	] )( 'ComboboxControl %s', ( ...modeAndComponent ) => {

[madhusudhand]

Updated the PR with revised changes and included unit tests for controlled and uncontrolled behaviours.

Note: removed useControlledValue hook from this revision as it is becoming redundant with Ariakit.useTabStore

[ciampo]

Hey @madhusudhand , thank you for continuing the work here!

In the past months, we've started working on improving the component:

• We've refactored TabPanel to Ariakit
• We started work on a new Tabs component — the name better follows the WAI-ARIA pattern, and having a new component allows us to introduce better APIs (including controlled mode and granular components for better composition)

The plan is to leave TabPanel around for backwards compatibility reasons, but to stop maintaining it (if not for bug fixes).

Furthermore, these components have quite an opinionated behaviour when uncontrolled around making sure that a tab gets selected even when the currently selected tab becomes disabled / gets removed. When allowing the component to be controlled, we have to ask ourselves how much of that custom behaviour we want to inherit (you can follow the conversation in #53960 ).

[madhusudhand]

Closing this change. Use the new Tabs component instead.