`TabPanel`: support manual tab activation by ciampo · Pull Request #46004 · WordPress/gutenberg

ciampo · 2022-11-23T12:51:28Z

Closes #45390

What?

Adds support for manual tab activation to the TabPanel component (see #45390 and W3C docs for more info)

Why?

New feature for the TabPanel component, for enhanced accessibility when necessary.

How?

Introduced a new prop hasManualTabActivation (name TBD)
The component keeps behaving as before when the new prop is not specified (backwards-compat)
Added unit tests for both automatic and manual tab activation (including a necessary mock to test keyboard interactions)

Testing Instructions

Load the default TabPanel example in Storybook
Click on the first tab, then use arrow key to switch tab. Note how the currently shown tabpanel changes as the arrow keys are pressed
Via Storybook controls, change the value of the hasManualTabActivation prop to true
Click on the first tab, then use arrow keys to navigate through the list of tabs. Notice how focusing a tab doesn't cause the currently shown tab panel to change.
Make sure that the tabpanel changes only when the space or enter keys are pressed while focusing a tab.

Screenshots or screencast

Kapture.2022-11-23.at.13.50.08.mp4

codesandbox · 2022-11-23T12:51:30Z

Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

ciampo · 2022-11-23T12:52:19Z

@jasmussen and @jameskoster , this may require design feedback as I don't think that TabPanel styles ever accounted for a tab to be focused while not being active.

ntsekouras

Thanks for the PR Marco! We just need to take a look at the non-active but focused styles, as you already mention. Other than that this looks good.

jameskoster · 2022-11-23T13:27:16Z

It's working for me:

tabpanel.mp4

I don't have a great idea for the name either, but perhaps something that relates more to the states involved; openPanelOnFocus?

ciampo · 2022-11-23T13:52:26Z

I don't have a great idea for the name either, but perhaps something that relates more to the states involved; openPanelOnFocus?

On a similar note, ariakit calls this feature selectOnMove — do we think it would be a better prop name?

jameskoster · 2022-11-23T14:16:01Z

@jasmussen and @jameskoster , this may require design feedback as I don't think that TabPanel styles ever accounted for a tab to be focused while not being active.

We can probably consider this when we get to #41904.

chad1008 · 2022-11-23T14:25:58Z

One thing that's occurring to me is that ariakit and this change both default to displaying the tab on focus, which is good, but we come at it from opposite directions. To enable manual selection:

ariakit wants selectOnMove to be false
This PR will want hasManualTabActivation (or whatever we name it) to be true

So we'd eventually end up managing the ariakit prop with something like selectOnMove = { ! hasManualTabActivation }

That's doable, of course, but maybe it'd be more readable in the long run if we went (as @ciampo suggests) with the same selectOnMove prop name, and flipped our boolean accordingly. That way our component and ariakit's implementation are completely in sync.

alexstine

@ciampo Thanks for getting this done quickly. Hopefully, once this is merged, other PRs can progress and further enhancements can be made to code already in trunk.

Giving the PR an approval while the last few bits of feedback are addressed. I agree using the same prop name as we'll have in Ariakit could be useful.

tyxla

This is looking great, there's not much to add to what has been suggested in prior review already. 🚀

tyxla · 2022-11-24T11:06:12Z


 const getSelectedTab = () => screen.getByRole( 'tab', { selected: true } );

+const originalGetClientRects = window.HTMLElement.prototype.getClientRects;


Nit: should this be declared right before we override it in beforeAll? This would then be a let and we'd just set it in beforeAll().

Done in 3128c26

…igableMenu` works as expected

ciampo · 2022-11-25T22:20:49Z

I renamed the prop to selectOnMove as agreed, and addressed all remaining feedback.

I guess the last thing to decide is whether we're ok to merge this PR with the current way tabs look in manual activation mode, when they are focused but not active (see video in the description).

tyxla

Looks great 🚀

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

ciampo · 2022-11-28T21:26:41Z

whether we're ok to merge this PR with the current way tabs look in manual activation mode

Given the absence of strong feedback against merging, I'll go ahead and merge this PR once CI is ✅ . As @jameskoster mentioned above:

We can probably consider this when we get to #41904.

ciampo added [Package] Components /packages/components [Type] Feature New feature to highlight in changelogs. labels Nov 23, 2022

ciampo requested review from alexstine, brookewp, chad1008, jameskoster, jasmussen, mirka and ntsekouras November 23, 2022 12:51

ciampo self-assigned this Nov 23, 2022

ciampo requested a review from ajitbohra as a code owner November 23, 2022 12:51

ciampo force-pushed the feat/tab-panel-manual-tab-activation branch from ab29098 to 7c5b61a Compare November 23, 2022 12:55

ciampo requested review from aaronrobertshaw, talldan and tyxla November 23, 2022 12:58

ciampo commented Nov 23, 2022

View reviewed changes

Comment thread packages/components/src/tab-panel/README.md Outdated

ntsekouras reviewed Nov 23, 2022

View reviewed changes

Comment thread packages/components/src/tab-panel/test/index.tsx

Comment thread packages/components/src/tab-panel/README.md Outdated

alexstine approved these changes Nov 24, 2022

View reviewed changes

alexstine added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 24, 2022

tyxla approved these changes Nov 24, 2022

View reviewed changes

ciampo added 4 commits November 25, 2022 22:28

TabPanel: support manual tab activation

b0983f0

Mock getClientRects in TabPanel s unit tests to make sure that `Nav…

313627f

…igableMenu` works as expected

Add tab activation unit tests

913114d

Update docs

3f0ecfd

ciampo added 3 commits November 25, 2022 22:28

CHANGELOG

0a28dbe

Rename prop to selectOnMove

ebcb84d

Typo

8ef6ea4

ciampo force-pushed the feat/tab-panel-manual-tab-activation branch from 7c5b61a to 8ef6ea4 Compare November 25, 2022 21:39

Move originalGetClientRects declaration inside beforeAll

3128c26

tyxla approved these changes Nov 28, 2022

View reviewed changes

Comment thread packages/components/src/tab-panel/README.md Outdated

Comment thread packages/components/src/tab-panel/types.ts Outdated

ciampo and others added 2 commits November 28, 2022 22:24

Apply suggestions from code review

5c7c88a

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

Merge branch 'trunk' into feat/tab-panel-manual-tab-activation

a649b3f

ciampo enabled auto-merge (squash) November 28, 2022 21:26

ciampo merged commit 03a278e into trunk Nov 28, 2022

ciampo deleted the feat/tab-panel-manual-tab-activation branch November 28, 2022 21:58

github-actions Bot added this to the Gutenberg 14.7 milestone Nov 28, 2022

DaisyOlsen added [Type] Enhancement A suggestion for improvement. and removed [Type] Feature New feature to highlight in changelogs. labels Nov 30, 2022


		const getSelectedTab = () => screen.getByRole( 'tab', { selected: true } );

		const originalGetClientRects = window.HTMLElement.prototype.getClientRects;

Uh oh!

Conversation

ciampo commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Uh oh!

codesandbox Bot commented Nov 23, 2022

Uh oh!

ciampo commented Nov 23, 2022

Uh oh!

Uh oh!

ntsekouras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jameskoster commented Nov 23, 2022

Uh oh!

ciampo commented Nov 23, 2022

Uh oh!

jameskoster commented Nov 23, 2022

Uh oh!

chad1008 commented Nov 23, 2022

Uh oh!

alexstine left a comment

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tyxla Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

ciampo Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ciampo commented Nov 25, 2022

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ciampo commented Nov 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ciampo commented Nov 23, 2022 •

edited

Loading