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.

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

Editor: Fix block highlight rendering after block is moved #23425

Merged
merged 13 commits into from
Jul 20, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
39 changes: 9 additions & 30 deletions packages/block-editor/src/components/block-toolbar/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,11 @@ import { noop } from 'lodash';
* WordPress dependencies
*/
import { useDispatch } from '@wordpress/data';
import { useState, useRef, useEffect, useCallback } from '@wordpress/element';
import { useState, useRef, useEffect } from '@wordpress/element';

const {
clearTimeout,
requestAnimationFrame,
cancelAnimationFrame,
setTimeout,
} = window;
const DEBOUNCE_TIMEOUT = 250;
const { clearTimeout, setTimeout } = window;

const DEBOUNCE_TIMEOUT = 200;

/**
* Hook that creates a showMover state, as well as debounced show/hide callbacks.
Expand Down Expand Up @@ -169,8 +165,6 @@ export function useShowMoversGestures( {
};
}

let requestAnimationFrameId;

/**
* Hook that toggles the highlight (outline) state of a block
*
Expand All @@ -181,28 +175,13 @@ let requestAnimationFrameId;
export function useToggleBlockHighlight( clientId ) {
const { toggleBlockHighlight } = useDispatch( 'core/block-editor' );

const updateBlockHighlight = useCallback(
( isFocused ) => {
toggleBlockHighlight( clientId, isFocused );
},
[ clientId ]
);
const updateBlockHighlight = ( isFocused ) => {
toggleBlockHighlight( clientId, isFocused );
};

useEffect( () => {
// On mount, we make sure to cancel any pending animation frame request
// that hasn't been completed yet. Components like NavigableToolbar may
// mount and unmount quickly.
if ( requestAnimationFrameId ) {
cancelAnimationFrame( requestAnimationFrameId );
}
return () => {
// Sequences state change to enable editor updates (e.g. cursor
// position) to render correctly.
requestAnimationFrameId = requestAnimationFrame( () => {
updateBlockHighlight( false );
} );
};
}, [] );
return () => toggleBlockHighlight( clientId, false );
Copy link
Member

Choose a reason for hiding this comment

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

Why are we toggling block highlight in an effect rather than at the reducer level? For a SELECT_BLOCK action etc., the highlight state should change immediately. Cc @youknowriad.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that if we can do it automatically on the reducer, it's better and avoid an extra rerenders for all components.

}, [ clientId ] );

return updateBlockHighlight;
}
18 changes: 17 additions & 1 deletion packages/block-editor/src/components/navigable-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,27 @@ function focusFirstTabbableIn( container ) {
}

function useIsAccessibleToolbar( ref ) {
/*
* By default, we'll assume the starting accessible state of the Toolbar
* is true, as it seems to be the most common case.
*
* Transitioning from an (initial) false to true state causes the
* <Toolbar /> component to mount twice, which is causing undesired
Copy link
Member

Choose a reason for hiding this comment

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

Mount or render?

Copy link
Contributor

Choose a reason for hiding this comment

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

mount :)

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is still needed after the adjustments I made?

Copy link
Author

Choose a reason for hiding this comment

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

I think so. From our debugging, the Toolbar was double mounting regardless of the toggleBlockHighlight dispatch action.

* side-effects. These side-effects appear to only affect certain
* E2E tests.
*
* This was initial discovered in this pull-request:
* https://github.com/WordPress/gutenberg/pull/23425
*/
const initialAccessibleToolbarState = true;

// By default, it's gonna render NavigableMenu. If all the tabbable elements
// inside the toolbar are ToolbarItem components (or derived components like
// ToolbarButton), then we can wrap them with the accessible Toolbar
// component.
const [ isAccessibleToolbar, setIsAccessibleToolbar ] = useState( false );
const [ isAccessibleToolbar, setIsAccessibleToolbar ] = useState(
initialAccessibleToolbarState
);

const determineIsAccessibleToolbar = useCallback( () => {
const tabbables = focus.tabbable.find( ref.current );
Expand Down