-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Editor: Fix JS error in the 'useTabNav' hook #67102
Conversation
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. |
@@ -64,7 +64,7 @@ export default function useTabNav() { | |||
.focus(); | |||
} | |||
// If we don't have any section blocks, focus the section root. | |||
else { | |||
else if ( sectionRootClientId ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide a fallback here? However, I'm not sure what the best one is for Zoom Out mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getdave - is the intention that getSectionRootClientId()
will always return something? My question is, should we handle a fallback in case there isn't a section root, or is the bug that a sectionRoot should always be defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, technically, it does; it returns an empty string, "," which in the editor means the main root. But it's only an internal marker, has no representation in Canvas, and can't be focused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to be able to access the editor canvas and know it's empty. I'd support moving focus to the canvas wrapper as a fallback. If there are no empty states or focus, we can add those in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of fallback I'd just allow it to focus the canvas if the section root is an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 8920245.
Size Change: +5 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
52ebe57
to
8920245
Compare
8920245
to
ef210ef
Compare
Flaky tests detected in ef210ef. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12024478497
|
Also realised that #67232 will help guard against this. But we still need this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resolves the error for me. Placing focus on the canvas is better than nothing and certainly better than an error 👍
Thanks for testing, @getdave! |
* Block Editor: Fix JS error in the 'useTabNav' hook * Focus on canvas when there's no section root Co-authored-by: Mamaduka <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: getdave <[email protected]>
* Block Editor: Fix JS error in the 'useTabNav' hook * Focus on canvas when there's no section root Co-authored-by: Mamaduka <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: getdave <[email protected]>
What?
PR fixes the JS error in the
useTabNav
hook whensectionRootClientId
isn't available in Zoom Out mode.Testing Instructions
Testing Instructions for Keyboard
Same.
Screenshots or screencast