Spacer: Fix changes being marked as persistent to undo

[yogeshbhutkar]

What, Why and How?

Fixes: #68863

Currently, inserting or moving a Spacer block into a Row or Stack block disrupts the undo functionality. Similarly, moving a Spacer block out of a Row or Stack block also breaks the undo behavior.

Also, Converting a Row block containing a Spacer block into a Group block caused undo to fail.

Testing Instructions

Step 1: Navigate to the post edit page.
Step 2: Verify the undo functionality with Spacer works for the following cases:

• Case 1: Using Flex Containers. ( i.e. Row & Stack without setting selfStretch to fit or fill ).
• Case 2: Using Flex Containers with selfStretch to be either fit or fill.
• Case 3: Non-Flex Container with selfStretch to be either fit or fill.
• Case 4: Move Spacer into Row/Stack.
• Case 5: Move Spacer out of Row/Stack.
• Case 6: With Spacer inserted into a Row/Stack, convert it to Group and verify undo.

Screencast

Screen.Recording.2025-01-24.at.3.32.05.PM.mov

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <[email protected]>
Co-authored-by: stokesman <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[yogeshbhutkar]

Hi, @stokesman, can you please review the PR when you get a moment?

[stokesman]

Thanks for tackling this one! The changes make sense. I’ve tested and can no longer reproduce the issue.

I have one specific suggestion I’d like to see done and I’ll leave a separate comment fort that. Alternatively, I’ll include here a larger suggested diff that incorporates it but also changes the code style to be less verbose. Please apply it only if you’d agree it’s a clear improvement. I’m fine with keeping the changes more minimal.

Reduce some verbosity and add a general comment about making changes non-persistent

diff --git a/packages/block-library/src/spacer/edit.js b/packages/block-library/src/spacer/edit.js
index cd21870f0c..c11201861a 100644
--- a/packages/block-library/src/spacer/edit.js
+++ b/packages/block-library/src/spacer/edit.js
@@ -259,13 +259,18 @@ const SpacerEdit = ( {
 	};
 
 	useEffect( () => {
+		// To avoid interfering with undo/redo operations any changes in this
+		// effect must not make history.
+		const setAttributesCovertly = ( nextAttributes ) => {
+			__unstableMarkNextChangeAsNotPersistent();
+			setAttributes( nextAttributes );
+		};
 		if (
 			isFlexLayout &&
 			selfStretch !== 'fill' &&
 			selfStretch !== 'fit' &&
 			flexSize === undefined
 		) {
-			__unstableMarkNextChangeAsNotPersistent();
 			if ( inheritedOrientation === 'horizontal' ) {
 				// If spacer is moving from a vertical container to a horizontal container,
 				// it might not have width but have height instead.
@@ -273,7 +278,7 @@ const SpacerEdit = ( {
 					getCustomValueFromPreset( width, spacingSizes ) ||
 					getCustomValueFromPreset( height, spacingSizes ) ||
 					'100px';
-				setAttributes( {
+				setAttributesCovertly( {
 					width: '0px',
 					style: {
 						...blockStyle,
@@ -289,7 +294,7 @@ const SpacerEdit = ( {
 					getCustomValueFromPreset( height, spacingSizes ) ||
 					getCustomValueFromPreset( width, spacingSizes ) ||
 					'100px';
-				setAttributes( {
+				setAttributesCovertly( {
 					height: '0px',
 					style: {
 						...blockStyle,
@@ -305,32 +310,16 @@ const SpacerEdit = ( {
 			isFlexLayout &&
 			( selfStretch === 'fill' || selfStretch === 'fit' )
 		) {
-			__unstableMarkNextChangeAsNotPersistent();
-			if ( inheritedOrientation === 'horizontal' ) {
-				setAttributes( {
-					width: undefined,
-				} );
-			} else {
-				setAttributes( {
-					height: undefined,
-				} );
-			}
+			setAttributesCovertly(
+				inheritedOrientation === 'horizontal'
+					? { width: undefined }
+					: { height: undefined }
+			);
 		} else if ( ! isFlexLayout && ( selfStretch || flexSize ) ) {
-			__unstableMarkNextChangeAsNotPersistent();
-			if ( inheritedOrientation === 'horizontal' ) {
-				setAttributes( {
-					width: flexSize,
-				} );
-			} else {
-				setAttributes( {
-					height: flexSize,
-				} );
-			}
-
-			// This ensures that the next change is not marked as persistent
-			// when the user changes the container layout.
-			__unstableMarkNextChangeAsNotPersistent();
-			setAttributes( {
+			setAttributesCovertly( {
+				...( inheritedOrientation === 'horizontal'
+					? { width: flexSize }
+					: { height: flexSize } ),
 				style: {
 					...blockStyle,
 					layout: {

[stokesman]

Given there are multiple places in the effect this comment is applicable I think it’d be better to remove this and instead put a more generalized version at the start of the effect. Something like:

// To avoid interfering with undo/redo operations any changes in this
// effect must not make history and should be preceded by
// `__unstableMarkNextChangeAsNotPersistent()`

[yogeshbhutkar]

Thanks for the review, @stokesman, I've incorporated the suggested changes in the latest commit.

[yogeshbhutkar]

While testing this PR, I discovered a bug in the Spacer block. When a flex container's orientation is vertical and the Spacer block's selfStretch is set to Grow, the resizable div lacks height, causing the draggable indicator to align at the top instead of the bottom. Additionally, the gray highlight on selecting the Spacer block is missing.

Screen.Recording.2025-01-27.at.10.38.58.AM.mov

Expected:

It's best to tackle this issue separately.