Do not focus new navigation block menu until loading is finished #59801
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
When a new navigation is created, we place focus on the navigation block. If we place focus while loading is still happening, focus will be lost when focus the loading is finished and the block gets replaced with the new content.
jeryj
changed the title
|
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. |
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
Instead of replacing the navigation block after loading, only replace the inside part of it so that focus is not lost
draganescu
approved these changes
Mar 13, 2024
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… used when creating a menu, this way the select block call is later and the focus is maintained
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
getdave
added
Backport to WP Beta/RC
getdave
reviewed
Mar 13, 2024
Comment on lines
583
to
588
| // Make sure it's a nav block appender. If it is, the linkControl search input will be opened on enter. | ||
| await page.keyboard.press( 'Enter' ); | ||
|
|
||
| // Expect to see the Link creation UI be focused. | ||
| const linkUIInput = linkControl.getSearchInput(); | ||
| await expect( linkUIInput ).toBeFocused(); |
There was a problem hiding this comment.
As this behaviour might change in future, would it be possible to scope the selector for the Add block to be relative to the Navigaiton block.
So in pseudo code
editor.canvas.getNavBlock().getByLabel('Add block')
…turn a Promise Co-authored-by: Dave Smith <getdavemail@gmail.com>
|
Thanks for the reviews and fixes, @draganescu and @getdave! This is ready for another review. |
|
The failed e2e tests pass locally. Rerunning. |
getdave
approved these changes
Mar 14, 2024
There was a problem hiding this comment.
I tested and this and the behaviour was as described and expected.
In the longer term, I'm wondering whetehr it would probably be better to have focus remain in the sidebar where you invoked the "Create" action. But for now this manages focus in a much better way and resolves the bug.
Great work 👏
|
This PR should be included in the WordPress 6.5 release during the RC phase. The bug was identified during the RC cycle and represents a focus loss issue which could make navigating the editor very difficult for users of assistive tech. |
youknowriad
pushed a commit
that referenced
this pull request
Mar 18, 2024
) * Do not focus navigation block until loading is finished When a new navigation is created, we place focus on the navigation block. If we place focus while loading is still happening, focus will be lost when focus the loading is finished and the block gets replaced with the new content. * Nest loading state inside of navigation ref Instead of replacing the navigation block after loading, only replace the inside part of it so that focus is not lost * Add coverage for focus loss on menu creation * select the navigatiom after import in an effect similar to the method used when creating a menu, this way the select block call is later and the focus is maintained * remove extraneous dependency causing err on delete newly created menus * Update test to not determine link control behavior * No need to return await inside an async function as it will always return a Promise Co-authored-by: Dave Smith <getdavemail@gmail.com> --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Dave Smith <getdavemail@gmail.com>
|
I just cherry-picked this PR to the update/packages-rc2-6.5 branch to get it included in the next release: 1d93d98 |
youknowriad
added
Backported to WP Core
getdave
added a commit
that referenced
this pull request
Mar 18, 2024
) * Do not focus navigation block until loading is finished When a new navigation is created, we place focus on the navigation block. If we place focus while loading is still happening, focus will be lost when focus the loading is finished and the block gets replaced with the new content. * Nest loading state inside of navigation ref Instead of replacing the navigation block after loading, only replace the inside part of it so that focus is not lost * Add coverage for focus loss on menu creation * select the navigatiom after import in an effect similar to the method used when creating a menu, this way the select block call is later and the focus is maintained * remove extraneous dependency causing err on delete newly created menus * Update test to not determine link control behavior * No need to return await inside an async function as it will always return a Promise Co-authored-by: Dave Smith <getdavemail@gmail.com> --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Dave Smith <getdavemail@gmail.com>
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…dPress#59801) * Do not focus navigation block until loading is finished When a new navigation is created, we place focus on the navigation block. If we place focus while loading is still happening, focus will be lost when focus the loading is finished and the block gets replaced with the new content. * Nest loading state inside of navigation ref Instead of replacing the navigation block after loading, only replace the inside part of it so that focus is not lost * Add coverage for focus loss on menu creation * select the navigatiom after import in an effect similar to the method used when creating a menu, this way the select block call is later and the focus is maintained * remove extraneous dependency causing err on delete newly created menus * Update test to not determine link control behavior * No need to return await inside an async function as it will always return a Promise Co-authored-by: Dave Smith <getdavemail@gmail.com> --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Dave Smith <getdavemail@gmail.com>
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.
Fixes #59771
When a new navigation is created, we place focus on the navigation block. If we place focus while loading is still happening, focus will be lost when focus the loading is finished and the block gets replaced with the new content.
Instead of replacing the entire nav HTML element, just replace the inner part of it so focus is not lost.
What?
Why?
How?
Testing Instructions