Zoom layout shift: second alternate fix. #66041
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +9 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for the persistence here @jasmussen! To me this is the right fix to get in for WP 6.7: it fixes the layout shift and addresses the width issues caused by the presence of the flex rule, as @stokesman has highlighted in a couple of places (e.g. #65627 (comment))
This PR does effectively revert the change in #59512 that expanded the main content area while in zoom out mode. I think this is a good trade-off for 6.7 IMO. Here's how it looks in this PR (note the main Type / to choose a block area does not expand on zoom out):
2024-10-14.14.10.10.mp4
I'll just add a few other folks as reviewers for a second opinion here in case my assumptions here are at all off, as see there's some related discussion in #65627. Overall, though, I like the idea of this sort of simple CSS change 👍
Adding a tentative approval as I think this would be really good to get in.
andrewserong
requested review from
draganescu,
getdave,
talldan,
richtabor,
stokesman and
t-hamano
| @@ -1,5 +1,6 @@ | |||
| .block-editor-iframe__body { | |||
| position: relative; | |||
| border: 0.01px solid transparent; | |||
There was a problem hiding this comment.
A comment here would help the readability here I believe 👍
There was a problem hiding this comment.
Wondering if this should be outline rather than border, unless the inset style is preferred?
| Border (trunk) | Outline |
|---|---|
 |
 |
There was a problem hiding this comment.
Actually, ignore me please - refreshing and updating trunk helps 😄
There was a problem hiding this comment.
I'd still use outline - the border is causing some minor layout discrepancies:
Kapture.2024-10-23.at.10.55.47.mp4
There was a problem hiding this comment.
I don’t think outline can work. The border is to cause margins of children to be contained something outline won’t do. That is this issue will return:
outline-v-margin.mp4
But it may be we need an alternative to using the border as well. Good spotting this Ramon.
There was a problem hiding this comment.
The border is to cause margins of children to be contained something outline won’t do.
Ah gotcha, thanks for the explainer.
I was testing in the site editor where the effect wasn't so pronounced.
I wonder if there's another property to prevent margin collapsing, e.g, display: flow-root;
There was a problem hiding this comment.
display: flow-root is looking good from my testing.
There was a problem hiding this comment.
box-shadow is also an alternative to display a visual border without causing layout shifts
There was a problem hiding this comment.
I think the goal was to prevent margin collapsing rather than displaying a border. The border in this PR is transparent.
See related:
kevin940726
added
the
Backport to WP 6.7 Beta/RC
|
I added the backport label since it seems to target the 6.7 release. |
|
I'm going to go ahead and merge this, because we need this in for zoom to make sense. Hopefully it can be upgraded and improved with additional patches later. |
Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: kevin940726 <[email protected]>
github-actions
bot
added
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: ecd845c |
|
This fix is having a side effect on patterns preview.
|
|
@madhusudhand Is this problem occurring on the trunk? I've tried several patterns but I can't reproduce it. However, I've seen this kind of strange behavior a few times in the past. |
Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: kevin940726 <[email protected]>
| @@ -32,8 +33,6 @@ | |||
|
|
|||
| body { | |||
| min-height: calc((#{$inner-height} - #{$total-frame-height}) / #{$scale}); | |||
| display: flex; | |||
| flex-direction: column; | |||
There was a problem hiding this comment.
Why was this removed? I believe this will break filling the page to fit the canvas height, cc @richtabor
There was a problem hiding this comment.
The context is in #65915. Either these rules persist on body, both zoomed in and zoomed out, or they are removed. Having them just in one case, causes the layout shift shown in 65915.
There was a problem hiding this comment.
I can reproduce it from the commit right before this, but I think this didn't happen with zoom-out mode with TT4 just a few months ago. Maybe something changed that caused this glitchy behaviour.
The problem is that now the footer will no longer be pushed to the bottom, so we'll have to revert these lines in particular.
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
It's fine in the site editor, it's the post editor that's the problem due to how the Title block has margins that collapse.
There was a problem hiding this comment.
The problem is that now the footer will no longer be pushed to the bottom
I think it was an understood tradeoff. The flex display on the body was also the cause of #63519 so bringing it back brings that back as well.
There was a problem hiding this comment.
Yes if the problem is only in the post editor, we should only remove it in the post editor
There was a problem hiding this comment.
I didn’t mean to disparage anything by calling it a tradeoff, but fixing a couple things while knowingly breaking another seems to fit the definition to me. Though, yes, perhaps it doesn’t have to break.
Though I find it odd that for that feature we justify layout shift, since that’s what pushing the footer is. It would seem better if there were a way to orchestrate it after the zoom out transition and include some helpful indicator like expanding one of the insertion points.
There was a problem hiding this comment.
Clarifying further, theoretically it could also be the site editor. It's any editor where the first block uses a top margin. Any such margin will collapse when switching between flex and not block. That also means in the post editor, if you turn "Show template" on or off, you can switch between maybe having top margins or not:
I'd be happy to restore the rule so it exists both zoomed and unzoomed. But the layout shift has to be fixed somehow.
There was a problem hiding this comment.
I'd be happy to restore the rule so it exists both zoomed and unzoomed
Maybe I am missing something but to add a such a rule on the body (especially unzoomed) seems contrary to the isolation of UI and theme styling that the iframe is supposed to provide. Do we mandate that block based themes do not control body styling? If so, I guess that’s what I'm missing. Otherwise, it seems like a non-starter—i.e. we should not add the rule (at least while unzoomed, but I have my doubts it’s proper even when zoomed).
There was a problem hiding this comment.
The problem is that now the footer will no longer be pushed to the bottom
I’ve opened up #66388 to restore the feature in a different way that should be less liable to have other impacts. I already requested a review from some of the folks here but I'm mentioning it in case anyone else cares to have a look.
Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: kevin940726 <[email protected]>
|
In resizable editors, the top border appears to cause an unintended vertical scrollbar. Please see #66297. |
Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: kevin940726 <[email protected]>
What?
Alternative to #65915, and #65967. Removes flex from the zoomed out body element, replaces with a transparent border to prevent margin collapsing from causing a layout shift:
Testing Instructions
Go to the post editor, zoom out. There should be no layout shift.