Escape on Block Toolbar returns focus to Editor Canvas #55712
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
jeryj
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
jeryj
added
the
[Focus] Accessibility (a11y)
|
Size Change: +264 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
jeryj
changed the title
|
Flaky tests detected in 18f4f14. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6698846222
|
scruffian
approved these changes
Nov 2, 2023
There was a problem hiding this comment.
This is looking good. I am seeing a unconnected issue where when I have a cursor inside a block (like a paragraph or a heading) alt+F10 isn't taking me to the toolbar, but this also happens in trunk. It might just be that I'm not very good at keyboard navigation!
Stores the element that had last focus from the editor when focus leaves the editor canvas
Adding the escape keypress from the navigable block tools will require forwarding the ref from the BlockTools all the way down to the NavigableToolbar.
This method requires forwarding a ref from the editor level down to the navigable toolbar so that the escape unselect shortcut can be blocked and the navigable toolbar event listener will still fire. Blocking the global escape event shouldn't be necessary, but we have a combination of a few things all combining to create a situation where: - Because the block toolbar uses createPortal to populate the block toolbar fills, we can't rely on the React event bubbling to hit the onKeyDown listener for the block toolbar - Since we can't use the React tree, we use the DOM tree which _should_ handle the event bubbling correctly from a `createPortal` element. - This bubbles via the React tree, which hits this `unselect` escape keypress before the block toolbar DOM event listener has access to it. Also, this is better than attaching it to all of the children in the block toolbar because when new items are attached to the toolbar (such as via the bubblesVirtually slots or the apply/cancel buttons when cropping an image) we can still catch the events. Otherwise, those buttons are added after the mount, and the children don't receive the listener.
I added it since it was passed in, but it is a callback and doesn't change. It was causing unnecssary rerenders that was messing with the focus position when selecting an item from a dropdown, such as when changing the heading level on a site title block.
jeryj
force-pushed
the
add/escape-from-toolbar
branch
from
dbfb084
to
ad87967
Compare
|
Since there is already agreement on this approach I think we should bring it in. |
scruffian
force-pushed
the
add/escape-from-toolbar
branch
from
08ec8dd
to
8558e69
Compare
draganescu
reviewed
Nov 2, 2023
| @@ -31,7 +32,10 @@ import BlockToolbar from '../block-toolbar'; | |||
| import { store as blockEditorStore } from '../../store'; | |||
| import { useHasAnyBlockControls } from '../block-controls/use-has-block-controls'; | |||
|
|
|||
| function BlockContextualToolbar( { focusOnMount, isFixed, ...props } ) { | |||
| function UnforwardBlockContextualToolbar( | |||
There was a problem hiding this comment.
This name confuses me UnforwardBlockContextualToolbar. Whati Unforward for? Maybe ... Unforwarded?
| return null; | ||
| } | ||
|
|
||
| const SelectedBlockPopover = forwardRef( UnforwardSelectedBlockPopover ); |
There was a problem hiding this comment.
Same comment, I think adding an -ed suffix makes it better :)
kevin940726
reviewed
Jan 2, 2024
| export function lastFocus( state = false, action ) { | ||
| switch ( action.type ) { | ||
| case 'LAST_FOCUS': | ||
| return action.lastFocus; |
There was a problem hiding this comment.
Storing unserializable values in Redux's store breaks the compatibility of Redux Devtools. It will just output t.toString() is not a function whenever it tries to serialize it and provide no helpful trace 😅.
What's worse is that it could just crash the page unexpectedly, without the Redux Devtools even being active (it just has to be initialized).
Related: #57198 (comment) and #57257 (comment).
There was a problem hiding this comment.
Thank you for tracking this down! Got an approach for review here: #57557
youknowriad
reviewed
Jan 3, 2024
| @@ -588,6 +588,18 @@ _Properties_ | |||
| - _isDisabled_ `boolean`: Whether or not the user should be prevented from inserting this item. | |||
| - _frecency_ `number`: Heuristic that combines frequency and recency. | |||
|
|
|||
| ### getLastFocus | |||
There was a problem hiding this comment.
Can we avoid this being a public API please. (and the action as well)
This was referenced Jan 4, 2024
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?
Adds using escape keypress to return from toolbar to editor canvas. Right now the escape keypress on the toolbar causes the blocks to be deselected and loses focus. Escape from a floating element is a very common pattern for returning focus to the canvas -- moreso than
tab.Why?
This is part of the Top Toolbar improvement work. This adds a real feature to Gutenberg that can be expanded upon, and will make it easier to merge #54513 instead of reviewing 70 commits...
How?
Storing
lastFocuselement when focus leaves the block toolbarTo help make managing focus easier across Gutenberg, anytime focus leaves the editor canvas, the element that previously had focus will be stored. This will help fix focus loss as well, as this PR uses the same methods: #54443.
Catching Escape Keypress from Block Toolbar
@ajlende and I investigated a lot of ways of catching the escape keypress from the navigable toolbar, and landed with three viable options. The selected method requires forwarding a ref from the editor level down to the navigable toolbar so that the escape unselect shortcut can be blocked and the navigable toolbar event listener will still fire.
Blocking the global escape event shouldn't be necessary, but we have a combination of a few things all combining to create a situation where:
createPortalto populate the block toolbar fills, we can't rely on the React event bubbling to hit theonKeyDownlistener for the block toolbarcreatePortalelement.unselectescape keypress before the block toolbar DOM event listener has access to it.Also, this is better than attaching it to all of the children in the block toolbar because when new items are dynamically rendered into the toolbar (such as via the
bubblesVirtuallyslots or the apply/cancel buttons when cropping an image) we can still catch the events. Otherwise, those buttons are added after the mount, and the children don't receive the listener.Testing Instructions for Keyboard
shift + tabto move focus to the block toolbarescapeto return focus to the block