Fix MarginVisualizer not appearing, and fix MarginVisualizer + Paddin…

…gVisualizer showing the *last* value
WordPress · Feb 21, 2024 · 5a67442 · 5a67442
1 parent 8180508
commit 5a67442
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 67 deletions.
diff --git a/packages/block-editor/src/hooks/margin.js b/packages/block-editor/src/hooks/margin.js
@@ -1,7 +1,12 @@
 /**
  * WordPress dependencies
  */
-import { useState, useRef, useEffect } from '@wordpress/element';
+import {
+	useState,
+	useRef,
+	useLayoutEffect,
+	useEffect,
+} from '@wordpress/element';
 import isShallowEqual from '@wordpress/is-shallow-equal';
 
 /**
@@ -16,60 +21,59 @@ function getComputedCSS( element, property ) {
 		.getPropertyValue( property );
 }
 
-export function MarginVisualizer( { clientId, attributes, forceShow } ) {
+export function MarginVisualizer( { clientId, value, forceShow } ) {
 	const blockElement = useBlockElement( clientId );
 	const [ style, setStyle ] = useState();
 
-	const margin = attributes?.style?.spacing?.margin;
+	const margin = value?.spacing?.margin;
 
-	useEffect( () => {
-		if (
-			! blockElement ||
-			null === blockElement.ownerDocument.defaultView
-		) {
+	useLayoutEffect( () => {
+		if ( ! blockElement ) {
 			return;
 		}
-
-		const top = getComputedCSS( blockElement, 'margin-top' );
-		const right = getComputedCSS( blockElement, 'margin-right' );
-		const bottom = getComputedCSS( blockElement, 'margin-bottom' );
-		const left = getComputedCSS( blockElement, 'margin-left' );
-
-		setStyle( {
-			borderTopWidth: top,
-			borderRightWidth: right,
-			borderBottomWidth: bottom,
-			borderLeftWidth: left,
-			top: top ? `-${ top }` : 0,
-			right: right ? `-${ right }` : 0,
-			bottom: bottom ? `-${ bottom }` : 0,
-			left: left ? `-${ left }` : 0,
-		} );
+		// It's not sufficient to read the computed padding value when value.spacing.padding
+		// changes as useEffect may run before the browser recomputes CSS and paints, and,
+		// unlike padding, there's no way to observe when the margin changes using ResizeObserver.
+		// We therefore combine useLayoutEffect and two rAF calls to ensure that we read the margin
+		// after the current paint but before the next paint.
+		window.requestAnimationFrame( () =>
+			window.requestAnimationFrame( () => {
+				const top = getComputedCSS( blockElement, 'margin-top' );
+				const right = getComputedCSS( blockElement, 'margin-right' );
+				const bottom = getComputedCSS( blockElement, 'margin-bottom' );
+				const left = getComputedCSS( blockElement, 'margin-left' );
+				setStyle( {
+					borderTopWidth: top,
+					borderRightWidth: right,
+					borderBottomWidth: bottom,
+					borderLeftWidth: left,
+					top: top ? `-${ top }` : 0,
+					right: right ? `-${ right }` : 0,
+					bottom: bottom ? `-${ bottom }` : 0,
+					left: left ? `-${ left }` : 0,
+				} );
+			} )
+		);
 	}, [ blockElement, margin ] );
 
+	const previousMargin = useRef( margin );
 	const [ isActive, setIsActive ] = useState( false );
-	const valueRef = useRef( margin );
-	const timeoutRef = useRef();
 
-	const clearTimer = () => {
-		if ( timeoutRef.current ) {
-			window.clearTimeout( timeoutRef.current );
+	useEffect( () => {
+		if ( isShallowEqual( margin, previousMargin.current ) || forceShow ) {
+			return;
 		}
-	};
 
-	useEffect( () => {
-		if ( ! isShallowEqual( margin, valueRef.current ) && ! forceShow ) {
-			setIsActive( true );
-			valueRef.current = margin;
+		setIsActive( true );
+		previousMargin.current = margin;
 
-			timeoutRef.current = setTimeout( () => {
-				setIsActive( false );
-			}, 400 );
-		}
+		const timeout = setTimeout( () => {
+			setIsActive( false );
+		}, 400 );
 
 		return () => {
 			setIsActive( false );
-			clearTimer();
+			clearTimeout( timeout );
 		};
 	}, [ margin, forceShow ] );
 

diff --git a/packages/block-editor/src/hooks/padding.js b/packages/block-editor/src/hooks/padding.js
@@ -20,47 +20,51 @@ export function PaddingVisualizer( { clientId, value, forceShow } ) {
 	const blockElement = useBlockElement( clientId );
 	const [ style, setStyle ] = useState();
 
-	const padding = value?.spacing?.padding;
-
 	useEffect( () => {
-		if (
-			! blockElement ||
-			null === blockElement.ownerDocument.defaultView
-		) {
+		if ( ! blockElement ) {
 			return;
 		}
-
-		setStyle( {
-			borderTopWidth: getComputedCSS( blockElement, 'padding-top' ),
-			borderRightWidth: getComputedCSS( blockElement, 'padding-right' ),
-			borderBottomWidth: getComputedCSS( blockElement, 'padding-bottom' ),
-			borderLeftWidth: getComputedCSS( blockElement, 'padding-left' ),
+		// It's not sufficient to read the computed padding value when value.spacing.padding
+		// changes as useEffect may run before the browser recomputes CSS and paints. Instead,
+		// we use a ResizeObserver with the box option set to 'border-box' to listen out for
+		// changes to the padding.
+		const observer = new window.ResizeObserver( () => {
+			setStyle( {
+				borderTopWidth: getComputedCSS( blockElement, 'padding-top' ),
+				borderRightWidth: getComputedCSS(
+					blockElement,
+					'padding-right'
+				),
+				borderBottomWidth: getComputedCSS(
+					blockElement,
+					'padding-bottom'
+				),
+				borderLeftWidth: getComputedCSS( blockElement, 'padding-left' ),
+			} );
 		} );
-	}, [ blockElement, padding ] );
+		observer.observe( blockElement, { box: 'border-box' } );
+		return () => observer.disconnect();
+	}, [ blockElement ] );
 
+	const padding = value?.spacing?.padding;
+	const previousPadding = useRef( padding );
 	const [ isActive, setIsActive ] = useState( false );
-	const valueRef = useRef( padding );
-	const timeoutRef = useRef();
 
-	const clearTimer = () => {
-		if ( timeoutRef.current ) {
-			window.clearTimeout( timeoutRef.current );
+	useEffect( () => {
+		if ( isShallowEqual( padding, previousPadding.current ) || forceShow ) {
+			return;
 		}
-	};
 
-	useEffect( () => {
-		if ( ! isShallowEqual( padding, valueRef.current ) && ! forceShow ) {
-			setIsActive( true );
-			valueRef.current = padding;
+		setIsActive( true );
+		previousPadding.current = padding;
 
-			timeoutRef.current = setTimeout( () => {
-				setIsActive( false );
-			}, 400 );
-		}
+		const timeout = setTimeout( () => {
+			setIsActive( false );
+		}, 400 );
 
 		return () => {
 			setIsActive( false );
-			clearTimer();
+			clearTimeout( timeout );
 		};
 	}, [ padding, forceShow ] );