Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zoom layout shift: second alternate fix. #66041

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Zoom layout shift: second alternate fix. #66041

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a75d3ce
Zoom layout shift: second alternate fix.
jasmussen Oct 11, 2024
4f03489
Use full border.
jasmussen Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/iframe/content.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.block-editor-iframe__body {
position: relative;
border: 0.01px solid transparent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment here would help the readability here I believe 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this should be outline rather than border, unless the inset style is preferred?

Border (trunk) Outline
Screenshot 2024-10-23 at 10 49 24 am Screenshot 2024-10-23 at 10 49 18 am

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, ignore me please - refreshing and updating trunk helps 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still use outline - the border is causing some minor layout discrepancies:

Kapture.2024-10-23.at.10.55.47.mp4

Copy link
Contributor

@stokesman stokesman Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

display: flow-root is looking good from my testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

box-shadow is also an alternative to display a visual border without causing layout shifts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the goal was to prevent margin collapsing rather than displaying a border. The border in this PR is transparent.

See related:

}

.block-editor-iframe__html {
Expand Down Expand Up @@ -32,8 +33,6 @@

body {
min-height: calc((#{$inner-height} - #{$total-frame-height}) / #{$scale});
display: flex;
flex-direction: column;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? I believe this will break filling the page to fit the canvas height, cc @richtabor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

zoom-out-glitch

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
Screenshot 2024-10-16 at 17 24 47 Screenshot 2024-10-16 at 17 22 38

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes if the problem is only in the post editor, we should only remove it in the post editor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

abstracted

I'd be happy to restore the rule so it exists both zoomed and unzoomed. But the layout shift has to be fixed somehow.

Copy link
Contributor

@stokesman stokesman Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@stokesman stokesman Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


> .is-root-container:not(.wp-block-post-content) {
flex: 1;
Expand Down
Loading