Spacer Block - Allows serializing heights greater than the maximum of 500px.

[ttahmouch]

Describe the bug
Contrary to the suggestion in the Deprecation Notice, it was not possible to create a Spacer in WP.com first with a value higher than 500px using the GUI (as it would force adjust it to 500px in the text input) or using the HTML editor (as it strips out <!-- wp:spacer {"height":"1600px"} --> back to <!-- wp:spacer --> when you attempt to save the post). If I modify it to <!-- wp:spacer {"height":"1600px"} --> and update the post in WP-iOS instead of WP.com, it saves correctly with the added JSON object in the HTML comment, but it renders with an error in the WP.com editor and only previews correctly in Tablet and Desktop mode; not Mobile mode. From parallel conversation, there may be recent changes that have been made to the block parser that have caused this regression regarding stripping the metadata from the HTML comments (but we haven't attempted to dive into it yet). It does render correctly in the WP-iOS editor if I modify to 1600px in the GUI text input; just not Mobile preview mode.

• For More Possibly Related Information

Sanity Tests

To Reproduce

• Spacer block - Settings: Height range extends if Spacer comes from the web is higher than 500px - TC005

Expected behavior
Should serialize the spacer block HTML correctly? I'm honestly unsure if this is a defect given the change of requirements to not support Spacers larger than 500px.

Smartphone (please complete the following information):
iPhone 13 Pro Max (15.1.1)

Additional context:
Sanity Test - 1.69.0

[ttahmouch]

I'm fairly confident this is not a defect with Gutenberg-Mobile and WP.com. It's just an awkward situation. If I remember correctly, WP-iOS+GBM rendered correctly if the post markup was modified locally because it would serialize the markup with the {"height":"1600px"} metadata when manually inputting 1600 into the text field. WP.com would just render it as an error in the editor presumably because it wasn't expecting a valid markup to include a {"height":">500px"}.

The only awkward part that I mentioned above is that a {"height":">500px"} rendered fine in WP.com Tablet and Desktop modes, and not Mobile. However, if this is not a valid state anyway since the editor would be expecting the value to be <=500px this doesn't feel like a defect with WP.com.

I'm inclined to close this instead of transferring it to Gutenberg. Do you agree?

[SiobhyB]

I came across the same while testing the most recent release of the Android app.

It's possible to increase the height of the spacer block beyond 500px in the app, meaning both of these two test cases currently "fail":

• https://github.com/wordpress-mobile/test-cases/blob/trunk/test-cases/gutenberg/spacer.md#tc004
• https://github.com/wordpress-mobile/test-cases/blob/trunk/test-cases/gutenberg/spacer.md#tc005

As this matches with the web's behaviour, I'd also be inclined to close this issue and update the tests, as it doesn't seem like a bug. I could perhaps see a case for wanting to limit the height setting on mobile, though.

[mchowning]

Just my thoughts on this...

With respect to test case TC004:

1. Add a Spacer block
2. Check if available Spacer height range to set via slider in options is between 1px and 500px

As this matches with the web's behaviour

Could you elaborate a bit on that @SiobhyB . I'm seeing on the web currently that anything over 500 gets reset to 500 automatically, but on mobile we allow those higher numbers, which seems different. Or am I misunderstanding?

Regarding testing spacer heights of more than 500px on mobile (TC005), it feels like this test is still "valid" since presumably it was possible to do this on the web at some point even if we can no longer do it from web (even using the HTML editor), and I can now (it seems) test this on mobile by manually entering a >500px spacer height and observing if the spacer adjusts its height appropriately.

With that said, I don't think it's worth us keeping this test case since it is addressing functionality (setting spacers with >500px from the web) that was removed around 2 years ago (based on when the deprecation notice was added to the test case). We should also update the spacer on mobile to not allow sizes greater than 500px like this issue suggests though.

[SiobhyB]

Could you elaborate a bit on that @SiobhyB . I'm seeing on the web currently that anything over 500 gets reset to 500 automatically, but on mobile we allow those higher numbers, which seems different. Or am I misunderstanding?

Ah, it's being reset on your side? I haven't been able to replicate that, I'm able to set the number to higher than 500 without issue. For example, I set the spacer's height in the screenshot to 1000px and it stayed at that height after saving, exiting, then re-opening the post on the web. I then opened the post in the app and it was still 1000px there. 🤔

When are you seeing the reset happen?

[mchowning]

Interesting. I see what you mean now, thanks for explaining. I was typing a number into the spacer height field, and then pressing Enter. When I pressed Enter is when it resets down to 500 if it is over 500. But if I don't press Enter, then you're right, it doesn't reset and you can set a height of over 500

Screen.Recording.2022-02-09.at.8.29.56.AM.mov

I tried to do a bit of digging to see if the spacer is supposed to be limited to 500px, and I'm not finding any clear indication that it should have that limit. @SergioEstevao added the const MAX_SPACER_HEIGHT = 500 line in this PR, but that PR also has a commit titled "Update code to now allow spacer larger than 500px.", which was added after the commit that added the MAX_SPACER_HEIGHT const.

I think that we should

1. First, confirm whether or not the spacer height should be limited in general (I left a message in the #core-editor Slack channel asking for input). Personally, I don't see any reason to limit the height to 500.
2. And then, update both mobile and web to respect that limit (or lack of limit).

[SiobhyB]

Oh, thanks for explaining that Matt! I hadn't been hitting enter, but am seeing what you describe now and all is clear.

If I test on version 5.9 of WordPress then I also see the 500px limit imposed there, regardless of whether the enter button is hit.

spacer.mov

It's only with the latest development version of Gutenberg installed, with the latest changes to the spacer block's height control, that I'm able to set the height above 500px.

With all that considered, it does seem likely that there should be a limit and there's a bug at play. I'll follow along with the message you've started 🙇‍♀️

[SergioEstevao]

has a commit titled "Update code to now allow spacer larger than 500px.", which was added after the commit that added

This may have been a typo on my side I think I meant Update code to NOT allow spacer larger than 500px

That said I don't recall the reason why there was a limit of 500, maybe something to do with screen size limitations on mobile?

[paaljoachim]

Hey @mchowning brought this up right before Wednesdays Core Editor chat.
https://wordpress.slack.com/archives/C02QB2JS7/p1644415002737119

I also checked for desktop and the Spacer block also has an upper limit to 500px there.
The latest comment from Joen in the slack thread is this: "Seems fine to do a simple PR to bump that very high."
I assume that a developer can then create a PR and perhaps bump it up from 500px to 2000px for desktop/mobile etc.

[mchowning]

Thanks @paaljoachim for following up! 🙇

It sounds like there is supposed to be a limit. I don't have a strong reason to change it from 500px to something else (although It sounds like there's nothing stopping us from increasing it if there's a reason).

It's possible to increase the height of the spacer block beyond 500px in the app, meaning both of these two test cases currently "fail":

https://github.com/wordpress-mobile/test-cases/blob/trunk/test-cases/gutenberg/spacer.md#tc004
https://github.com/wordpress-mobile/test-cases/blob/trunk/test-cases/gutenberg/spacer.md#tc005

WIth respect to the manual test cases on mobile, I would lean toward removing the test case for checking that we support heights over 500px on mobile (TC005). Since that height isn't intended to be supported on web, I don't think supporting that is an edge case we need to regularly test manually on mobile, especially since we still have a test case to confirm that mobile is supporting the proper range of spacer heights (TC004). TC004 itself might be a "failing" test in that case since it doesn't look like we're currently limiting the spacer height to 500px on mobile. wdyt?