Skip to content

Commit

Permalink
Don't move focus within the toolbar if it is already focused (#58570)
Browse files Browse the repository at this point in the history
If the toolbar has focus already, we don't want to run a requestanimationFrame to then move focus to the previously unmounted index. Adding a check to see if focus is within the toolbar already, then not running the RAF fixes an issue where moving the block via the mover arrows would steal focus to the index before mount.

Also added a fix for getAllFocusableToolbarItemsIn to not exclude aria-disabled buttons, as those can still receive focus.
  • Loading branch information
jeryj authored Feb 5, 2024
1 parent ae193eb commit a432f66
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 5 deletions.
12 changes: 8 additions & 4 deletions packages/block-editor/src/components/navigable-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ function hasOnlyToolbarItem( elements ) {

function getAllFocusableToolbarItemsIn( container ) {
return Array.from(
container.querySelectorAll(
'[data-toolbar-item]:not([disabled]):not([aria-disabled="true"])'
)
container.querySelectorAll( '[data-toolbar-item]:not([disabled])' )
);
}

Expand Down Expand Up @@ -144,7 +142,13 @@ function useToolbarFocus( {
// We have to wait for the next browser paint because block controls aren't
// rendered right away when the toolbar gets mounted.
let raf = 0;
if ( ! initialFocusOnMount ) {

// If the toolbar already had focus before the render, we don't want to move it.
// https://github.com/WordPress/gutenberg/issues/58511
if (
! initialFocusOnMount &&
! hasFocusWithin( navigableToolbarRef )
) {
raf = window.requestAnimationFrame( () => {
const items =
getAllFocusableToolbarItemsIn( navigableToolbarRef );
Expand Down
63 changes: 62 additions & 1 deletion test/e2e/specs/editor/various/navigable-toolbar.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ test.describe( 'Block Toolbar', () => {
exact: true,
} );

// Yes, this is the way to get the block toolbar, and yes, it is annoying.
const blockToolbar = page.getByRole( 'toolbar', {
name: 'Block tools',
} );
Expand Down Expand Up @@ -240,6 +239,68 @@ test.describe( 'Block Toolbar', () => {
await editor.setIsFixedToolbar( false );
await pageUtils.setBrowserViewport( 'large' );
} );

test( 'Focus should remain on mover when moving blocks', async ( {
editor,
page,
pageUtils,
} ) => {
// On default floating toolbar
await editor.insertBlock( {
name: 'core/paragraph',
attributes: { content: 'Paragraph 1' },
} );
await editor.insertBlock( {
name: 'core/paragraph',
attributes: { content: 'Paragraph 2' },
} );
await editor.insertBlock( {
name: 'core/paragraph',
attributes: { content: 'Paragraph 3' },
} );
await pageUtils.pressKeys( 'shift+Tab' );
// check focus is within the block toolbar
const blockToolbarParagraphButton = page.getByRole( 'button', {
name: 'Paragraph',
exact: true,
} );
await expect( blockToolbarParagraphButton ).toBeFocused();
// Go to Move Up Button
await pageUtils.pressKeys( 'ArrowRight' );
const blockToolbarMoveUpButton = page.getByRole( 'button', {
name: 'Move up',
exact: true,
} );

// Make sure it's in an acvite state for now
await expect( blockToolbarMoveUpButton ).not.toHaveAttribute(
'aria-disabled',
'true'
);

await expect( blockToolbarMoveUpButton ).toBeFocused();
await pageUtils.pressKeys( 'Enter' );
await expect( blockToolbarMoveUpButton ).toBeFocused();
await pageUtils.pressKeys( 'Enter' );
await expect( blockToolbarMoveUpButton ).toBeFocused();
await expect( blockToolbarMoveUpButton ).toHaveAttribute(
'aria-disabled',
'true'
);

// Check to make sure focus returns to the Move Up button roving index after all of this
await pageUtils.pressKeys( 'Tab' );
// Hide the block toolbar
await pageUtils.pressKeys( 'ArrowRight' );
// Check the block toolbar is hidden
const blockToolbar = page.getByRole( 'toolbar', {
name: 'Block tools',
} );
await expect( blockToolbar ).toBeHidden();
await pageUtils.pressKeys( 'shift+Tab' );
// We should be on the Move Up button again
await expect( blockToolbarMoveUpButton ).toBeFocused();
} );
} );

class BlockToolbarUtils {
Expand Down

0 comments on commit a432f66

Please sign in to comment.