Tabs: sync browser focus to selected tab in controlled mode #56658

chad1008 · 2023-11-29T22:44:50Z

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.

Launch Storybook and open the "Controlled Mode" story
Using the dropdown, choose a tab to switch to
Before it updates (you have three seconds) click somewhere on the canvas, and press [Tab] to focus the currently selected tab.
Note that when the controlled selection takes effect, the newly selected tab receives browser focus.
Repeat this test, but skip step three. Don't force the focus onto a tab.
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

packages/components/src/tabs/index.tsx

github-actions · 2023-12-01T13:17:26Z

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 · 2023-12-01T14:53:09Z

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 · 2023-12-01T16:12:59Z

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 · 2023-12-01T17:16:58Z

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

chad1008 · 2023-12-04T18:26:08Z

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 · 2023-12-06T17:12:48Z

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 · 2023-12-07T16:17:08Z

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

chad1008 · 2023-12-07T21:30:28Z

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)

packages/components/src/tabs/test/index.tsx

chad1008 · 2023-12-08T16:44:44Z

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.

chad1008 added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Nov 29, 2023

chad1008 self-assigned this Nov 29, 2023

chad1008 requested a review from ajitbohra as a code owner November 29, 2023 22:44

chad1008 requested a review from a team November 29, 2023 22:45

chad1008 mentioned this pull request Nov 29, 2023

Implement Tabs in editor settings #55360

Merged

ciampo reviewed Nov 30, 2023

View reviewed changes

packages/components/src/tabs/index.tsx Outdated Show resolved Hide resolved

packages/components/src/tabs/index.tsx Outdated Show resolved Hide resolved

packages/components/src/tabs/index.tsx Show resolved Hide resolved

packages/components/src/tabs/index.tsx Outdated Show resolved Hide resolved

chad1008 force-pushed the tabs-controlled-mode-focus branch from 308a036 to ea1e323 Compare December 1, 2023 16:09

chad1008 mentioned this pull request Dec 6, 2023

Tabs: Implement ariakit/test in unit tests #56835

Merged

chad1008 added 8 commits December 7, 2023 11:02

link browser focus to selected tab changes in controlled mode

88e443c

remove unnecessary memoization

85c4090

remove duplicate condition

8f92beb

dont apply when selectOnMove is false

b86bbd1

use items to find active element, instead of a ref

62b5f67

CHANGELOG

0450da7

add selectOnMove unit tests

dc04a47

convert new tests to ariakit/test

0738fd7

chad1008 force-pushed the tabs-controlled-mode-focus branch from 09ce42d to 0738fd7 Compare December 7, 2023 16:16

ciampo reviewed Dec 8, 2023

View reviewed changes

packages/components/src/tabs/test/index.tsx Outdated Show resolved Hide resolved

remove redundant tests

e4a28c0

ciampo approved these changes Dec 8, 2023

View reviewed changes

chad1008 merged commit 2e95c20 into trunk Dec 8, 2023
52 checks passed

chad1008 deleted the tabs-controlled-mode-focus branch December 8, 2023 18:18

github-actions bot added this to the Gutenberg 17.3 milestone Dec 8, 2023

chad1008 mentioned this pull request Jan 9, 2024

Components: Tabs: Improve Controlled Mode Focus Handling #57696

Merged

Directory

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tabs: sync browser focus to selected tab in controlled mode #56658

Tabs: sync browser focus to selected tab in controlled mode #56658

chad1008 commented Nov 29, 2023

ciampo left a comment

github-actions bot commented Dec 1, 2023 •

edited

andrewhayward commented Dec 1, 2023

chad1008 commented Dec 1, 2023

ciampo commented Dec 1, 2023

chad1008 commented Dec 4, 2023

chad1008 commented Dec 6, 2023

chad1008 commented Dec 7, 2023

chad1008 commented Dec 7, 2023

ciampo left a comment

chad1008 commented Dec 8, 2023

ciampo left a comment

Directory

Tabs: sync browser focus to selected tab in controlled mode #56658

Tabs: sync browser focus to selected tab in controlled mode #56658

Conversation

chad1008 commented Nov 29, 2023

What?

Why?

How?

Testing Instructions

Notes

ciampo left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023 • edited

andrewhayward commented Dec 1, 2023

chad1008 commented Dec 1, 2023

ciampo commented Dec 1, 2023

chad1008 commented Dec 4, 2023

chad1008 commented Dec 6, 2023

chad1008 commented Dec 7, 2023

chad1008 commented Dec 7, 2023

ciampo left a comment

Choose a reason for hiding this comment

chad1008 commented Dec 8, 2023

ciampo left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023 •

edited