Navigation Block: Use dom.focus for focus control#57362
Merged
Conversation
t-hamano
added
[Type] Bug
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php |
|
Size Change: -52 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
t-hamano
requested review from
Mamaduka,
kevin940726,
spacedmonkey and
tellthemachines
as code owners
t-hamano
commented
Dec 24, 2023
Comment on lines
-115
to
+119
| // Not Tested: overlay menu traps focus | ||
| // Test: overlay menu traps focus | ||
| await pageUtils.pressKeys( 'Tab', { times: 2, delay: 50 } ); | ||
| await expect( closeMenuButton ).toBeFocused(); | ||
| await pageUtils.pressKeys( 'Shift+Tab', { times: 2, delay: 50 } ); | ||
| await expect( overlayMenuFirstElement ).toBeFocused(); |
Contributor
Author
There was a problem hiding this comment.
I didn't know why this test wasn't done in Safari, but let's try it and see if it works.
Mamaduka
reviewed
Dec 26, 2023
Member
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
Using the dom package could significantly increase the bundle size (for a front-end script). Using plain JS might be better for the moment.
Contributor
Author
Thank you for teaching me. Certainly, I think that it can be modified with a simple code without using this package, so I will try it. |
luisherranz
approved these changes
Dec 28, 2023
Member
luisherranz
left a comment
luisherranz
left a comment
There was a problem hiding this comment.
Look good to me, both the code and the fix 🙂
getdave
reviewed
Jan 25, 2024
| $responsive_dialog_directives = ''; | ||
| $close_button_directives = ''; | ||
| if ( $should_load_view_script ) { | ||
| $open_button_directives = ' |
Contributor
There was a problem hiding this comment.
I checked and these changes are still included in the WP_Navigation_Block_Renderer which has now been moved to the block-library package and thus will be automatically synced to Core as part of the packages sync process.
Contributor
|
✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR no longer requires a backport for WP 6.5. Why? Because the code was moved into the block-library package in #57979 and thus should be automatically handled by the packages sync process. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 #57359
What?
This PR avoids using
querySelector/querySelectorAlland usesfocusfrom the@wordpress/dompackage for focus control in the navigation dialog. This fixes a focus error when the Navigation block has a search block, as reported in #57359.Why?
The following two things are expected in the Navigation block dialog:
Regarding the first point, it tries to find focusable elements in the dialog starting from the element with the
.wp-block-navigation-itemclass. However, if it is just a Search block, there is no element with this class, so focus will fail and you will get a console error.How?
The@wordpress/dompackage has afocusobject that finds focusable elements. I used this and replacedquerySelector/querySelectorAll. This should always focus the appropriate first element, no matter what blocks the navigation block contains.Update: To avoid the increase in bundle size, I fixed the logic of a problem (searching for the first focusing element in the dialog) without using the
@wordpress/dompackage.
I also moved the
focusFirstElementcallback to the content wrapper div (.wp-block-navigation__responsive-container-content) to accurately locate the first element (not including the close button) when the dialog opens.Before:
After:
Testing Instructions for Keyboard
I'm a Windows user, and I've confirmed it works in Chrome and Firefox browsers. I would be happy if you could confirm that it works as expected on Mac OS.
Tabkey to focus on the navigation button and pressEnter.Screenshots or screencast
27ab37f5125e56c5a469012622c43858.mp4