SpacerControls: default the height to `100px` if left unset #68819

yogeshbhutkar · 2025-01-22T04:59:04Z

What, Why and How?

When the height of the spacer block is set to undefined, on reloading the editor, a Recovery Error is shown for the Spacer Block.

The root cause of the problem lies in the fact that the default value of height attribute defined in block.json is 100px, therefore, whenever the height attribute is made undefined, on reload, the default value kicks in and causes a mismatch in the content generated by the save function as well as the post body leading to this warning:

Content generated by the `save` function:
<div style="height:100px" aria-hidden="true" class="wp-block-spacer"></div>

Content retrieved from post body:
<div aria-hidden="true" class="wp-block-spacer"></div>

There are two potential solutions for this problem:

Remove the default value of height attribute (100px) from block.json.

gutenberg/packages/block-library/src/spacer/block.json

Line 12 in a60dd78

"default": "100px"
Save 0px if height is undefined.

The later approach i.e. (2) makes more sense as:
i. This is how the height slider implements it.
ii. Width follows a similar approach.
iii. It doesn't quite make sense to save the height of the spacer as undefined.

Testing Instructions

Create a spacer block.
Set its height to undefined by trying to save an empty input.
Save and reload the page and confirm no Block Recovery error occurs.

Screencast

Screen.Recording.2025-01-22.at.11.23.20.AM.mov

github-actions · 2025-01-22T06:42:10Z

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 <yogeshbhutkar@git.wordpress.org>
Co-authored-by: stokesman <presstoke@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>
Co-authored-by: himanshupathak95 <abcd95@git.wordpress.org>
Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>

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

yogeshbhutkar · 2025-01-22T06:49:16Z

Hi @Mamaduka, could you please review this PR and share your thoughts on the proposed approach? I'd appreciate your feedback!

stokesman

The idea to avoid setting height to undefined seems right to me. 0px seems a fine fallback and fixes the issue. Though perhaps an empty string is a hair better because height will not be output in the inline styles. That said, clearing the spacer height and saving it is rare usage so not much of a concern 🤷.

yogeshbhutkar · 2025-01-23T10:57:31Z

Hi @stokesman, thanks for the review. I've updated the PR to include the suggested changes.

The empty string works better and the styles aren't reflected in the inline styles. Thanks for pointing that out.

yogeshbhutkar · 2025-01-27T08:29:01Z

Hi @carolinan, can I please get a review on this PR?

Mamaduka · 2025-01-27T09:00:40Z

@yogeshbhutkar, my suggestion would be to avoid pinging people often. Everyone is monitoring the PRs, and they'll get reviewed when folks have time.

carolinan · 2025-01-28T07:03:08Z

This solves the problem for newly inserted spacer blocks.
Blocks that have already been placed and are broken because the height attribute is missing are still broken.
Can this be fixed with a deprecation that adds the height if the height and width attribute is missing?

yogeshbhutkar · 2025-01-28T12:07:08Z

packages/block-library/src/spacer/deprecated.js

+		return {
+			...attributes,
+			width: width !== undefined ? `${ width }px` : undefined,
+			height: `${ height }px`,


Thanks for the review!

The issue with the previously broken spacer blocks, which had missing heights, has been resolved using deprecation. The current implementation has been updated accordingly.

Additionally, the undefined check for the height attribute was redundant since a default value is always provided. Therefore, this check has been removed.

carolinan · 2025-01-29T06:43:47Z

I am confused, why was the change to controls.js reverted?

yogeshbhutkar · 2025-01-29T06:47:27Z

That change would then become redundant. The idea is that if undefined gets stored, the deprecation would run and initialize height to be an empty string. This way, the block validation error never occurs, and we don't need to repeat the logic twice. Therefore, that change was not required.

What do you think about the approach? Should we also keep the change in control.js?

carolinan · 2025-01-29T07:14:26Z

I don't think the deprecation script should run unless it is strictly necessary, when the block is invalid. It would be better to prevent it from being invalid in the first place.

yogeshbhutkar · 2025-01-29T08:10:08Z

It would be better to prevent it from being invalid in the first place.

Thanks for highlighting that! After reviewing the context, I completely agree and have incorporated the changes in my latest commit ✨.

t-hamano · 2025-02-23T09:38:38Z

Personally, I would like the default height of 100px to be restored when the value of the Height option is deleted, but what do you think? When a user deletes the value of the Height option, I suspect that they want to restore the default value. Currently, the only way to restore the default value is to enter a value directly or operate the slider.

In other words, make the following changes:

diff

diff --git a/packages/block-library/src/spacer/edit.js b/packages/block-library/src/spacer/edit.js
index b2de69ad9a..b3e9103914 100644
--- a/packages/block-library/src/spacer/edit.js
+++ b/packages/block-library/src/spacer/edit.js
@@ -171,7 +171,7 @@ const SpacerEdit = ( {
                if ( isFlexLayout ) {
                        return undefined;
                }
-               return temporaryHeight || getSpacingPresetCssVar( height ) || undefined;
+               return temporaryHeight || getSpacingPresetCssVar( height ) || '100px';
        };
 
        const getWidthForHorizontalBlocks = () => {
@@ -186,6 +186,8 @@ const SpacerEdit = ( {
                height:
                        inheritedOrientation === 'horizontal'
diff --git a/packages/block-library/src/spacer/save.js b/packages/block-library/src/spacer/save.js
index dd3a595a80..a1ab58c15b 100644
--- a/packages/block-library/src/spacer/save.js
+++ b/packages/block-library/src/spacer/save.js
@@ -3,6 +3,8 @@
  */
 import { useBlockProps, getSpacingPresetCssVar } from '@wordpress/block-editor';
 
+const DEFAULT_HEIGHT = '100px';
+
 export default function save( { attributes } ) {
        const { height, width, style } = attributes;
        const { layout: { selfStretch } = {} } = style || {};
@@ -13,7 +15,8 @@ export default function save( { attributes } ) {
                <div
                        { ...useBlockProps.save( {
                                style: {
-                                       height: getSpacingPresetCssVar( finalHeight ),
+                                       height:
+                                               getSpacingPresetCssVar( finalHeight ) || DEFAULT_HEIGHT,
                                        width: getSpacingPresetCssVar( width ),
                                },
                                'aria-hidden': true,

42a4f51f34bad196e2acec115ecd5508.mp4

yogeshbhutkar · 2025-02-24T04:36:43Z

This makes more sense. A 0px Spacer Block wouldn’t be beneficial anyway, so it’s best to revert it to the default height of 100px.

yogeshbhutkar · 2025-02-24T05:42:28Z

Hi @t-hamano,
I've implemented the suggestions. This edge case now works as expected.

Do we also account for cases where the Spacer Block was previously undefined but should default to 100px after migration? Removing the default: 100 statement from deprecations might easily fix this, but I’m unsure if it’s even necessary in the first place, as this scenario is quite rare.

Diff

diff --git a/packages/block-library/src/spacer/deprecated.js b/packages/block-library/src/spacer/deprecated.js
index 79ecc09fe1..91874413e4 100644
--- a/packages/block-library/src/spacer/deprecated.js
+++ b/packages/block-library/src/spacer/deprecated.js
@@ -8,7 +8,6 @@ const deprecated = [
                attributes: {
                        height: {
                                type: 'number',
-                               default: 100,
                        },
                        width: {
                                type: 'number',

t-hamano

Thanks for the update!

Do we also account for cases where the Spacer Block was previously undefined but should default to 100px after migration? Removing the default: 100 statement from deprecations might easily fix this, but I’m unsure if it’s even necessary in the first place, as this scenario is quite rare.

I think this is a rare case and we don't need to deal with it. Sure, we can solve it by deleting default: 100, but then it will no longer match the attribute definition that this block had before.

My guess is that if the save function had been properly updated in the previous PR that added the update to save.js, the issue reported in #68817 would not have occurred in the first place. Furthermore, I expect that most users have already removed the broken spacer blocks or performed a recovery.

packages/block-library/src/spacer/edit.js

…sage

packages/block-library/src/spacer/edit.js

t-hamano

LGTM! Finally, can we apply DEFAULT_HEIGHT to the following places as well?

gutenberg/packages/block-library/src/spacer/controls.js

Line 103 in 0d5c310

height: '100px',

gutenberg/packages/block-library/src/spacer/controls.js

Lines 131 to 133 in 0d5c310

    
           hasValue={ () => height !== '100px' } 
        
           onDeselect={ () => 
        
           	setAttributes( { height: '100px' } )

t-hamano · 2025-03-05T02:40:23Z

Sorry, it seems that the approach I proposed caused some backwards compatibility issues in certain scenarios. See #69414

yogeshbhutkar · 2025-03-05T06:39:40Z

Apologies for introducing the above issue.

@t-hamano, since we now default to a 100px height when a spacer block’s height isn’t explicitly defined, previously saved Spacer blocks without a height attribute will trigger a validation error.

To maintain backward compatibility, I propose introducing a new deprecation:

const v2 = {
	attributes: {
		height: {
			type: 'string',
		},
		width: {
			type: 'string',
		},
	},
	isEligible( { height } ) {
		return height === undefined;
	},
	migrate( attributes ) {
		const { height } = attributes;
		return {
			...attributes,
			height: height !== undefined ? height : DEFAULT_HEIGHT,
		};
	},
	save,
};

What do you think?

Edit: I believe the height on flex containers should have been properly applied using flex-basis. This should be fixed in #69425

t-hamano · 2025-03-05T12:17:52Z

Maybe I was mistaken, but this PR is part of Gutenberg 20.5, so it hasn’t shipped as a plugin yet and won’t be bundled with WordPress 6.8, right?

cc @Mamaduka

If so, it might be better to revert this PR to avoid backwards compatibility issues, and then spend some more time finding the ideal solution.

Mamaduka · 2025-03-05T12:27:00Z

Correct. Gutenberg 20.4 is the last full release included in WP 6.8.

t-hamano · 2025-03-05T12:30:48Z

@yogeshbhutkar Can you submit a PR to revert this PR at once? Then we can take our time to work on this issue.

…s#68819) * SpacerControls: Set default height to '0px' when no value is provided * SpacerControls: Set height to an empty string when no value is provided * fix: deprecation logic to add height if height & width missing * refactor: minimize verbosity while exporting deprecations * fix: add width property to spacer deprecation schema for v2 * fix: set height to an empty string when nextHeight is null or undefined * fix: add `100px` as the default fallback * refactor: introduce `DEFAULT_HEIGHT` constant for consistent height usage * fix: add `DEFAULT_HEIGHT` fallback for vertical flex containers * refactor: apply `DEFAULT_HEIGHT` to missing places Co-authored-by: yogeshbhutkar <yogeshbhutkar@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: himanshupathak95 <abcd95@git.wordpress.org> Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>

yogeshbhutkar changed the title ~~SpacerControls: Set default height to '0px' when no value is provided~~ SpacerControls: Set default height to 0px when no value is provided Jan 22, 2025

Mamaduka added [Type] Bug An existing feature does not function as intended [Block] Spacer Affects the Spacer Block labels Jan 22, 2025

yogeshbhutkar marked this pull request as ready for review January 22, 2025 06:41

yogeshbhutkar requested review from ajitbohra and fabiankaegy as code owners January 22, 2025 06:41

Mamaduka requested review from Mamaduka and carolinan January 22, 2025 07:47

Mayank-Tripathi32 mentioned this pull request Jan 22, 2025

Saving a spacer block without a height value causes a block validation error #68817

Closed

6 tasks

stokesman reviewed Jan 23, 2025

View reviewed changes

yogeshbhutkar requested a review from stokesman January 23, 2025 10:57

yogeshbhutkar force-pushed the 68817-spacer-height-limit branch from c7b6d47 to 2a44603 Compare January 28, 2025 05:00

yogeshbhutkar commented Jan 28, 2025

View reviewed changes

yogeshbhutkar force-pushed the 68817-spacer-height-limit branch from 1d10e3d to 378a1da Compare January 29, 2025 07:24

yogeshbhutkar changed the title ~~SpacerControls: Set default height to 0px when no value is provided~~ SpacerControls: Set height to empty string if undefined and add deprecations for undefined height Jan 29, 2025

carolinan requested a review from t-hamano February 18, 2025 10:24

yogeshbhutkar changed the title ~~SpacerControls: Set height to empty string if undefined and add deprecations for undefined height~~ SpacerControls: default the height to 100px if left unset Feb 24, 2025

yogeshbhutkar force-pushed the 68817-spacer-height-limit branch from 844927f to 679f2e0 Compare February 24, 2025 05:16

t-hamano reviewed Feb 24, 2025

View reviewed changes

packages/block-library/src/spacer/edit.js Show resolved Hide resolved

yogeshbhutkar added 8 commits February 25, 2025 09:36

SpacerControls: Set default height to '0px' when no value is provided

07e498c

SpacerControls: Set height to an empty string when no value is provided

e952a2d

fix: deprecation logic to add height if height & width missing

92ccb6e

refactor: minimize verbosity while exporting deprecations

7639cb6

fix: add width property to spacer deprecation schema for v2

738be43

fix: set height to an empty string when nextHeight is null or undefined

f09d7a7

fix: add 100px as the default fallback

59d32a4

refactor: introduce DEFAULT_HEIGHT constant for consistent height u…

b115447

…sage

yogeshbhutkar force-pushed the 68817-spacer-height-limit branch from 219cde8 to b115447 Compare February 25, 2025 04:07

yogeshbhutkar requested a review from t-hamano February 25, 2025 11:10

t-hamano reviewed Feb 26, 2025

View reviewed changes

packages/block-library/src/spacer/edit.js Outdated Show resolved Hide resolved

fix: add DEFAULT_HEIGHT fallback for vertical flex containers

1aecb63

yogeshbhutkar requested a review from t-hamano February 28, 2025 03:49

t-hamano approved these changes Feb 28, 2025

View reviewed changes

refactor: apply DEFAULT_HEIGHT to missing places

ce92005

t-hamano merged commit 31ee991 into WordPress:trunk Mar 2, 2025
59 checks passed

github-actions bot added this to the Gutenberg 20.5 milestone Mar 2, 2025

t-hamano mentioned this pull request Mar 5, 2025

Spacer: Spacer blocks in Row/Stack layouts may break when upgrading from WP 6.7 #69414

Open

6 tasks

This comment was marked as resolved.

Sign in to view

yogeshbhutkar mentioned this pull request Mar 5, 2025

Spacer Block: use flex-basis to apply DEFAULT_HEIGHT in Flex Containers #69425

Closed

yogeshbhutkar mentioned this pull request Mar 6, 2025

Spacer: Revert height defaults to 100px if left unset #69450

Merged

	hasValue={ () => height !== '100px' }
	onDeselect={ () =>
	setAttributes( { height: '100px' } )

SpacerControls: default the height to 100px if left unset #68819

SpacerControls: default the height to 100px if left unset #68819

Uh oh!

Conversation

yogeshbhutkar commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What, Why and How?

Testing Instructions

Screencast

Uh oh!

github-actions bot commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yogeshbhutkar commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stokesman left a comment

Choose a reason for hiding this comment

Uh oh!

yogeshbhutkar commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yogeshbhutkar commented Jan 27, 2025

Uh oh!

Mamaduka commented Jan 27, 2025

Uh oh!

carolinan commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yogeshbhutkar Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolinan commented Jan 29, 2025

Uh oh!

yogeshbhutkar commented Jan 29, 2025

Uh oh!

carolinan commented Jan 29, 2025

Uh oh!

yogeshbhutkar commented Jan 29, 2025

Uh oh!

t-hamano commented Feb 23, 2025

Uh oh!

yogeshbhutkar commented Feb 24, 2025

Uh oh!

yogeshbhutkar commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-hamano commented Mar 5, 2025

Uh oh!

yogeshbhutkar commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

t-hamano commented Mar 5, 2025

Uh oh!

Mamaduka commented Mar 5, 2025

Uh oh!

t-hamano commented Mar 5, 2025

Uh oh!

Uh oh!

SpacerControls: default the height to `100px` if left unset #68819

SpacerControls: default the height to `100px` if left unset #68819

yogeshbhutkar commented Jan 22, 2025 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading

yogeshbhutkar commented Jan 22, 2025 •

edited

Loading

yogeshbhutkar commented Jan 23, 2025 •

edited

Loading

carolinan commented Jan 28, 2025 •

edited

Loading

yogeshbhutkar Jan 28, 2025 •

edited

Loading

yogeshbhutkar commented Feb 24, 2025 •

edited

Loading

yogeshbhutkar commented Mar 5, 2025 •

edited

Loading