Block Toolbar: update position when moving blocks (#44301)

* Block Toolbar: update position when moving blocks * Make sure counter does not overflow * Add explainer for "< 0" check * Apply same enhancements to inbetween inserter
WordPress · Sep 21, 2022 · ed8d25e · ed8d25e
1 parent afd40a5
commit ed8d25e
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 12 deletions.
diff --git a/packages/block-editor/src/components/block-popover/inbetween.js b/packages/block-editor/src/components/block-popover/inbetween.js
@@ -23,6 +23,8 @@ import { store as blockEditorStore } from '../../store';
 import { __unstableUseBlockElement as useBlockElement } from '../block-list/use-block-props/use-block-refs';
 import usePopoverScroll from './use-popover-scroll';
 
+const MAX_POPOVER_RECOMPUTE_COUNTER = Number.MAX_SAFE_INTEGER;
+
 export const InsertionPointOpenRef = createContext();
 
 function BlockPopoverInbetween( {
@@ -34,8 +36,9 @@ function BlockPopoverInbetween( {
 	...props
 } ) {
 	// This is a temporary hack to get the inbetween inserter to recompute properly.
-	const [ positionRecompute, forceRecompute ] = useReducer(
-		( s ) => s + 1,
+	const [ popoverRecomputeCounter, forcePopoverRecompute ] = useReducer(
+		// Module is there to make sure that the counter doesn't overflow.
+		( s ) => ( s + 1 ) % MAX_POPOVER_RECOMPUTE_COUNTER,
 		0
 	);
 
@@ -66,7 +69,14 @@ function BlockPopoverInbetween( {
 	const nextElement = useBlockElement( nextClientId );
 	const isVertical = orientation === 'vertical';
 	const style = useMemo( () => {
-		if ( ( ! previousElement && ! nextElement ) || ! isVisible ) {
+		if (
+			// popoverRecomputeCounter is by definition always equal or greater than 0.
+			// This check is only there to satisfy the correctness of the
+			// exhaustive-deps rule for the `useMemo` hook.
+			popoverRecomputeCounter < 0 ||
+			( ! previousElement && ! nextElement ) ||
+			! isVisible
+		) {
 			return {};
 		}
 
@@ -102,12 +112,19 @@ function BlockPopoverInbetween( {
 		previousElement,
 		nextElement,
 		isVertical,
-		positionRecompute,
+		popoverRecomputeCounter,
 		isVisible,
 	] );
 
 	const popoverAnchor = useMemo( () => {
-		if ( ( ! previousElement && ! nextElement ) || ! isVisible ) {
+		if (
+			// popoverRecomputeCounter is by definition always equal or greater than 0.
+			// This check is only there to satisfy the correctness of the
+			// exhaustive-deps rule for the `useMemo` hook.
+			popoverRecomputeCounter < 0 ||
+			( ! previousElement && ! nextElement ) ||
+			! isVisible
+		) {
 			return undefined;
 		}
 
@@ -161,19 +178,26 @@ function BlockPopoverInbetween( {
 	}, [
 		previousElement,
 		nextElement,
-		positionRecompute,
+		popoverRecomputeCounter,
 		isVertical,
 		isVisible,
 	] );
 
 	const popoverScrollRef = usePopoverScroll( __unstableContentRef );
 
 	// This is only needed for a smooth transition when moving blocks.
+	// When blocks are moved up/down, their position can be set by
+	// updating the `transform` property manually (i.e. without using CSS
+	// transitions or animations). The animation, which can also scroll the block
+	// editor, can sometimes cause the position of the Popover to get out of sync.
+	// A MutationObserver is therefore used to make sure that changes to the
+	// selectedElement's attribute (i.e. `transform`) can be tracked and used to
+	// trigger the Popover to rerender.
 	useLayoutEffect( () => {
 		if ( ! previousElement ) {
 			return;
 		}
-		const observer = new window.MutationObserver( forceRecompute );
+		const observer = new window.MutationObserver( forcePopoverRecompute );
 		observer.observe( previousElement, { attributes: true } );
 
 		return () => {
@@ -185,7 +209,7 @@ function BlockPopoverInbetween( {
 		if ( ! nextElement ) {
 			return;
 		}
-		const observer = new window.MutationObserver( forceRecompute );
+		const observer = new window.MutationObserver( forcePopoverRecompute );
 		observer.observe( nextElement, { attributes: true } );
 
 		return () => {
@@ -199,12 +223,12 @@ function BlockPopoverInbetween( {
 		}
 		previousElement.ownerDocument.defaultView.addEventListener(
 			'resize',
-			forceRecompute
+			forcePopoverRecompute
 		);
 		return () => {
 			previousElement.ownerDocument.defaultView.removeEventListener(
 				'resize',
-				forceRecompute
+				forcePopoverRecompute
 			);
 		};
 	}, [ previousElement ] );

diff --git a/packages/block-editor/src/components/block-popover/index.js b/packages/block-editor/src/components/block-popover/index.js
@@ -8,14 +8,21 @@ import classnames from 'classnames';
  */
 import { useMergeRefs } from '@wordpress/compose';
 import { Popover } from '@wordpress/components';
-import { forwardRef, useMemo } from '@wordpress/element';
+import {
+	forwardRef,
+	useMemo,
+	useReducer,
+	useLayoutEffect,
+} from '@wordpress/element';
 
 /**
  * Internal dependencies
  */
 import { __unstableUseBlockElement as useBlockElement } from '../block-list/use-block-props/use-block-refs';
 import usePopoverScroll from './use-popover-scroll';
 
+const MAX_POPOVER_RECOMPUTE_COUNTER = Number.MAX_SAFE_INTEGER;
+
 function BlockPopover(
 	{
 		clientId,
@@ -47,8 +54,41 @@ function BlockPopover(
 		};
 	}, [ selectedElement, lastSelectedElement, __unstableRefreshSize ] );
 
+	const [ popoverAnchorRecomputeCounter, forceRecomputePopoverAnchor ] =
+		useReducer(
+			// Module is there to make sure that the counter doesn't overflow.
+			( s ) => ( s + 1 ) % MAX_POPOVER_RECOMPUTE_COUNTER,
+			0
+		);
+
+	// When blocks are moved up/down, they are animated to their new position by
+	// updating the `transform` property manually (i.e. without using CSS
+	// transitions or animations). The animation, which can also scroll the block
+	// editor, can sometimes cause the position of the Popover to get out of sync.
+	// A MutationObserver is therefore used to make sure that changes to the
+	// selectedElement's attribute (i.e. `transform`) can be tracked and used to
+	// trigger the Popover to rerender.
+	useLayoutEffect( () => {
+		if ( ! selectedElement ) {
+			return;
+		}
+
+		const observer = new window.MutationObserver(
+			forceRecomputePopoverAnchor
+		);
+		observer.observe( selectedElement, { attributes: true } );
+
+		return () => {
+			observer.disconnect();
+		};
+	}, [ selectedElement ] );
+
 	const popoverAnchor = useMemo( () => {
 		if (
+			// popoverAnchorRecomputeCounter is by definition always equal or greater
+			// than 0. This check is only there to satisfy the correctness of the
+			// exhaustive-deps rule for the `useMemo` hook.
+			popoverAnchorRecomputeCounter < 0 ||
 			! selectedElement ||
 			( bottomClientId && ! lastSelectedElement )
 		) {
@@ -88,7 +128,12 @@ function BlockPopover(
 			},
 			ownerDocument: selectedElement.ownerDocument,
 		};
-	}, [ bottomClientId, lastSelectedElement, selectedElement ] );
+	}, [
+		bottomClientId,
+		lastSelectedElement,
+		selectedElement,
+		popoverAnchorRecomputeCounter,
+	] );
 
 	if ( ! selectedElement || ( bottomClientId && ! lastSelectedElement ) ) {
 		return null;