Fix the List View focus management #69190
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afercia
added
[Type] Bug
|
Size Change: +183 B (+0.01%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
afercia
force-pushed
the
fix/test-list-view-focus
branch
from
483936c to
2ae83b0
Compare
6 tasks
afercia
force-pushed
the
fix/test-list-view-focus
branch
from
2ae83b0 to
a90b76b
Compare
|
Flaky tests detected in 263f379. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13457053249
|
6 tasks
afercia
force-pushed
the
fix/test-list-view-focus
branch
from
a90b76b to
263f379
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I'd recommend to test this PR with a post that contains at least one nested block e.g. a PAragraph within a Group. Sample content: |
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Closes #69147
It is important to understand the List View focus management and the intended behavior of the keyboard shortcut as implemented in #45135
This mechanism is buggy for several user flows, some of them are detailed on the associated issue #69147.
This PR aims to make the focus management and the keyboard shortcut behavior more reliable and adds e2e tests to cover some failing scenarios.
Why?
The List View keyboard shortcut should always work as intended.
The currently selected item (if any) should always receive focus when opening the List View or when using the keyboard shortcut.
How?
Initial focus
One notable change is about initial focus:
When there are no blocks selected:
Initial focus is now set on the first tab of the Document Overview panel because of two reasons:
RovingTabIndexItemhandles logic to make only one item "tabbable" at a time. Attempting to set initial focus on mount on the first item conflicts with theRovingTabIndexIteminternal logic and may end up in two items being "tabbable", which should never happen.When there are blocks selected:
Initial focus should be set to the corresponding item in the List View. On trunk, this fails in at lest two cases:
The 'Always open List View' preference
With this preference enabled, things are a little more complicated because:
The
core/editor/toggle-list-viewis used globally and it is also used in the ListViewSidebar component.This isn't a big deal when the panel is closed by default. However, when the panel is open by default because of the 'Always open List View' preference, using the keyboard shortcut will actually run both of them, the 'local' one and the 'global' one. As a result, the panel closes and then reopens immediately. It does it so fast that it's invisible to the eyes and it seems like nothing happened, the panel stays open. Apparently, this unintended behavior missed some proper testing when the preference was implemented.
Note: This happens only on first page load. This PR tries to introduce some logic to determine whether the
ListViewSidebarrenders for the first time or not.Technical points for discussion
ListViewSidebarnow uses arenderCountervariable to determine if it renders for the first time or not. I'm not sure this is the best 'React' way to do it but I couldn't find other ways. I also tried to useuseInstanceIdbut it doesn't guarantee the count is correct because it must be used inside the component and at that point it appears the component already rendered twice. Any suggestions for a better approach are welcome.ListViewComponentnow uses a timeout when callingfocusListItem, which is the function responsible for setting focus on the selected item. The timeout is important to preventuseFocusOnMount(which internally does use a timeout) to set focus last. Actually,focusListItemshould be called last instead.data-is-selectedattribute to make moving focus to the actual selected item more reliable. Suggestions for any better approach are welcome.Testing Instructions
Regardless of whether the List View is opened and closed by clicking the toggle button in the top bar or via the keyboard shortcut the most important behaviors to test are:
Testing Instructions for Keyboard
Screenshots or screencast