Skip to content

Commit

Permalink
Do not mount block toolbar popover until needed.
Browse files Browse the repository at this point in the history
Once the block toolbar popover is mounted, it will remain mounted until the block id changes. This is a lot of extra work to delay this mounting. It could be a performance improvement in one sense, but the top toolbar and mobile toolbars get remounted each time a block changes, so I lean towards removing all this work and keeping the code simpler.
  • Loading branch information
jeryj committed Dec 8, 2023
1 parent 3bc597b commit 06b687d
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,28 @@
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { useDispatch } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { useShortcut } from '@wordpress/keyboard-shortcuts';
/**
* Internal dependencies
*/
import BlockPopover from '../block-popover';
import { PrivateBlockToolbar } from '../block-toolbar';
import useBlockToolbarPopoverProps from './use-block-toolbar-popover-props';
import useSelectedBlockToolProps from './use-selected-block-tool-props';
import BlockToolbar from '../block-toolbar';
import { store as blockEditorStore } from '../../store';

export default function BlockToolbarPopover( {
clientId,
isTyping,
__unstableContentRef,
} ) {
const { stopTyping } = useDispatch( blockEditorStore );
const { capturingClientId, isInsertionPointVisible, lastClientId } =
useSelectedBlockToolProps( clientId );

Expand All @@ -23,18 +32,64 @@ export default function BlockToolbarPopover( {
clientId,
} );

/**
* This is necessary to prevent the toolbar from being in the DOM until a user has stopped typing.
* Ideally this is all unnecessary if we can get the toolbar to not impact typing performance, as it
* should not. However, once it's mounted, we don't want to remount it again until the block changes.
* This is both a bit of a hack and a performance improvement.
*/
// Store a ref to always return true once the user has stopped typing. We don't want to remount the toolbar until the clientID changes to avoid doing unnecessary remounting work.
const hasStoppedTyping = useRef( false );

useEffect( () => {
if ( hasStoppedTyping.current ) {
return;
}

if ( ! hasStoppedTyping.current && ! isTyping ) {
hasStoppedTyping.current = true;
}
}, [ isTyping ] );

// The shortcut doing the work lives in the NavigableToolbar component.
// It doesn't exist until it's mounted, so we need to dispatch `stopTyping`
// when the shortcut is pressed to cause a rerender, which mounts the
// NavigableToolbar and sends focus to the toolbar.
const isToolbarForced = useRef( false );
useShortcut( 'core/block-editor/focus-toolbar', () => {
hasStoppedTyping.current = true;
isToolbarForced.current = true;
stopTyping();
} );

// Force the component to unmount when the clientID changes.
useEffect( () => {
hasStoppedTyping.current = false;
isToolbarForced.current = false;
}, [ clientId ] );

// isTyping will cause a rerender since it's a state change.
// After that, we can rely on hasStoppedTyping to keep the toolbar in the DOM until the clientID changes.
return (
<BlockPopover
clientId={ capturingClientId || clientId }
bottomClientId={ lastClientId }
className={ classnames( 'block-editor-block-list__block-popover', {
'is-insertion-point-visible': isInsertionPointVisible,
'is-hidden': isTyping, // Leave the toolbar in the DOM so it can be focused at the same roving tabindex it was previously at
} ) }
resize={ false }
{ ...popoverProps }
>
<BlockToolbar variant="toolbar" />
</BlockPopover>
( ! isTyping || hasStoppedTyping.current ) && (
<BlockPopover
clientId={ capturingClientId || clientId }
bottomClientId={ lastClientId }
className={ classnames(
'block-editor-block-list__block-popover',
{
'is-insertion-point-visible': isInsertionPointVisible,
'is-hidden': isTyping, // Leave the toolbar in the DOM so it can be focused at the same roving tabindex it was previously at
}
) }
resize={ false }
{ ...popoverProps }
>
<PrivateBlockToolbar
focusOnMount={ isToolbarForced.current }
variant="toolbar"
/>
</BlockPopover>
)
);
}
29 changes: 26 additions & 3 deletions packages/block-editor/src/components/navigable-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
useEffect,
useCallback,
} from '@wordpress/element';
import { useDispatch, useSelect } from '@wordpress/data';
import { useSelect } from '@wordpress/data';
import deprecated from '@wordpress/deprecated';
import { focus } from '@wordpress/dom';
import { useShortcut } from '@wordpress/keyboard-shortcuts';
Expand All @@ -25,6 +25,14 @@ function hasOnlyToolbarItem( elements ) {
return ! elements.some( ( element ) => ! ( dataProp in element.dataset ) );
}

function getAllToolbarItemsIn( container ) {
return Array.from( container.querySelectorAll( '[data-toolbar-item]' ) );
}

function hasFocusWithin( container ) {
return container.contains( container.ownerDocument.activeElement );
}

function focusFirstTabbableIn( container ) {
const [ firstTabbable ] = focus.tabbable.find( container );

Expand Down Expand Up @@ -100,15 +108,13 @@ function useToolbarFocus( {
} ) {
// focusOnMount deprecated in 6.5.0
const [ initialFocusOnMount ] = useState( focusOnMount );
const { stopTyping } = useDispatch( blockEditorStore );

const focusToolbar = useCallback( () => {
focusFirstTabbableIn( toolbarRef.current );
}, [ toolbarRef ] );

const focusToolbarViaShortcut = () => {
if ( shouldUseKeyboardFocusShortcut ) {
stopTyping( true ); // This matches the behavior of the Tab/Escape observe typing to stopTyping. Should we add this shortcut there?
focusToolbar();
}
};
Expand All @@ -123,6 +129,23 @@ function useToolbarFocus( {
}
}, [ isAccessibleToolbar, initialFocusOnMount, focusToolbar ] );

// Focus the first toolbar item when the toolbar is mounted and attempting to be focused (such as via alt+f10 shortcut).
useEffect( () => {
// We have to wait for the next browser paint because block controls aren't
// rendered right away when the toolbar gets mounted.
window.requestAnimationFrame( () => {
const items = getAllToolbarItemsIn( toolbarRef.current );
if ( items[ 0 ] && hasFocusWithin( toolbarRef.current ) ) {
items[ 0 ].focus( {
// When focusing newly mounted toolbars,
// the position of the popover is often not right on the first render
// This prevents the layout shifts when focusing the dialogs.
preventScroll: true,
} );
}
} );
}, [ toolbarRef ] );

const { lastFocus } = useSelect( ( select ) => {
const { getLastFocus } = select( blockEditorStore );
return {
Expand Down

0 comments on commit 06b687d

Please sign in to comment.