From ed8d25e651f9673583ae1c4800339eec45684830 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Sep 2022 18:10:55 +0200 Subject: [PATCH] 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 --- .../src/components/block-popover/inbetween.js | 44 +++++++++++++---- .../src/components/block-popover/index.js | 49 ++++++++++++++++++- 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/packages/block-editor/src/components/block-popover/inbetween.js b/packages/block-editor/src/components/block-popover/inbetween.js index 442ae06edffe08..e7e207cb733f1f 100644 --- 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,7 +178,7 @@ function BlockPopoverInbetween( { }, [ previousElement, nextElement, - positionRecompute, + popoverRecomputeCounter, isVertical, isVisible, ] ); @@ -169,11 +186,18 @@ function BlockPopoverInbetween( { 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 index e2b2f58d5d4e31..03913faa26e421 100644 --- a/packages/block-editor/src/components/block-popover/index.js +++ b/packages/block-editor/src/components/block-popover/index.js @@ -8,7 +8,12 @@ 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 @@ -16,6 +21,8 @@ import { forwardRef, useMemo } from '@wordpress/element'; 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;