Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Buttons block: fix focus on insert #39684

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Buttons block: fix focus on insert #39684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,14 @@ function isFormElement( element ) {
export function useFocusFirstElement( clientId ) {
const ref = useRef();
const initialPosition = useInitialPosition( clientId );
const { isBlockSelected } = useSelect( blockEditorStore );

useEffect( () => {
// Check if the block is still selected at the time this effect runs.
Copy link
Member

@oandregal oandregal Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 initialPosition is 0, hence this hooks gives it the focus.
  • At the point the inner block hook runs it's no longer selected and the initialPosition is nullish, so it doesn't gain focus. The reason it's no longer selected (and initialPosition nullish) is because the useFocusHandler has already detected that selection & focus are not in sync and has dispatched the select_block action to update selection to the wrapper.

In trunk, this code works as expected when templateInsertUpdatesSelection is false because:

  • The wrapper is selected and initialPosition is 0, hence it gets the focus.
  • The inner block is not selected and initialPosition is nullish, so the hook does nothing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In trunk, or you mean in this branch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The replace_blocks action is dispatched and the useFocusFirstElement runs for the wrapper. At this point, the wrapper is selected and so initialPosition is 0. The effect is hooked for React to execute it.
  2. Before React executes the effect, the replace_inner_blocks action is dispatched making the inner block selected. At this point the initialPosition computation 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.
  3. The effect for the wrapper runs with stale data (initialPosition to 0) and focuses the wrapper.
  4. The useFocusHandler hook detects the wrapper is focused and updates the selection to be in the wrapper again (select_block action).
  5. The useFocusFirstElement hook runs for the inner block, but the initialPosition is 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if ( ! isBlockSelected( clientId ) ) {
return;
}

if ( initialPosition === undefined || initialPosition === null ) {
return;
}
Expand Down Expand Up @@ -127,7 +133,7 @@ export function useFocusFirstElement( clientId ) {
setContentEditableWrapper( ref.current, false );

placeCaretAtHorizontalEdge( target, isReverse );
}, [ initialPosition ] );
}, [ initialPosition, clientId ] );

return ref;
}
8 changes: 8 additions & 0 deletions packages/e2e-tests/specs/editor/blocks/__snapshots__/buttons.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ exports[`Buttons dismisses link editor when escape is pressed 1`] = `
<!-- /wp:buttons -->"
`;

exports[`Buttons has focus on button content (slash inserter) 1`] = `
"<!-- wp:buttons -->
<div class=\\"wp-block-buttons\\"><!-- wp:button -->
<div class=\\"wp-block-button\\"><a class=\\"wp-block-button__link\\">Content</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->"
`;

exports[`Buttons has focus on button content 1`] = `
"<!-- wp:buttons -->
<div class=\\"wp-block-buttons\\"><!-- wp:button -->
Expand Down
10 changes: 10 additions & 0 deletions packages/e2e-tests/specs/editor/blocks/buttons.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
getEditedPostContent,
createNewPost,
pressKeyWithModifier,
clickBlockAppender,
} from '@wordpress/e2e-test-utils';

describe( 'Buttons', () => {
Expand All @@ -20,6 +21,15 @@ describe( 'Buttons', () => {
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'has focus on button content (slash inserter)', async () => {
await clickBlockAppender();
await page.keyboard.type( '/buttons' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Content' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'dismisses link editor when escape is pressed', async () => {
// Regression: https://github.com/WordPress/gutenberg/pull/19885
await insertBlock( 'Buttons' );
Expand Down