Fix navigation link ui close focus management #59925
Merged
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
|
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. |
jeryj
added
[Type] Bug
jeryj
changed the title
* Sets openedBy state based on what opened the link ui (toolbar link button or navigation link block text * Returns focus to the item that opened it * If the action is cancelled (no link is added), move focus to the previous block (sibling navigation link or parent navigation link or block) * linting changes to let me save * Removed a case where focus would no longer need to be handled
jeryj
force-pushed
the
fix/navigation-link-close-focus-management
branch
from
51b7df6 to
00be503
Compare
|
Size Change: +1.91 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
jeryj
commented
Mar 22, 2024
| @@ -93,7 +90,7 @@ const useIsDraggingWithin = ( elementRef ) => { | |||
| ownerDocument.removeEventListener( 'dragend', handleDragEnd ); | |||
| ownerDocument.removeEventListener( 'dragenter', handleDragEnter ); | |||
| }; | |||
| }, [] ); | |||
| }, [ elementRef ] ); | |||
| @@ -226,7 +229,7 @@ export default function NavigationLinkEdit( { | |||
| /** | |||
| * Transform to submenu block. | |||
| */ | |||
| function transformToSubmenu() { | |||
| const transformToSubmenu = () => { | |||
Comment on lines
+277
to
+281
| ! prevUrl && | ||
| url && | ||
| isLinkOpen && | ||
| isURL( prependHTTP( label ) ) && | ||
| /^.+\.[a-z]+/.test( label ) |
There was a problem hiding this comment.
We only want the label text to be selected if we add a link with a custom url like https://www.example.com. Previously, this would select the label text every time the link popover closed, not just when the link was created. We only want this to happen when the link is created.
Comment on lines
-281
to
-282
| // Focus it (but do not select). | ||
| placeCaretAtHorizontalEdge( ref.current, true ); |
There was a problem hiding this comment.
We want the caret to go back to wherever it was. We shouldn't assume it to go to the start.
| @@ -290,6 +313,220 @@ test.describe( 'Navigation block', () => { | |||
| await expect( warningMessage ).toBeVisible(); | |||
| } ); | |||
|
|
|||
| test( 'creating navigation menus via keyboard without losing focus', async ( { | |||
There was a problem hiding this comment.
This test is huge, but it's better that it is:
- Faster than running it in several smaller tests
- We don't need to "redo" work in each test. For example, each test needs to build off the previous one, so there would either be repeating tests or lots of initial menus to start from.
- Creating a navigation block using a keyboard should work. This is a more realistic example of creating a menu and is more likely to catch edge cases.
I tried to break it down into sensible smaller tests, and refactor reusable pieces but keep them readable. I wrote this with the idea that it will likely need to be debugged since I expect focus management bugs to happen. When we debug it or change the implementation, my intent is that this will be easier to understand and maintain.
scruffian
approved these changes
Mar 27, 2024
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
Co-authored-by: jeryj <[email protected]> Co-authored-by: scruffian <[email protected]>
t-hamano
reviewed
May 4, 2024
| @@ -565,10 +573,24 @@ export default function NavigationLinkEdit( { | |||
| // If there is no link then remove the auto-inserted block. | |||
| // This avoids empty blocks which can provided a poor UX. | |||
| if ( ! url ) { | |||
| // Need to handle refocusing the Nav block or the inserter? | |||
| // Select the previous block to keep focus nearby | |||
| selectPreviousBlock( clientId, true ); | |||
There was a problem hiding this comment.
Perhaps this line is causing the main inserter to not open when you click the "Browse All" button (See #61361).
My guess is that this occurs for the following reasons:
- The main inserter opens when "Browse All" is clicked (See).
onCloseis called because the focus leaves the popover.- Since the URL does not exist, this part will be executed and the previous block will be selected.
- The main inserter automatically closes because the block is selected.
I don't understand everything about this PR, but I would suggest not binding onSelectBlock directly to the popover component's onClose prop. It might be better to clearly separate onClose and onSelectBlock, and only move the focus to the previous block when onSelectBlock is called.
I haven't tested it thoroughly, but the code below works fine as far as I've tested it. I hope this helps.
diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js
index 4902a6cb2b..cb89881f79 100644
--- a/packages/block-library/src/navigation-link/edit.js
+++ b/packages/block-library/src/navigation-link/edit.js
@@ -562,6 +562,15 @@ export default function NavigationLinkEdit( {
clientId={ clientId }
link={ attributes }
onClose={ () => {
+ // When onClose is called, it means that the "Browse all" button
+ // was clicked and the main inserter was opened. In this case,
+ // remove the empty block and close the link UI. It does not return
+ // focus to the previous block to prevent the main inserter from
+ // closing.
+ onReplace( [] );
+ setIsLinkOpen( false );
+ } }
+ onSelectBlock={ () => {
// If there is no link then remove the auto-inserted block.
// This avoids empty blocks which can provided a poor UX.
if ( ! url ) {
diff --git a/packages/block-library/src/navigation-link/link-ui.js b/packages/block-library/src/navigation-link/link-ui.js
index ce79af40e4..64a742e776 100644
--- a/packages/block-library/src/navigation-link/link-ui.js
+++ b/packages/block-library/src/navigation-link/link-ui.js
@@ -208,10 +208,6 @@ export function LinkUI( props ) {
`link-ui-link-control__description`
);
- // Selecting a block should close the popover and also remove the (previously) automatically inserted
- // link block so that the newly selected block can be inserted in its place.
- const { onClose: onSelectBlock } = props;
-
return (
<Popover
placement="bottom"
@@ -291,7 +287,7 @@ export function LinkUI( props ) {
setAddingBlock( false );
setFocusAddBlockButton( true );
} }
- onSelectBlock={ onSelectBlock }
+ onSelectBlock={ props.onSelectBlock }
/>
) }
</Popover>There was a problem hiding this comment.
Thanks for noticing and debugging this, @t-hamano! The onClose is all work-arounds for focus management, as we couldn't work out a cleaner way to return focus to the + inserter when closing it. This test covers a lot of scenarios to help show what the onClose code does. The diff you shared does make the inserter work again, but breaks the focus management. I'm not sure how to keep them both working consistently. Maybe there's a way to tell the onClose to return early if we're opening the all blocks inserter panel?
There was a problem hiding this comment.
Yes, when I made this change locally and ran the E2E tests, some tests failed 😅 There may be a better approach.
ellatrix
reviewed
Aug 6, 2024
| // Now the appender should be visible and reachable with an arrow down | ||
| await pageUtils.pressKeys( 'ArrowDown' ); | ||
| await expect( navBlockInserter ).toBeFocused(); | ||
| } ); |
There was a problem hiding this comment.
Does this have to be one big test? It's nice to split things into smaller tests if possible, it's quite overwhelming to debug.
There was a problem hiding this comment.
I think it can certainly be splitted. I submitted #64305.
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 #58820
What?
Focus when closing the link ui has not been managed. This PR fixes focus loss from:
It also adds an extensive e2e test that checks for focus management and being able to create, use the link control, and delete navigation items and submenu items.
Why?
Accessibility, focus management. There were severe focus loss issues across the navigation block, especially when using the link control popover.
How?
Adds openedBy state to navigation-link and submenu components and handles the various situations.
Testing Instructions for Keyboard
Check for focus loss when interacting with the navigation block link ui only using your keyboard
For the navigation label selection issue:
Screenshots or screencast
This video shows all the steps in the "navigation manages focus for creating, editing, and deleting items" test in slowMo mode:
Screen.Recording.2024-03-22.at.1.58.53.PM.mov