Tabs: improve focus behavior

[chad1008]

Note

We're talking about Tabs, which is a new version of TabPanel. There are subcomponents whose names involve the word 'Tab' or 'TabPanel'. There are actual elements with tab and tabpanel roles. To top it all off, this PR focuses on the behavior of the [Tab] key on the keyboard. Basically there are a lot of ways to use the word "tab" here, so let me know if anything fails to make sense 😉

What?

Add focus styles and a new prop to Tabs.TabPanel to declare whether or not the tabpanel itself should be focusable.

Why?

The relevant ARIA guidance for the Tabs Pattern states:

When the tab list contains the focus, [Tab] 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.

How?

Currently, the [Tab] key moves focus from the tablist to the currently displayed tabpanel, regardless of the presence of focusable child elements within that tabpanel.

We've decided to maintain the existing behavior as the default to remain consistent with the behavior of other components (Modal, for example). Along with that, this PR exposes a focusable prop on the Tabs.TabPanel subcomponent to better control this behavior.

The new prop will default to true, making the tabpanel focusable, so it will be the next tabstop when using the [Tab] key to navigate off of the tablist. When the new prop is false, the tabpanel is no longer focusable. Focus will instead jump to the first focusable element in the DOM. That could be a button or input within the tabpanel, or if none are present, whatever the next tabstop is elsewhere on the page. We aren't targeting any specific elements to focus, as we don't have a reliable means of determining which elements contain "meaningful content" from one implementation to the next.

While this doesn't perfectly align with the guidance described above, it feels like a good compromise of consistency with other components and flexibility for consumers.

Testing Instructions

• Launch Storybook and select the Default story
• Move the focus to the main canvas, either by clicking on the empty space of the main pane, or by pressing [Tab] followed by [Return/Enter]
• Use the [Tab] key to focus the first Tab. You may need to press it twice to cycle over Storybook's "Return to sidebar" button.
• Once the first Tab has focus (it will gain an outline), press the [Tab] key again, and confirm that the focus moves to the tabpanel element.
• While the tabpanel is focused, inspect the element and confirm the component's new :focus-visible style is overriding the browser defaults.
• Select the "TabPanels with focusable={ false }" story. Repeat the above steps to test the tabstop behavior for each tab. Remember to use your arrow keys to navigate between Tabs within the tablist
• For each Tab, confirm that the tabpanel does not get focus. The focus should instead cycle through any focusable elements within the TabPanel contents, then to the button below (and outside of) the component.
• For the third tab in the story, confirm that the [Tab] key skips the focus directly down to the button lower on the page due to the absence of any focusable items within the displayed tabpanel.

[alexstine]

@chad1008 Setting this prop to true will likely have unattended consequences in trunk. I am pretty sure my E2E test should catch any failures but need to see how this behaves around my custom list view focus management code in edit-post/edit-site.

[chad1008]

There shouldn't be any issues on trunk for now @alexstine 🙂

This new prop is isolated to Tabs, but that hasn't yet replaced TabPanel which is what's actively in use. I'm starting work on some experimental PRs to test Tabs in the editor, so I'll keep an eye out for any existing focus behavior as I go!

[brookewp]

This tests well in SB when going through your detailed instructions! 🎉

One note - the story reads a bit like a unit test to me. Since focusable is set per panel, perhaps instead it could be demoed in the default story by mentioning it in the tab panel content? Then have a unit test to test the behaviors you've outlined in your steps.

[ciampo]

@alexstine , to add some more context to the work being done here:

• Tabs is a new, separate component from TabPanel, and is currently not used anywhere in the editor (it's not even yet exported from the components package).
• Compared to TabPanel, it has a few advantages (mainly, the fact that it exposes its subcomponents, allowing for more flexibility when building the UI, and the fact that it can be controlled from an external consumer)
• Therefore, you can rest assured that existing tabs behaviour won't change as a result of this PR changing
• Soon, we plan on experimenting with using the new Tabs component instead of the old TabPanel component (and we will definitely ping you)

This PR simply adds extra control for consumers of Tabs to decide whether the tabpanel itself should be focusable (ie. a tab stop) or not.

[chad1008]

One note - the story reads a bit like a unit test to me. Since focusable is set per panel, perhaps instead it could be demoed in the default story by mentioning it in the tab panel content? Then have a unit test to test the behaviors you've outlined in your steps.

Thanks for that @brookewp! Somehow I just totally spaced on unit tests, so I've added a couple now.

Also liked your idea of demoing this prop in the default, so the new story is gone and Tab 3 of the default story now has this prop set up to serve as an example.

[github-actions]

Flaky tests detected in 096f66a.
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/6786306372
📝 Reported issues:

• [Flaky Test] create a new pattern #54983 in /test/e2e/specs/site-editor/patterns.spec.js

[brookewp]

In your testing instructions, you have the following step:

While the tabpanel is focused, inspect the element and confirm the component's new :focus-visible style is overriding the browser defaults.

Since you mentioned it in your testing instructions, maybe it would be worth including in the test? Something like:

expect( selectedTabPanel ).toHaveAttribute( 'data-focus-visible' );

[chad1008]

The tabpanel element receiving :focus-visible isn't new in this case, so I don't want to add a test for it. The change around :focus-visible for this PR is that when the tab-panel gains that attribute, it gets styled by our new CSS, rather than the browser defaults, which is what was styling it previously :)

[brookewp]

Thanks for clarifying! 👍

[brookewp]

Have you run into any flaky test behavior? When watching the tests, sometimes I'll get a random test failing. For an example:

● Tabs › Uncontrolled mode › Disabled tab › should disable the tab when `disabled` is `true`

expect(element).toHaveFocus()

Expected element with focus:
<button aria-controls="tabs-21-delta-view" aria-disabled="true" aria-selected="false" class="delta-class css-1v2gegi-Tab enfox0g1" data-command="" id="tabs-21-delta" role="tab" tabindex="-1" type="button">Delta</button>
Received element with focus:
<button aria-controls="tabs-21-alpha-view" aria-selected="true" class="alpha-class css-1v2gegi-Tab enfox0g1" data-active-item="" data-command="" data-focus-visible="" id="tabs-21-alpha" role="tab" type="button">Alpha</button>

It'll pass the next 4-5 times then fail again. I'm wondering if it's something related to past flaky test issues we've enountered in the ariakit migrations: #52133 (comment)

[chad1008]

Hm. If I run them a bunch in a row, I do see that. 🤔 Took me probably close to 20 runs to trigger it, but there it is. I can actually see two tests doing it:

• Tabs › Uncontrolled mode › Disabled tab › should disable the tab when disabledistrue`
• Tabs › Tab Activation › should not focus the next tab when the Tab key is pressed

Maybe it's related to that other issue you mentioned, but if it is, the same fix isn't helping us here... one of those failures is actually already using findByRole.

I'll note this to investigate in a followup, that way if we deploy something to fix it, it isn't wrapped up in this change!

[brookewp]

For tooltip, we added a helper function to ensure everything was resolved before moving to the next test. So I wonder if it would help to add another user.tab() at the end of each test (or to specific tests).

I'll note this to investigate in a followup, that way if we deploy something to fix it, it isn't wrapped up in this change!

Good plan!

[brookewp]

This is very minor, but I just noticed the variety of formatting of TabPanel in the types.

[chad1008]

Good catch. I made one small change, but I didn't update the style JSDoc because that one's going away in a separate PR merging soon, and changing it here would have just been asking for a merge conflict to reconcile 😆

[chad1008]

Thanks for the eyes @brookewp! I've responded to your feedback, added a CHANGELOG, and rebased.

[brookewp]

Looks good to me! 🎉

Just have the one changelog conflict to address... 😅