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

TabPanel: support manual tab activation #46004

Merged
merged 10 commits into from
Nov 28, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Nov 23, 2022

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

@ciampo ciampo added [Package] Components /packages/components [Type] Feature New feature to highlight in changelogs. labels Nov 23, 2022
@ciampo ciampo self-assigned this Nov 23, 2022
@ciampo ciampo requested a review from ajitbohra as a code owner November 23, 2022 12:51
@codesandbox
Copy link

codesandbox bot commented Nov 23, 2022

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

@ciampo
Copy link
Contributor Author

ciampo commented Nov 23, 2022

@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.

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

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

packages/components/src/tab-panel/README.md Outdated Show resolved Hide resolved
@jameskoster
Copy link
Contributor

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
Copy link
Contributor Author

ciampo commented Nov 23, 2022

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
Copy link
Contributor

@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
Copy link
Contributor

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.

Copy link
Contributor

@alexstine alexstine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/components/src/tab-panel/index.tsx Outdated Show resolved Hide resolved
@@ -34,7 +34,23 @@ const TABS = [

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

const originalGetClientRects = window.HTMLElement.prototype.getClientRects;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 3128c26

packages/components/src/tab-panel/test/index.tsx Outdated Show resolved Hide resolved
@ciampo ciampo force-pushed the feat/tab-panel-manual-tab-activation branch from 7c5b61a to 8ef6ea4 Compare November 25, 2022 21:39
@ciampo
Copy link
Contributor Author

ciampo commented Nov 25, 2022

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).

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🚀

packages/components/src/tab-panel/README.md Outdated Show resolved Hide resolved
packages/components/src/tab-panel/types.ts Outdated Show resolved Hide resolved
@ciampo
Copy link
Contributor Author

ciampo commented Nov 28, 2022

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 ciampo enabled auto-merge (squash) November 28, 2022 21:26
@ciampo ciampo merged commit 03a278e into trunk Nov 28, 2022
@ciampo ciampo deleted the feat/tab-panel-manual-tab-activation branch November 28, 2022 21:58
@github-actions github-actions bot added this to the Gutenberg 14.7 milestone Nov 28, 2022
@DaisyOlsen DaisyOlsen added [Type] Enhancement A suggestion for improvement. and removed [Type] Feature New feature to highlight in changelogs. labels Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TabPanel: add support for manual activation of tabs
7 participants