Fix/expanding thumbnails

[ixkaito]

Description

Resolves #34729.

How has this been tested?

Tested with TT2 and TT1.

Screenshots

Types of changes

Bugfix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
• I've updated related schemas if appropriate.

[ixkaito]

For more information about the transform property:

For example, contentHeight is 100, and scale is 0.1. The height of .block-editor-block-preview__content would be 1px because the height is 100 * 0.1 and then transformed by scale(0.1), but it shouldn't. What we want to transform is not the container but the iframe.

[jasmussen]

Thanks so much for this PR.

For example, contentHeight is 100, and scale is 0.1. The height of .block-editor-block-preview__content would be 1px because the height is 100 * 0.1 and then transformed by scale(0.1), but it shouldn't. What we want to transform is not the container but the iframe.

Interesting. It sounds like this might potentially explain the expanding vh unit thumbnails: the scale prop combined with the height provides enough opportunity for browser rounding errors that a 100vh element inside can grow a fraction, prompting a new rounding error, and so. Is that your impression as well?

We have a CSS bandaid in place. But for any future fixes, ideally we can fix the issue in a way so that no CSS can break it, because themes can load anything into the editing canvas.

[ixkaito]

@jasmussen

Interesting. It sounds like this might potentially explain the expanding vh unit thumbnails: the scale prop combined with the height provides enough opportunity for browser rounding errors that a 100vh element inside can grow a fraction, prompting a new rounding error, and so. Is that your impression as well?

Unfortunately, this doesn't solve the expanding thumbnails issue. This is just the way it should be.

We have a CSS bandaid in place. But for any future fixes, ideally we can fix the issue in a way so that no CSS can break it, because themes can load anything into the editing canvas.

That's for sure. I left the MAX_HEIGHT just in case. And I added some inline documentation for the additional CSS.

[jasmussen]

Nice, thank you for the clarification.

I do still think we need to look at the source reason for why there is what appears to be a race condition with the expanding patterns, and fix that, separately. But as an improvement and code improvement, I think this one would be great to get wide testing across a bunch of themes, so we could land it. Thank you!

[ixkaito]

Thank you, @jasmussen

The following image shows the reason for the race condition with the expanding patterns.

The race condition occurs with full-height blocks with vertical margins and also blocks with a height over than 100vh, so it seems that we still need to set a max height to the viewport (iframe).

[jasmussen]

That's a really useful illustration, thanks a ton! CC: @Mamaduka

It seems like:

• The CSS to zero out top and bottom margins on the top level block, as already present in this PR, does matter, and helps explain the discrepancy between classic and block themes. But I would move this outside of classic.scss, because block themes can have margins applied by themes or plugins, so no reason to limit it.
• It seems like we need to throttle the container resizing. If the illustration is accurate, it does explain the infinite growth, and we probably do want to let the container resize at least once or twice to account for loading.

[ixkaito]

I would like to split my PR because it had two different topics. I moved the scale() issue to #38543.

This PR doesn't completely fix the expanding issue, but it makes pattern preview look better.

[jasmussen]

Per the latest change (thank you), this is a small PR that brings important improvements (see this comment, notably), and I'd love for us to land it and backport it. The extra CSS, to me, looks good enough, and I'd be happy to give this a green check, but I'd love a quick sanity check. Maybe another look, @Mamaduka? Or perhaps some people I know have pattern skills, such as @MaggieCabrera or @scruffian?

[Mamaduka]

Thanks for splitting changes into to PRs, @ixkaito.

The changes test well for me. But it would be great to get feedback from Maggie and Ben.

[scruffian]

If we're going to zero the margins I think we need to do it for the padding too, otherwise a full width block doesn't appear full width:

[skorasaurus]

Is this PR still applicable or useful since the Issue that it addresses - #34729 has been closed?

[jasmussen]

Yes, it is my impression that this PR is still needed. The reason why is well illustrated in this comment, but essentially the worst aspect of the bug has been fixed, but this PR offers enhancements to the general behavior.