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 all 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
10 changes: 6 additions & 4 deletions packages/block-editor/src/components/block-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { useSelect, useDispatch } from '@wordpress/data';
import { useRef } from '@wordpress/element';
import { useViewportMatch } from '@wordpress/compose';
import { getBlockType, hasBlockSupport } from '@wordpress/blocks';
Expand All @@ -21,7 +21,7 @@ import BlockControls from '../block-controls';
import BlockFormatControls from '../block-format-controls';
import BlockSettingsMenu from '../block-settings-menu';
import BlockDraggable from '../block-draggable';
import { useShowMoversGestures, useToggleBlockHighlight } from './utils';
import { useShowMoversGestures } from './utils';
import ExpandedBlockControlsContainer from './expanded-block-controls-container';

export default function BlockToolbar( {
Expand Down Expand Up @@ -65,13 +65,15 @@ export default function BlockToolbar( {
};
}, [] );

const toggleBlockHighlight = useToggleBlockHighlight( blockClientId );
const { toggleBlockHighlight } = useDispatch( 'core/block-editor' );
const nodeRef = useRef();

const { showMovers, gestures: showMoversGestures } = useShowMoversGestures(
{
ref: nodeRef,
onChange: toggleBlockHighlight,
onChange( isFocused ) {
toggleBlockHighlight( blockClientId, isFocused );
},
}
);

Expand Down
52 changes: 5 additions & 47 deletions packages/block-editor/src/components/block-toolbar/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,15 @@
* External dependencies
*/
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, setTimeout } = window;

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

/**
* Hook that creates a showMover state, as well as debounced show/hide callbacks.
Expand Down Expand Up @@ -168,41 +164,3 @@ export function useShowMoversGestures( {
},
};
}

let requestAnimationFrameId;

/**
* Hook that toggles the highlight (outline) state of a block
*
* @param {string} clientId The block's clientId
*
* @return {Function} Callback function to toggle highlight state.
*/
export function useToggleBlockHighlight( clientId ) {
const { toggleBlockHighlight } = useDispatch( 'core/block-editor' );

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

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 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
21 changes: 14 additions & 7 deletions packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1630,14 +1630,21 @@ export function automaticChangeStatus( state, action ) {
* @return {string} Updated state.
*/
export function highlightedBlock( state, action ) {
const { clientId, isHighlighted } = action;
switch ( action.type ) {
case 'TOGGLE_BLOCK_HIGHLIGHT':
const { clientId, isHighlighted } = action;

if ( action.type === 'TOGGLE_BLOCK_HIGHLIGHT' ) {
if ( isHighlighted ) {
return clientId;
} else if ( state === clientId ) {
return null;
}
if ( isHighlighted ) {
return clientId;
} else if ( state === clientId ) {
return null;
}

return state;
case 'SELECT_BLOCK':
if ( action.clientId !== state ) {
return null;
}
}

return state;
Expand Down