Writing flow: fix selecting synced pattern #55221
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
ellatrix
added
[Type] Bug
|
Size Change: +1 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 59b6e7a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6473752803
|
|
Please see: #54369 |
andrewserong
approved these changes
Oct 11, 2023
There was a problem hiding this comment.
What an interesting problem!
In manual testing, this does appear to resolve the issue of the Up arrow being stuck when moving from a root level block and then up to a synced pattern or template part.
One potential issue could be to do with consistency, though, in that in terms of the block tree, instead of moving to the previous adjacent block, we now step up a level to the root of that synced block or template part, whereas when we press the Down arrow key, we traverse the tree for that parent block. Here's a screengrab:
2023-10-11.11.44.09.mp4
In the above video, the following happens:
- Press Up arrow from a Group block, and go to the root of the Footer template part
- Press Down arrow from there, and you traverse down through each vertical block of the hierarchy until you reach the bottom of the Footer, and then progress to the adjacent Group block
When there are adjacent Group blocks, the Up arrow will go through the deepest part of the adjacent Group block, rather than jump to the root. Would that be closer to the desired behaviour, or is it preferred to jump to the root?
All that said, the behaviour in this PR feels much better to me than on trunk where the user is stuck when navigating via keyboard, so this LGTM as an improvement over what we have currently!
|
@alexstine apologies I missed your comment before I left a review on this PR. Does this PR seem like an okay short-term fix, or would it be better to hold off until there's a better fix for the use of |
|
@andrewserong I would much rather us fix the problem as this severely impacts blind users. I tried to warn about a cost here but seems people are only really taking this seriously off a visual UX report. Happy to land this as long as someone will be committed to solving the larger issue at play. EDIT: Not directed at PR contributors, just general frustration. |
|
Thanks for clarifying, and for the extra context, Alex! I've added a comment over in #54369 to ensure the discussion here is captured over there as well, and to flag that the issue still needs a solution. |
|
@andrewserong The behaviour is in line with the "click to select more" thing. First you have to select the block and then you can navigate down. It's just that arrow up will not more further down the tree but rather to the next block whereas arrow down will. I think that's expected. This PR does not further cement in the inert prop, I just added an exception to writing flow so things don't break. Seems like a fine thing to add generally, also if a non core block decides to use the inert prop for some reason. |
|
Gotcha, thanks for explaining @ellatrix and for putting together the fix! |
|
The new e2e test looks to be flaky. Failure was reported 40+ times. See #55284. |
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.
What?
Fixes #55177.
Arrowing into a synced pattern is broken. This is because of the use of the
inertattribute, preventing the browser from setting focus. The solution is skip element inside an inert element and try a parent element.Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast