Fix: Cover Block: Default height cuts off taller content.

[jorgefilipecosta]

Description

Fixes: #17339

This PR fixes a problem introduced during the addition of cover resizing in #17143.

The resizing functionality should set a min-height to make sure we don't cut content, on the front end everything worked as expected, but on the editor, given the way, re-resizable works content may end up cut.

We were also relying on a hardcoded default height; themes may set their preferred height, so this PR also removes the usage of a default height.

How has this been tested?

I added a cover block I added some content inside.
I verified when the resize finishes if it made content not appear, the height automatically increases, to make sure content is always shown.
If I remove content, the height decreases until the min-height that was previously set.
The resizing operation is smooth, and after resizing to several different values, when I start resizing again, there are no "jumps" in the dimensions.

[senadir]

Hi Jorge, thank you for working on this, there a bit of introduced problems, and I'm fully sure why you introduced some things, I will still look into this issue from my side and see if we can fix it without breaking other things.

[jorgefilipecosta]

Hi @senadir, thank you for your review, the problems and suggestions you identified/proposed were addressed.

This problem ends up being complex, and before this solution, I tried many others without success.
Re-resizable always sets a height style value and needs a height during the resizing to work.
We can not set a height we need to set min-height to make sure we don't cut content.
This PR adds an important CSS rule to remove the height inline style set by Re-resizable (excluding when resizing).
If a height value is not passed, re-resizable starts resizing from the true height of the element. So if an element has min-height 50px but needs 1000px when resizing, we will start from 1000px and not from 50px to avoid a jump.
When resizing starts, we add a class to make sure the height set by resizing is applied (we need to allow its application).
During the resize, we set a temporary state value, and at the end, we change the attribute.
Changing the attribute during the resize instead of a state value seemed unreliable. I think when we change attributes, a significant block rerender happens. If some dom element resizable is depending on rerenders during a resize, things may not work as expected.
When the resizing stops we commit the attribute change, we remove the temporary resize value, and we remove the is-resizing class. So at that time, the style rule applied will be a min-height again.
I hope I managed to explain what's bewind the solution I'm proposing if you have any other question/thought feel free to ask/share.

[senadir]

I've checked all the changes and it seems they work fine, I would appreciate another eye on this just to be sure I didn't miss anything.

There is however one tiny problem, not a big deal really, when you try to resize the block to something below its height, the block snap back but the input stay at that value, here is a gif.

this problem happens since onResizeStop update the attribute with elm.clientHeight but the problem, clientHeight immediately change after to the actual height.

This issue is easily fixed by removing the class before reading the height, might use a callback on setIsResizing to be safe but as far as I've seen all of those functions are synchronous so no need.

const onResizeStopEvent = useCallback(
	( event, direction, elt ) => {
		setIsResizing( false );
		onResizeStop( elt.clientHeight );
	},
	[ onResizeStop, setIsResizing ]
);

[jorgefilipecosta]

Hi @senadir thank you for the review!

There is however one tiny problem, not a big deal really, when you try to resize the block to something below its height, the block snap back but the input stay at that value, here is a gif.

this problem happens since onResizeStop update the attribute with elm.clientHeight but the problem, clientHeight immediately change after to the actual height.

This behavior is intended. If we have a content height of 700px, if we set in case A the minimum height to 500px, and in case B to 700px, case A and B look the same. But if in case A we remove some content (remove blocks) the height will decrease until the 500px minimum while case B removing content does not decrease the height.

The case where we resize below its height is case A and although after resizing the minimum height setting takes effect and it expands tbe height, the input should stay the same because that's the minimum height attribute of the block.
I guess we should change the label of the field to reflect that the user is setting a minimum height and not height, but I will propose that in other PR to keep discussions focused.

[senadir]

took me a couple of minute to understand that, but now I get it, it's actually brilliant way to do it, but I'm afraid it will be confusing.

I guess we should change the label of the field to reflect that the user is setting a minimum height and not height

I think we should

but I will propose that in other PR to keep discussions focused.

yes, let's not block this.

[jorgefilipecosta]

Given that a very important release is approaching I'm merging this PR so we get as much as testing as possible. If there are other comments anyone feel free to share and we will iterate on this.