-
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
Fix regression with Edit site Navigate regions #52940
Conversation
Size Change: +12 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
Flaky tests detected in d5000ca. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5657768125
|
Props to @carolinan for the idea on how to fix this ❤️ |
I took it for a quick spin and region hopping appears to work as intended for me. That said, the nuance of the original issue is not entirely clear to me, so I wasn't quite sure how to reproduce the issue in trunk. I'm afraid I'm also not yet as familiar with the more modern animation practices at play here, so I'm afraid I can't be of the best help. Perhaps it needs a broader ping. |
Thank you @jasmussen. To better illustrate the bug I attached an animated GIF to the issue. For the animation part, ping @jameskoster 🙏 and @WordPress/gutenberg-design |
Indeed, that's helpful, and this PR seems to fix it. Nevertheless the code might need a review I can't confidently provide. Perhaps @aristath can help? |
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.
Thanks for fixing this! Testing in the site editor confirms that with this change it's possible to loop through all navigable regions with Ctrl+Opt+N and Ctrl+Opt+P without getting stuck anywhere and animation from browse to edit more still works as expected.
How do we test "routing on mobile" ? |
This is working well on my end too. Nice catch and fix! Thanks for the explanations on how the landmark regions work and their meanings. The animations look correct to me, and match what I see on trunk.
@carolinan, I'm not sure either. @kevin940726 may know. My understanding was that as long as the |
I'm going to go ahead and merge this one so it can be added to RC3. Thanks everyone! |
* Make the navigabel region wrap the inert sidebar. * Adjust animation.
I just cherry-picked this PR to the update/second-round-6-3-rc3 branch to get it included in the next release: 1d8bb10 |
|
* Top toolbar: Fix issues with save button overlap, and plugin buttons (#53101) * Shorten the width of the top toolbar overlay and make doc title smaller. * Add a scrim and a margin to handle plugin buttons better. * Remove block tools back compat component schedule for deprecated in 6.3 (#53115) * Removes usage of BlockToolsBackCompat * Remove unwanted BlockTools from Nav sidebar * Footnotes/RichText: fix getRichTextValues for deeply nested blocks (#53034) * Defer to preceding handlers in command palette keyboard shortcut (#53001) * Image block: fix image size at wide and full width (#53184) * Fix regression with Edit site Navigate regions (#52940) * Make the navigabel region wrap the inert sidebar. * Adjust animation. * Fix not expanding pattern in page editor (#53169) --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Footnotes: fix published preview (#53072) * Footnotes: fix published preview * remove var dump * Fix php lint * PHP lint * Address feedback * Add e2e test * Footnotes: disable for synced patterns and prevent duplication for pages in site editor (#53003) * Initial commit: - Prevent footnote creation withing core/block - Only insert a footnote if one isn't found in the entity block list * Try grabbing controlled blocks from parent post content block * Cache `selectedClientId` Get hasParentCoreBlocks using separate useSelect call. * Rename hasParentCoreBlocks to isBlockWithinPattern Add comments * Removing while loop since we're already fetching the post content parent in the `getBlockParentsByBlockName` call above * Reinstating while loop because it can deal with nested blocks --------- Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> * Footnotes: add missing _ in revision field filter (#53135) * Footnotes: add missing _ in revision field filter * Use correct hook name * Revert prefixing callback names * don't display BlockContextualToolbar at all in contentonly locking (#53110) * Render the footer conditionally in the global styles sidebar component so that any side effects from the footer wrapper are not rendered, e.g., styles and what not (#53204) Ensure that the precise bottom margin persists if the footer isn't rendered * Pattern: Add getBlockRootClientId call (#53206) --------- Co-authored-by: Joen A <1204802+jasmussen@users.noreply.github.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
Fixes #52920
What?
Fixes a regression with the ravigate regions feature (
useNavigateRegions
) after #51956.Please. note this is a regression compared to WP 6.2 and should be fixed for the WP 6.3 release. Cc @annezasu and @youknowriad for some kind help with triaging and reviewing.
Why?
Users can't navigate the main regions of the editor via keyboard shortcuts any longer, At some point jumping through the regions gets stuck on the navigation panel because it now uses
inert
after #51956.How?
All
NavigableRegion
must always be rendered in the DOM and keyboard interaction on them must not be prevented via keyboard.As such, I changed the order of the wrapping elements. Now, the NavigableRegion wraps the animated sidebar that uses
inert
. This way,useNavigateRegions
still works as expected. The animation should still work as expected.This PR contains the minimum amount of changes to fix the regression.
Still to do in follow-ups:
Testing Instructions
Use thes keyboard shortcut for testing:
Navigate to the next part of the editor: Control + backtick or alternatively Control + Option + N
Navigate to the previous part of the editor: Shift + Control + backtick or alternatively Control + Option + P
Important note: the animation when switching the editor from View mode to Edit mode should be tested in all browsers (especially Safari). The switching behavior should be tested also in the small screens view. I'd greatly appreciate some eyes to check the animation from someone who;s more familiar with this. Cc @jasmussen
Testing Instructions for Keyboard
Screenshots or screencast