Tabs: sync browser focus to selected tab in controlled mode

[chad1008]

What?

In controlled mode, make browser focus follow any changes in tab selection that occur while a rendered tab currently has browser focus.

Why?

In some cases, it's possible for focus to be passed to the selected tab, only to have that selection change immediately afterwards. This could leave keyboard navigation and/or assistive tech in a confusing state where the focused tab does not reflect the content actually being displayed.

Real life example in the editor when tabbing to the sidebar without having a block actively selected.

How?

The main Tabs component now wraps its content provider in a div that we can attach a ref to. Using that ref, we can identify what DOM element currently has focus in the browser. Comparing that element's id to our tabs ids, we can then then make sure that the focus remains on the selected tab at the right times (specifically, only if a tab was actively focused when the change occurred).

I wasn't able to use Ariakit's activeId value for this, because it appears to already follow the selected tab, which can leave it out of sync with browser focus if conditions are right as in the example above

Testing Instructions

Apply the following diff to perform this test in Storybook:

diff --git a/packages/components/src/tabs/stories/index.story.tsx b/packages/components/src/tabs/stories/index.story.tsx
index ce8c8324ed..2a724c0171 100644
--- a/packages/components/src/tabs/stories/index.story.tsx
+++ b/packages/components/src/tabs/stories/index.story.tsx
@@ -273,17 +273,29 @@ const ControlledModeTemplate: StoryFn< typeof Tabs > = ( props ) => {
 					<DropdownMenu
 						controls={ [
 							{
-								onClick: () => setSelectedTabId( 'tab1' ),
+								onClick: () =>
+									setTimeout(
+										() => setSelectedTabId( 'tab1' ),
+										3000
+									),
 								title: 'Tab 1',
 								isActive: selectedTabId === 'tab1',
 							},
 							{
-								onClick: () => setSelectedTabId( 'tab2' ),
+								onClick: () =>
+									setTimeout(
+										() => setSelectedTabId( 'tab2' ),
+										3000
+									),
 								title: 'Tab 2',
 								isActive: selectedTabId === 'tab2',
 							},
 							{
-								onClick: () => setSelectedTabId( 'tab3' ),
+								onClick: () =>
+									setTimeout(
+										() => setSelectedTabId( 'tab3' ),
+										3000
+									),
 								title: 'Tab 3',
 								isActive: selectedTabId === 'tab3',
 							},

The above diff adds a delay to the controlled-mode selection changes. This will allow you to simulate the tab changing on you while it's already focused.

1. Launch Storybook and open the "Controlled Mode" story
2. Using the dropdown, choose a tab to switch to
3. Before it updates (you have three seconds) click somewhere on the canvas, and press [Tab] to focus the currently selected tab.
4. Note that when the controlled selection takes effect, the newly selected tab receives browser focus.
5. Repeat this test, but skip step three. Don't force the focus onto a tab.
6. Confirm that the focus doesn't move when the selection kicks in (in this case, it stays on the dropdown toggle)

Notes

In most implementations, if a tab has focus it will automatically be the selected tab, because by default focusing a tab selects it. The exception to this is when selectOnMove = false. This means that in controlled mode, with that prop set to false it is possible for a user to be navigating tabs with their arrow keys at the same time that a controlling component forces a selection change. I expect that to be pretty rare, and this change will mostly take effect during race conditions like the one in the example above. I don't anticipate it happening often while someone's casually navigating a bit of UI. It is technically possible though, and with this PR in effect that would move the focus to the newly selected tab. I went back and forth on how to approach that.

On the one hand, we could limit this change to only fire when selectOnMove = true, or fine-tune it a bit to only take effect if the selected tab specifically was focused when the controlled selection change took effect.

On the other hand, I thought the change in focus might be a good way to notify assistive tech that something had just happened, rather than letting it happen silently in the background with no warning. Very much open to feedback on this point! cc @andrewhayward in case you have thoughts.

[ciampo]

Thank you for working on this!

It would be great if we had unit tests, so that we could also test for alternative solution much quicker!

I would also love to hear from @diegohaz in case he had any thoughts on alternative solutions

[github-actions]

Flaky tests detected in e4a28c0.
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/7143785893
📝 Reported issues:

• [Flaky Test] should display the "Manage Fonts" icon #56776 in /test/e2e/specs/site-editor/font-library.spec.js
• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

[andrewhayward]

On the one hand, we could limit this change to only fire when selectOnMove = true, or fine-tune it a bit to only take effect if the selected tab specifically was focused when the controlled selection change took effect.

On the other hand, I thought the change in focus might be a good way to notify assistive tech that something had just happened, rather than letting it happen silently in the background with no warning.

Tricky one. Refocusing can be very disorienting, particularly when you don't know why it happened, so should be avoided. But at the same time, being out of sync with a control can also be confusing, particularly when focus is tied to selection state.

I'd err on not switching focus over staying in sync, but if we can prevent that from happening too then so much the better. Looks like you're already accounting for selectOnMove? Does this PR now resolve the issue in #55360?

[chad1008]

I'd err on not switching focus over staying in sync, but if we can prevent that from happening too then so much the better. Looks like you're already accounting for selectOnMove? Does this PR now resolve #55360 (comment)?

Perfect, thank you for the input! And yes, this PR should address that issue.

[ciampo]

The code is looking good! Could you also add a unit test for the desired behaviour (both controlled/uncontrolled, selectOnMove true/false) ?

[chad1008]

Could you also add a unit test for the desired behaviour (both controlled/uncontrolled, selectOnMove true/false) ?

I've added the new tests, but they're currently flakey. It doesn't look like the tests themselves, as they pass consistently when run individually. It's when I run them together that they fail.

There are two new uncontrolled mode tests, and I only see the second one occasionally being flaky, not the first. There are four new controlled mode tests, and the flakiness there also appears isolated to the second, third, and/or fourth but not the first.

All of that to say, it really feels like something is leaking between tests but I can't tell what, and I haven't yet figured out how to prevent it.

One other note: there isn't really a way to test the new behavior of this PR in uncontrolled mode, due to the nature of the changes, so the uncontrolled tests are really only testing that selectOnMove works properly, but not the new stuff. Still good tests to have though!

I'm going to switch tasks for a bit, but I'll circle back to continue investigating what's leaking between these tests later.

[chad1008]

Update: moving from userEvent to ariakit/test utils appears to resolve the timing/leaking issues I was seeing in unit tests. (h/t @ciampo for the suggestion).

I'm moving the existing tests to ariakit/test in #56835, and will update the new tests on this PR once it has been rebased.

[chad1008]

The ariakit/test changes are all in place, and the tests are passing consistently for me locally 🎉

[chad1008]

CI checks are finally passing! One of them was oddly stubborn, but passes consistently locally, and is now fine here as well.

[ciampo]

Not something for this PR but rather for a potential follow-up:

I may be forgetting some context about past decisions, but is there a reason why the "Accessibility and semantics", "Focus Behavior", "Tab Attributes", and "Tab Activation" sub-sections in the tests are only testing the uncontrolled version of the component? And why do we have then dedicated "Uncontrolled mode" and "Controlled mode" sections?

I feel like a better structure could be to:

• have a top-level describe.each which makes sure that we're testing both uncontrolled and controlled version of the components
• for controlled-only and uncontrolled-only, we could have separate sections (or subsections)

[chad1008]

I may be forgetting some context about past decisions, but is there a reason why the "Accessibility and semantics", "Focus Behavior", "Tab Attributes", and "Tab Activation" sub-sections in the tests are only testing the uncontrolled version of the component? And why do we have then dedicated "Uncontrolled mode" and "Controlled mode" sections?

If I recall correctly, that structure grew out of how we implemented Controlled Mode initially, with a focus on how tab activation would differ (or not) between the two modes. When I added the "Controlled" and "Uncontrolled" sections originally, it was to test those differences in behavior between the two modes. The tests in those first sections you've mentioned are things that shouldn't be impacted by the behavioral changes between the two modes.

I feel like a better structure could be to:

• have a top-level describe.each which makes sure that we're testing both uncontrolled and controlled version of the components
• for controlled-only and uncontrolled-only, we could have separate sections (or subsections)

A problem with a top-level describe.each (but please correct me if there's a good way around this) is that a fair number of tests will be different in controlled vs uncontrolled, so the assertions would change. For example in the table linked above, an assertion for what the selected tab should be when there is no initialTabId provided wouldn't be the same for both runs of the .each. We'd probably end up with a lot of conditional assertions.

All of that said, I'm more than happy to look into reworking the testing suite's structure! At the very least, we can probably run more of the tests through both modes, making sure controlled/uncontrolled doesn't impact them in ways they shouldn't.

[ciampo]

🚀

Regarding the structure of the test file, definitely not a priority right now anyway.