Fix navigate regions shortcuts on the back button WP logo slot #63611
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
[Focus] Accessibility (a11y)
2 tasks
|
Size Change: -266 B (-0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
afercia
force-pushed
the
try/simplify-navigate-regions-shortcuts
branch
from
f4a78c5
to
3be09fa
Compare
|
Flaky tests detected in c67cad4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10961486395
|
afercia
requested review from
draganescu,
adamziel and
kevin940726
as code owners
|
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. |
|
@afercia, do you mind rebasing the branch and resolving merge conflicts? |
afercia
force-pushed
the
try/simplify-navigate-regions-shortcuts
branch
from
3be09fa
to
b1c9676
Compare
|
@Mamaduka rebased 👍🏻 |
afercia
force-pushed
the
try/simplify-navigate-regions-shortcuts
branch
from
b1c9676
to
dfb5f6d
Compare
|
@WordPress/gutenberg-core I'd appreciate a review. This fixes a regression that is important enough to be fixed in the next WordPress releae. thanks. |
|
In my testing, moving the keyboard shortcut event up in the tree has an added bonus. It makes the keyboard shortcut work also from the metaboxes area. Apparently, it didn't ever work from the metaboxes area. It works now which is even more important after #64351 I will update the test instructions. |
stokesman
reviewed
Sep 18, 2024
There was a problem hiding this comment.
This tests well for me in all the affected editors. Code-wise, I like it. The key thing it seems to me is that InterfaceSkeleton has a breaking change. Before this it applied region navigation by default and now it doesn’t (nor has the capability to do so). I think this is acceptable given that any plugins consuming InterfaceSkeleton aren’t going to experience breakage unless they upgrade their @wordpress/interface dependency. This is because the package isn’t exposed on the wp global; i.e. plugins using components from the package are bundling them into their build so they control the version.
A "breaking" changelog entry and dev note seem warranted. Other than that, this looks 🚢 shape to me.
noisysocks
added
the
Backport to WP 6.7 Beta/RC
|
@stokesman thanks for looking into this. Please do feel free to go ahead with the changelog entry. As a native English speaker I guess you're a better fit to do that 🙏🏻 |
getdave
force-pushed
the
try/simplify-navigate-regions-shortcuts
branch
from
c67cad4
to
0e87a88
Compare
|
@afercia @stokesman I noticed this was targetted for 6.7 but hadn't been merged. I've taken the liberty of rebasing it to resolve conflicts. In my testing based on the PR instructions this still works as expected but I'd appreciate you verifying before we look to merge 🙇 |
kevin940726
approved these changes
Oct 7, 2024
|
I can't remember the details about Anyways, I would like to remind everyone this is a regression and it should be fixed for the release. |
That prop is removed from two places:
It seems possible to not break |
|
I wasn't aware that the |
stokesman
added
the
Needs Dev Note
|
Given the breaking change a dev note seems like a proper thing so I added the label. Though, still the WP release won’t force the upgrade/breakage so I’m not totally sure it’s required. |
|
Thanks everyone for the in depth feedback. Is it OK to everyone I push the green button? Cc @youknowriad |
|
Go for it. Thanks all :) |
|
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk. |
|
Looks like the conflict when cherry picking this to Shall I just follow the instructions and move the entry under the |
I would think it‘s better to put under the |
I'm not sure if there is a documented official process but this is what #65905 did. Here's the original PR that one was a manual backport to the 6.7 branch for. |
afercia
added a commit
that referenced
this pull request
Oct 9, 2024
* Avoid to pass unnecessary props to useNavigateRegions. * Remove bubblesVirtually from WP logo slot. * Restore bubblesVirtually. * Move useNavigateRegions to an outermost ancestor. * Remove no longer used enableRegionNavigation prop. * Add changelog entry --------- Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
|
Thanks all. I created #65971 for the backport. |
kevin940726
added a commit
that referenced
this pull request
Oct 14, 2024
* Avoid to pass unnecessary props to useNavigateRegions. * Remove bubblesVirtually from WP logo slot. * Restore bubblesVirtually. * Move useNavigateRegions to an outermost ancestor. * Remove no longer used enableRegionNavigation prop. * Add changelog entry --------- Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
kevin940726
added a commit
that referenced
this pull request
Oct 14, 2024
* Avoid to pass unnecessary props to useNavigateRegions. * Remove bubblesVirtually from WP logo slot. * Restore bubblesVirtually. * Move useNavigateRegions to an outermost ancestor. * Remove no longer used enableRegionNavigation prop. * Add changelog entry --------- Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
kevin940726
added
Backported to WP Core
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…ress#63611) * Avoid to pass unnecessary props to useNavigateRegions. * Remove bubblesVirtually from WP logo slot. * Restore bubblesVirtually. * Move useNavigateRegions to an outermost ancestor. * Remove no longer used enableRegionNavigation prop. * Add changelog entry --------- Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
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 #63552
What?
The navigate regions keyboard shortcut doesn't work in the Post Editor when focus is on the WP Logo.
The WP logo now uses a Slot with the
bubblesVirtuallyprop. That means events 'bubble' up the React hierarchy tree which may be different from the DOM tree. This may be triggered by any other slot implementation added in the future so it would be best to find a solit way to make sure navigate regions works in any case.This works in the Site editor but it doesn't in the Post editor. It took me a while to understand how the different React components hierarchy impacts this feature.
In the Site editor:
It works because
useNavigateRegionsis used on the edit-site Layout component. This component contains basically everything so that the Slots are inside the Layout in the React hierarchy.In the Post editor
It does not work because
useNavigateRegionsis used on theInterfaceSkeletoncomponent. However, the WP logoBackButtonis actually inside the Layout (edit-post package) > Editor (editor package) component, which thenuses EditorInterface which in turn uses InterfaceSkeleton.Similar hierarchy in the Widgets editor.
Basically, to make sure the navigate regions shortcut works in all editors regardless of where any Slot is used, there is the need to make sure
useNavigateRegionsis used on the outermost component of the editors.This PR tries this approach by moving the usage of
useNavigateRegionsup high in the components hierarchy to a couple ew div elements. If it turns to work well, maybe it would be worth considering to abstract to a 'navigate region provider' component that can be reused where desired.Cc @youknowriad to my understanding there are a few experimental 'editor providers' in place that I guess they are meant to attach behaviors to the editor and be consolidated in the future. Would a new provider for the navigate region props and ref be a good option?
Why?
To make sure the navigate regions keyboard shortcut works from everywhere.
How?
useNavigateRegionshigher up in the components hierarchy.useNavigateRegionsalready has default shortcuts.enableRegionNavigationprop.Testing Instructions
PostsAppexperiment and test.Testing Instructions with metaboxes.
Ctrl+backtickor alternatively (macOS) Control+Option+N.Testing Instructions for Keyboard
Screenshots or screencast