Focus currently selected block when entering canvas #61472
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
…he canvas Now that the inserter stays open when focus leaves it, an edge case can occur where blocks are inserted before the canvas has ever been focused. When that happens, focus is trying to get sent to the previously focused block, but no block has been focused. This focuses the currently selected block when entering the canvas.
|
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. |
jeryj
added
the
[Feature] Writing Flow
jeryj
commented
May 8, 2024
Comment on lines
48
to
52
| container.current | ||
| .querySelector( | ||
| `[data-block="${ getSelectedBlockClientId() }"]` | ||
| ) | ||
| .focus(); |
There was a problem hiding this comment.
@ellatrix suggested this, so it's good :)
There was a problem hiding this comment.
We should probably add a non-hook util at some point similar to useBlockRef
|
Size Change: +2 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
afercia
added
the
[Focus] Accessibility (a11y)
|
i would appreciate that any proposed change that touches important things for accessibility had the 'accessibility' label set on the related PR and / or issue. Thanks. That said, I'd kindly ask everyone to wais for this PR to be merged until it gets tested by contributors more familiar with keyboard and screen reader users needs. Cc @alexstine I'd appreciate your thoughts, when you have a chance. Edit: OK I think I understand it a bit better now. This is to make sure the inserted block (which is the 'selected' block) is focused when tentering the canvas. To my understanding this i sthe same behavior that exists now in WordPress 6.5. However, there is a relevant difference that comes from the changed beheavior of the inserter. In WordPress 6.5:
On trunk right now:
I'd like to see the new behavior mor tested by keyboard users and screen reader users. |
When inserting a new block from the inserter, focus should go to this block when focus is moved to the canvas via the Tab keypress. To do this, we need to clear where focus previously was when a new block is inserted, otherwise it tries to place focus where it was before.
jeryj
commented
May 9, 2024
| shouldFocusBlock || | ||
| selectBlockOnInsert | ||
| ) { | ||
| setLastFocus( null ); |
There was a problem hiding this comment.
I considered doing this within the inserter sidebar menu, but I think it should live here as it will be an issue for anyone trying to insert new blocks from outside the editor canvas. This allows people to choose if they want focus to return to the canvas where it was (preserve last focus) or clear it and focus the new block.
jeryj
commented
May 9, 2024
Comment on lines
+48
to
+50
| if ( getLastFocus()?.current ) { | ||
| getLastFocus().current.focus(); | ||
| } else { |
There was a problem hiding this comment.
I had to preserve this, as focusing the selected block broke focusing inner blocks or things like focusing the table grid cell text field within a table block.
|
@jeryj I'm not sure how this was working before because someone got in a bit of a merge hurry but the experience is still quite bad.
Tested in Firefox. Not sure if any of these bugs are related to this PR but this is really worrisome. One of the most used features of the editor, block inserter, is currently mostly unuseable for keyboard users. Could we please refrain from merging to trunk before it can be checked for accessibility? I have no doubt this was probably an improvement over the experience but as mentioned above, these bugs are starting to come out of nowhere and it's really hard to identify which PR does it when there's never enough time for testing. Thanks. |
|
Also, another bug I found, it seems that when the inserter is open, pressing Escape in the canvas no longer activates navigation mode. 😞 |
|
Thanks for the testing @alexstine. I've got a PR for one and am looking at the other. |
|
Switching modes should be fixed by #61563. |
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.
Now that the inserter stays open when focus leaves it, an edge case can occur where blocks are inserted before the canvas has ever been focused. When that happens, focus is trying to get sent to the previously focused block, but no block has been focused.
This focuses the currently selected block when entering the canvas.
Testing Instructions