Buttons block: fix focus on insert #39684
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
[Feature] Writing Flow
|
Size Change: +10 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
oandregal
reviewed
Mar 23, 2022
|
|
||
| useEffect( () => { | ||
| // Check if the block is still selected at the time this effect runs. |
There was a problem hiding this comment.
In trunk, this code fails to get the focus to the inner block when templateInsertUpdatesSelection is true because:
- The wrapper is not selected but the
initialPositionis 0, hence this hooks gives it the focus. - At the point the inner block hook runs it's no longer selected and the
initialPositionis nullish, so it doesn't gain focus. The reason it's no longer selected (and initialPosition nullish) is because theuseFocusHandlerhas already detected that selection & focus are not in sync and has dispatched theselect_blockaction to update selection to the wrapper.
In trunk, this code works as expected when templateInsertUpdatesSelection is false because:
- The wrapper is selected and
initialPositionis 0, hence it gets the focus. - The inner block is not selected and
initialPositionis nullish, so the hook does nothing.
There was a problem hiding this comment.
In trunk, or you mean in this branch?
There was a problem hiding this comment.
I meant in trunk, just trying to document the before/after behavior :)
A more precise description of why this fails in trunk when templateInsertUpdatesSelection is true is this:
- The replace_blocks action is dispatched and the
useFocusFirstElementruns for the wrapper. At this point, the wrapper is selected and soinitialPositionis 0. The effect is hooked for React to execute it. - Before React executes the effect, the replace_inner_blocks action is dispatched making the inner block selected. At this point the
initialPositioncomputation for the wrapper wouldn't be 0. However, the effect for it is already queued and we don't have a way to update that stale info. - The effect for the wrapper runs with stale data (
initialPositionto 0) and focuses the wrapper. - The
useFocusHandlerhook detects the wrapper is focused and updates the selection to be in the wrapper again (select_block action). - The
useFocusFirstElementhook runs for the inner block, but theinitialPositionis undefined. When the effect is executed, it does nothing.
So this fix prevents 4 from focusing the wrapper and 5 from happening. Hence, at 6, the initialPosition for the inner block is 0 and will focus it.
There was a problem hiding this comment.
I've looked at useInitialPosition for other potential issues. It seems the rest are more difficult to get into a race condition like block selection is because they're user actions (happen at lower cycles than redux/react cycles). Though, in theory, it could happen again: what would you think of removing useInitialPosition hook entirely and inline its contents within the effect itself for clarity?
oandregal
approved these changes
Mar 23, 2022
There was a problem hiding this comment.
This fixes the issue and the code makes sense.
I've done a bit of testing:
- with blocks that use
templateInsertUpdatesSelectionto false (columns, navigation, media &text, etc) - with blocks that use
templateInsertUpdatesSelectionto true (buttons & cover)
The only behavior change I've noticed is that now the inner button will get the focus instead of the buttons wrapper, as expected.
I've also tested by setting templateInsertUpdatesSelection to false for the buttons block (and it works as expected in that case as well: focus remains in the wrapper) to make sure that blocks without a placeholder state aren't affected.
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?
The button block uses
templateInsertUpdatesSelection, but it currently doesn't move focus to the first button block. This is because there are two actions dispatched. First,REPLACE_BLOCKSinserts the wrapper block (buttons) and selects that block. Then after a small delay,REPLACE_INNER_BLOCKSis dispatched to insert the button block and select that block. But right before that,useFocusFirstElementis called and places focus on the wrapper (to reflect what was in the store previously). Because what gets focussed (wrapper) and what is selected in the store (button),SELECT_BLOCKis now dispatched to update the selection to reflect what is focussed.The problem here is that
useFocusFirstElementis also called with a delay (useEffect) while the information it got from the store may no longer be fresh. Double checking if the correct block is still selected fixes the issue. If by the time theuseFocusFirstElementhook runs and according to the store the child block (button) is now selected, it must no longer focus the wrapper.Why?
How?
Testing Instructions
Screenshots or screencast