SpacerControls: Set height to empty string if undefined and add deprecations for undefined height

[yogeshbhutkar]

What, Why and How?

Fixes: #68817

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:

1. 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"

2. 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

1. Create a spacer block.
2. Set its height to undefined by trying to save an empty input.
3. 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]

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: 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]

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]

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]

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

[Mamaduka]

@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]

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]

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]

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

[yogeshbhutkar]

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]

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]

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 ✨.