Fix pattern preview expanding height and scrollbar issues #38175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Feature] Inserter
Mamaduka
requested review from
a team,
talldan and
carolinan
and removed request for
a team
Mamaduka
changed the title
|
Note: As a future follow-up, we might want to review |
|
Size Change: +78 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
WP Version: 5.9-beta3-52380
Conclusions: Screen.Recording.2022-01-24.at.11.08.21.mov |
talldan
reviewed
Jan 24, 2022
| if ( styles ) { | ||
| return [ | ||
| ...styles, | ||
| { css: 'body{overflow:hidden;}', __unstableType: 'presets' }, |
There was a problem hiding this comment.
I left a comment over here, but just wanted to make sure you see it: #37573 (comment)
I think if we need to override a style in the iframe, it'd be good to also do something like height: unset, the reason being that height: 100% is what's causing the scrollbar for patterns.
I'm not sure if you also need overflow:hidden to fix the other issue. It seems ok in conjunction with overriding the height, but just overflow:hidden on its own causes patterns to be cropped unnecessarily.
There was a problem hiding this comment.
Here's a comparison to show what I mean:
Just overflow: hidden on the preview body:
height: unset and optionally overflow: hidden on the preview body:
It's subtle, but you can see for the second one there's the correct spacing at the bottom of the preview when unsetting the height. My concern is that some patterns may be more affected than others by this effect.
There was a problem hiding this comment.
Thanks, @talldan.
Added the rule to unset height.
Thanks for the video, that's helpful. I want to note I've seen that issue separately from the patterns panel, occasionally in the block inspector as well, so although a frustrating issue, I believe it's a separate one. Can you reproduce that one on trunk as well? In my past experience, the issue "goes away" by simply making the browser taller or shorter. |
|
Thanks for testing, @c4rl0sbr4v0. That might not be related to this fix. I've seen a similar issue before. Unfortunately, we can fix everything an hour before RC4. Can you confirm that PR fixes the original issues? |
|
Thanks for working on this, thanks for pushing the fix. This is working well for me, and substantially better than trunk:
I tested with a number of themes. TT2: TT1 Blocks:
TT1 (classic theme): My own theme which has very crazy no-good margin rules:
One thing to keep in mind is that these thumbnails are subject to the editor styles that the theme loads into them. Which means if the theme (like my own) loads some crazy styles into the editor, they will affect thumbnails as well, and they might not look like "thumbnails". The addition of overflow and max-height rules feel completely sensible to me, and act as guard-rails. For example this prank pattern could wreak havoc without any guard-rails: A 200vh bottom padding would take up all the space of the pattern browser if we don't cap it. 1800px as a value was chosen because in my testing, it was sufficient for every pattern I could see. In the case of the above april fools pattern, the max-height would benefit from the overflow rule, ensuring no scrollbar appeared. Because of the simplicity of the height, overflow and max-height rules added, and because it tests well for me, this feels like a good fix. It could always use a ton of additional testing, looking specifically for patterns that have scrollbars or scale infinitely. In my testing, I couldn't find any. |
|
Thank you, @ixkaito. Do you mind opening a new PR against trunk? We won't be able to ship it with WP 5.9, but if it works, it would significantly improve the plugin |
|
Thank you @Mamaduka ! Of course. I'll do it. |
|
I re-sent the PR to trunk. Please check it. #38181 |
desrosj
pushed a commit
that referenced
this pull request
Jan 24, 2022
* Apply a 99vh max-height to the thumbnail. * Polish * Apply pixel based max-height. * Don't apply overflow auto to all iframes * Memoize editor styles * Remove no need for iframe changes * Unset height * Apply max-height to container * Update max-height Co-authored-by: jasmussen <joen@automattic.com>
desrosj
added a commit
that referenced
this pull request
Jan 24, 2022
…38182) * Apply a 99vh max-height to the thumbnail. * Polish * Apply pixel based max-height. * Don't apply overflow auto to all iframes * Memoize editor styles * Remove no need for iframe changes * Unset height * Apply max-height to container * Update max-height Co-authored-by: jasmussen <joen@automattic.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: jasmussen <joen@automattic.com>
desrosj
added a commit
to desrosj/wordpress-develop
that referenced
this pull request
Jan 24, 2022
desrosj
added a commit
to desrosj/wordpress-develop
that referenced
this pull request
Jan 24, 2022
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jan 24, 2022
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. git-svn-id: https://develop.svn.wordpress.org/trunk@52633 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jan 24, 2022
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. Built from https://develop.svn.wordpress.org/trunk@52633 git-svn-id: http://core.svn.wordpress.org/trunk@52221 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott
pushed a commit
to gMagicScott/core.wordpress-mirror
that referenced
this pull request
Jan 24, 2022
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. Built from https://develop.svn.wordpress.org/trunk@52633 git-svn-id: https://core.svn.wordpress.org/trunk@52221 1a063a9b-81f0-0310-95a4-ce76da25c4cd
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jan 24, 2022
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. git-svn-id: https://develop.svn.wordpress.org/branches/5.9@52634 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jan 24, 2022
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. Built from https://develop.svn.wordpress.org/branches/5.9@52634 git-svn-id: http://core.svn.wordpress.org/branches/5.9@52222 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott
pushed a commit
to gMagicScott/core.wordpress-mirror
that referenced
this pull request
Jan 24, 2022
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. Built from https://develop.svn.wordpress.org/branches/5.9@52634 git-svn-id: https://core.svn.wordpress.org/branches/5.9@52222 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
Just testing in RC4, still a lot of issues 😢 This PR was definitely an improvement though, most noticeably in Gutenberg itself, but there seems to be a larger issue with regards to the way WordPress handles stylesheet concatenations. I am also still seeing some scrollbars so maybe we should keep #37573 open 🤔 |
|
Like @ndiego I've just tested with RC4 without the Gutenberg plugin active and the issues are still present. I used our Aino theme but the 2022 gives a similar experience. The scrollbars are gone, but the height of the patterns is not displayed correctly (some are too short, some stretch longer than the actual height. So now for the short ones no preview is possible at all, see:
|
|
Thanks for testing, @ellenbauer. @ndiego or @ellenbauer, do you mind creating a new issue? I will add to the 5.9.1 milestone and re-post the technical findings of the problem so far. As Nick mentioned, the way WP handles stylesheet concatenations can cause different results. Currently, we can't reproduce the same environment via the plugin. |
@ellenbauer Do they still get cut off if you're not loading extra styling from your plugin? |
|
I'm seeing the same experience with utility styles that are added with Extendify. If I don't have those loaded, then the scaled previews are generated in time for the height to not be cut off. |
7 tasks
gziolo
removed
the
Backport to WP 6.7 Beta/RC
whereiscodedude
pushed a commit
to whereiscodedude/wpss
that referenced
this pull request
Sep 18, 2022
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. Built from https://develop.svn.wordpress.org/branches/5.9@52634
5 tasks
VenusPR
added a commit
to VenusPR/Wordpress_Richard
that referenced
this pull request
Mar 9, 2023
This merges the latest package updates for the block editor to include the fix for WordPress/gutenberg#38175. Props hellofromTonya, mamaduka, joen, talldanwp, cbravobernal, poena. Fixes #54487. Built from https://develop.svn.wordpress.org/branches/5.9@52634 git-svn-id: http://core.svn.wordpress.org/branches/5.9@52222 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #34729
Resolves #37573.
Description by @jasmussen
This PR fixes the scrollbar shown inside thumbnails and applies a bandaid to stop
vhbased patterns from endlessly expanding.In testing, it became clear that 100vh equals the height of your browser. Inside an iframe, things are different. This StackOverflow question led me to this spec note:
That seems to suggest it’s very easy to create a race condition, as “initial containing block” can’t be properly sized until content inside is loaded.
Because of that, this PR applies a pixel based max-height. A tall one, and one that will let the VH based pattern scale until it reaches that max-limit, but nevertheless it should mitigate the issue.
As noted in comments in the code, a better fix might be apply a max-height based on the initial-content height. But due to the iframe scaling behavior, this seems nontrivial
How has this been tested?
I tested with TT2, Blockbase, and Blank Canvas themes. The "vertically expanding" issue was most visible with Blank Canvas.
Screenshots
Try similar testing method with different Classic and Block themes.
Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).