From 14929a805c13e389df99ba9ca8151c404d49f229 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 21 Feb 2024 15:23:26 +1100 Subject: [PATCH 1/3] Fix MarginVisualizer not appearing, and fix MarginVisualizer + PaddingVisualizer showing the *last* value --- packages/block-editor/src/hooks/margin.js | 82 ++++++++++++---------- packages/block-editor/src/hooks/padding.js | 60 ++++++++-------- 2 files changed, 75 insertions(+), 67 deletions(-) diff --git a/packages/block-editor/src/hooks/margin.js b/packages/block-editor/src/hooks/margin.js index 7be1179d29510..4a0a07fd7a68e 100644 --- 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 index 50eed7ac05d5e..f6d9b3a912f54 100644 --- 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 ] ); From 97956dbd8369cefb421bc83543060e36d7917d04 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 23 Feb 2024 14:03:00 +1100 Subject: [PATCH 2/3] Use rAF approach for PaddingVisualizer as well ResizeObserver doesn't work in the case where an element with a fixed width has its padding changed. --- packages/block-editor/src/hooks/padding.js | 58 +++++++++++++--------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/packages/block-editor/src/hooks/padding.js b/packages/block-editor/src/hooks/padding.js index f6d9b3a912f54..84883d416a80d 100644 --- a/packages/block-editor/src/hooks/padding.js +++ b/packages/block-editor/src/hooks/padding.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'; /** @@ -20,33 +25,40 @@ export function PaddingVisualizer( { clientId, value, forceShow } ) { const blockElement = useBlockElement( clientId ); const [ style, setStyle ] = useState(); - useEffect( () => { + const padding = value?.spacing?.padding; + + useLayoutEffect( () => { if ( ! blockElement ) { return; } // 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' ), - } ); - } ); - observer.observe( blockElement, { box: 'border-box' } ); - return () => observer.disconnect(); - }, [ blockElement ] ); + // changes as useEffect may run before the browser recomputes CSS. We therefore combine + // useLayoutEffect and two rAF calls to ensure that we read the padding after the current + // paint but before the next paint. + window.requestAnimationFrame( () => + window.requestAnimationFrame( () => { + setStyle( { + borderTopWidth: getComputedCSS( + blockElement, + 'padding-top' + ), + borderRightWidth: getComputedCSS( + blockElement, + 'padding-right' + ), + borderBottomWidth: getComputedCSS( + blockElement, + 'padding-bottom' + ), + borderLeftWidth: getComputedCSS( + blockElement, + 'padding-left' + ), + } ); + } ) + ); + }, [ blockElement, padding ] ); - const padding = value?.spacing?.padding; const previousPadding = useRef( padding ); const [ isActive, setIsActive ] = useState( false ); From 77ef06c45b381f26f0a1c1f3d2381e6710dd6aa5 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 23 Feb 2024 14:19:53 +1100 Subject: [PATCH 3/3] DRY MarginVisualizer + PaddingVisualizer -> SpacingVisualizer --- packages/block-editor/src/hooks/dimensions.js | 3 +- packages/block-editor/src/hooks/padding.js | 95 --------------- .../{margin.js => spacing-visualizer.js} | 108 ++++++++++++------ .../src/hooks/{padding.scss => spacing.scss} | 2 +- packages/block-editor/src/style.scss | 2 +- 5 files changed, 74 insertions(+), 136 deletions(-) delete mode 100644 packages/block-editor/src/hooks/padding.js rename packages/block-editor/src/hooks/{margin.js => spacing-visualizer.js} (52%) rename packages/block-editor/src/hooks/{padding.scss => spacing.scss} (84%) diff --git a/packages/block-editor/src/hooks/dimensions.js b/packages/block-editor/src/hooks/dimensions.js index 6f03ddac2c650..f78a39230e89b 100644 --- a/packages/block-editor/src/hooks/dimensions.js +++ b/packages/block-editor/src/hooks/dimensions.js @@ -19,8 +19,7 @@ import { DimensionsPanel as StylesDimensionsPanel, useHasDimensionsPanel, } from '../components/global-styles'; -import { MarginVisualizer } from './margin'; -import { PaddingVisualizer } from './padding'; +import { MarginVisualizer, PaddingVisualizer } from './spacing-visualizer'; import { store as blockEditorStore } from '../store'; import { unlock } from '../lock-unlock'; import { cleanEmptyObject, shouldSkipSerialization } from './utils'; diff --git a/packages/block-editor/src/hooks/padding.js b/packages/block-editor/src/hooks/padding.js deleted file mode 100644 index 84883d416a80d..0000000000000 --- a/packages/block-editor/src/hooks/padding.js +++ /dev/null @@ -1,95 +0,0 @@ -/** - * WordPress dependencies - */ -import { - useState, - useRef, - useLayoutEffect, - useEffect, -} from '@wordpress/element'; -import isShallowEqual from '@wordpress/is-shallow-equal'; - -/** - * Internal dependencies - */ -import BlockPopoverCover from '../components/block-popover/cover'; -import { __unstableUseBlockElement as useBlockElement } from '../components/block-list/use-block-props/use-block-refs'; - -function getComputedCSS( element, property ) { - return element.ownerDocument.defaultView - .getComputedStyle( element ) - .getPropertyValue( property ); -} - -export function PaddingVisualizer( { clientId, value, forceShow } ) { - const blockElement = useBlockElement( clientId ); - const [ style, setStyle ] = useState(); - - const padding = value?.spacing?.padding; - - useLayoutEffect( () => { - if ( ! blockElement ) { - return; - } - // It's not sufficient to read the computed padding value when value.spacing.padding - // changes as useEffect may run before the browser recomputes CSS. We therefore combine - // useLayoutEffect and two rAF calls to ensure that we read the padding after the current - // paint but before the next paint. - window.requestAnimationFrame( () => - window.requestAnimationFrame( () => { - setStyle( { - borderTopWidth: getComputedCSS( - blockElement, - 'padding-top' - ), - borderRightWidth: getComputedCSS( - blockElement, - 'padding-right' - ), - borderBottomWidth: getComputedCSS( - blockElement, - 'padding-bottom' - ), - borderLeftWidth: getComputedCSS( - blockElement, - 'padding-left' - ), - } ); - } ) - ); - }, [ blockElement, padding ] ); - - const previousPadding = useRef( padding ); - const [ isActive, setIsActive ] = useState( false ); - - useEffect( () => { - if ( isShallowEqual( padding, previousPadding.current ) || forceShow ) { - return; - } - - setIsActive( true ); - previousPadding.current = padding; - - const timeout = setTimeout( () => { - setIsActive( false ); - }, 400 ); - - return () => { - setIsActive( false ); - clearTimeout( timeout ); - }; - }, [ padding, forceShow ] ); - - if ( ! isActive && ! forceShow ) { - return null; - } - - return ( - -
- - ); -} diff --git a/packages/block-editor/src/hooks/margin.js b/packages/block-editor/src/hooks/spacing-visualizer.js similarity index 52% rename from packages/block-editor/src/hooks/margin.js rename to packages/block-editor/src/hooks/spacing-visualizer.js index 4a0a07fd7a68e..7189b0f480d2c 100644 --- a/packages/block-editor/src/hooks/margin.js +++ b/packages/block-editor/src/hooks/spacing-visualizer.js @@ -6,6 +6,7 @@ import { useRef, useLayoutEffect, useEffect, + useReducer, } from '@wordpress/element'; import isShallowEqual from '@wordpress/is-shallow-equal'; @@ -15,57 +16,36 @@ import isShallowEqual from '@wordpress/is-shallow-equal'; import BlockPopoverCover from '../components/block-popover/cover'; import { __unstableUseBlockElement as useBlockElement } from '../components/block-list/use-block-props/use-block-refs'; -function getComputedCSS( element, property ) { - return element.ownerDocument.defaultView - .getComputedStyle( element ) - .getPropertyValue( property ); -} - -export function MarginVisualizer( { clientId, value, forceShow } ) { +function SpacingVisualizer( { clientId, value, computeStyle, forceShow } ) { const blockElement = useBlockElement( clientId ); - const [ style, setStyle ] = useState(); - - const margin = value?.spacing?.margin; + const [ style, updateStyle ] = useReducer( () => + computeStyle( blockElement ) + ); useLayoutEffect( () => { if ( ! blockElement ) { return; } - // 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. + // It's not sufficient to read the computed spacing value when value.spacing changes as + // useEffect may run before the browser recomputes CSS. We therefore combine + // useLayoutEffect and two rAF calls to ensure that we read the spacing after the current + // paint but before the next paint. + // See https://github.com/WordPress/gutenberg/pull/59227. 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, - } ); - } ) + window.requestAnimationFrame( updateStyle ) ); - }, [ blockElement, margin ] ); + }, [ blockElement, value ] ); - const previousMargin = useRef( margin ); + const previousValue = useRef( value ); const [ isActive, setIsActive ] = useState( false ); useEffect( () => { - if ( isShallowEqual( margin, previousMargin.current ) || forceShow ) { + if ( isShallowEqual( value, previousValue.current ) || forceShow ) { return; } setIsActive( true ); - previousMargin.current = margin; + previousValue.current = value; const timeout = setTimeout( () => { setIsActive( false ); @@ -75,7 +55,7 @@ export function MarginVisualizer( { clientId, value, forceShow } ) { setIsActive( false ); clearTimeout( timeout ); }; - }, [ margin, forceShow ] ); + }, [ value, forceShow ] ); if ( ! isActive && ! forceShow ) { return null; @@ -86,7 +66,61 @@ export function MarginVisualizer( { clientId, value, forceShow } ) { clientId={ clientId } __unstablePopoverSlot="block-toolbar" > -
+
); } + +function getComputedCSS( element, property ) { + return element.ownerDocument.defaultView + .getComputedStyle( element ) + .getPropertyValue( property ); +} + +export function MarginVisualizer( { clientId, value, forceShow } ) { + 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' ); + return { + borderTopWidth: top, + borderRightWidth: right, + borderBottomWidth: bottom, + borderLeftWidth: left, + top: top ? `-${ top }` : 0, + right: right ? `-${ right }` : 0, + bottom: bottom ? `-${ bottom }` : 0, + left: left ? `-${ left }` : 0, + }; + } } + forceShow={ forceShow } + /> + ); +} + +export function PaddingVisualizer( { clientId, value, forceShow } ) { + return ( + ( { + borderTopWidth: getComputedCSS( blockElement, 'padding-top' ), + borderRightWidth: getComputedCSS( + blockElement, + 'padding-right' + ), + borderBottomWidth: getComputedCSS( + blockElement, + 'padding-bottom' + ), + borderLeftWidth: getComputedCSS( blockElement, 'padding-left' ), + } ) } + forceShow={ forceShow } + /> + ); +} diff --git a/packages/block-editor/src/hooks/padding.scss b/packages/block-editor/src/hooks/spacing.scss similarity index 84% rename from packages/block-editor/src/hooks/padding.scss rename to packages/block-editor/src/hooks/spacing.scss index dbb9991988189..52894d8ff2ba6 100644 --- a/packages/block-editor/src/hooks/padding.scss +++ b/packages/block-editor/src/hooks/spacing.scss @@ -1,4 +1,4 @@ -.block-editor__padding-visualizer { +.block-editor__spacing-visualizer { position: absolute; top: 0; bottom: 0; diff --git a/packages/block-editor/src/style.scss b/packages/block-editor/src/style.scss index 01931adace3f1..55ea0b7fa91d9 100644 --- a/packages/block-editor/src/style.scss +++ b/packages/block-editor/src/style.scss @@ -52,8 +52,8 @@ @import "./hooks/color.scss"; @import "./hooks/dimensions.scss"; @import "./hooks/layout.scss"; -@import "./hooks/padding.scss"; @import "./hooks/position.scss"; +@import "./hooks/spacing.scss"; @import "./hooks/typography.scss"; @import "./components/block-toolbar/style.scss";