Fix auto-size patterns triggering scrollbar flickering on certain size #52921
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
kevin940726
added
[Type] Bug
kevin940726
commented
Jul 25, 2023
| // Those inline styles are calculated to fit a perfect thumbnail. | ||
| width: 100%; |
There was a problem hiding this comment.
I'm not sure if this breaks other usages of this component elsewhere. I've tested it both in the patterns screen and in the inserter, but there might be more edge cases?
|
@t-hamano Could you be kind enough to help me test this on Windows? 🙇 Since you're the original reporter and you use Windows. Thanks! |
|
Size Change: +523 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Jul 25, 2023
t-hamano
reviewed
Jul 25, 2023
aaronrobertshaw
approved these changes
Jul 25, 2023
There was a problem hiding this comment.
This tests well for me with @t-hamano's suggested tweak to prevent the Infinity aspect ratio value.
I couldn't spot any issues across browsers, Mac or Windows (only VM I have setup). The previews in the inserter and pattern lists (e.g. query block) all look the same before and after this PR.
Nice work @kevin940726 👍
|
Flaky tests detected in 83a806e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5662986379
|
|
Let's try this. If something breaks, someone will yell at this 😆 . |
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.
What?
Fix #52027. Prevents
BlockPreviews from flickering on certain viewport sizes.Why?
The issue happens when the scrollbar has a width and causes the viewport width to decrease when there's scrollable content.
<BlockPreview>component uses dynamicheightto auto-resize itself based on the preview content's height and container's width. The formula for calculating the height isheight = contentHeight * (containerWidth / viewportWidth)whereviewportWidthis often a constant integer andcontentHeightalso doesn't change for each pattern.containerWidthto decrease a little bit.containerWidthgets smaller, based on the formula in (1.), the height also gets smaller.<BlockPreview>gets smaller in height, the scrollable content can now fit in the screen and doesn't require a scrollbar.containerWidthnow also grows bigger.containerWidthis now bigger, based on the formula in (1.), theheightalso grows bigger. The grown amount causes the content to overflow, and the scrollbar appears again.How?
The solution might not be obvious, but excluding the scrollbar width right before the content overflows magically solves the issue. The key is that if the delta of height isn't enough to cause the transition of the width change, then it won't cause the flickering. I believe it can be proved with some math but I'd rather not go into that rabbit hole 😅.
Surprisingly, browsers natively handle this for us. The only gotcha is that we have to define
widthinstead ofheightfor some reason. Hence the solution in this PR is simply to usewidth: 100%andaspect-ratioto mimic the same behavior with dynamic height. It's also possibly more performant as we only change compositor-only properties.I've only tested this on macOS though, I'm not sure if other OSes behave differently. Could someone with access to Windows or Linux help test this?
Testing Instructions
Testing Instructions for Keyboard
Repeat the same above but use keyboard to resize the window (I don't know how to do that?).
Screenshots or screencast
Before
Kapture.2023-07-25.at.14.26.20.mp4
After
52027-after.mp4