Zoom layout shift: Alternate fix. #65967
Conversation
|
Size Change: +4 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Thanks for the PR! The flex styles were originally applied to the |
Unclear, my read of the flex rules is they exist in part to update the height of the body element to match that of the HTML element. Commonly that needs rules on both html and body. Though if in your testing, the net result is the same when applying it only to the html element, perhaps that's safer! Feel free to push if you have something working. |
There was a problem hiding this comment.
I like this option better because it doesn’t seem to affect existing theme visuals. It also fixes #63519 from my testing.
It does currently break the "expand main content area" feature added in #59512. I think that’s acceptable and, if so, then we can probably clean up some of the other styles it added as well and maybe some of the JS. Otherwise, I’ve yet to see a way of keeping it without this layout shift being an issue. There are surely alternative approaches to that feature anyway.
|
I like the approach here, too. In regards to:
I found testing locally that this feature could be restored by using Aki's idea in this comment: #65967 (comment) (using the 2024-10-10.15.59.00.mp4I've pushed to this branch since it sounds like @jasmussen was happy for others to jump in. Feel free to revert if it isn't working for anyone, though, but this appears to be testing pretty well for me now! What do you all think? |
andrewserong
added
[Feature] Zoom Out
Backport to WP 6.7 Beta/RC
|
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. |
|
I'm always happy for others to jump in. Hurrah for open source. Thank you! Ultimately I'd be happy for a technical review/testing of this: my read is that these flex rules are "safer" to use than the border hack. But whether we use one or the other, I'd suggest either is important to land as a bugfix, if zoom is to ship. |
There was a problem hiding this comment.
To me, I think this approach is the best approach for WP 6.7 and has the least scope of impact.
I haven't found any side effects from using the .block-editor-iframe__html body selector, but I'm in favor of shipping this PR unless others have concerns about using this selector.
|
Thanks for the review, @t-hamano! Did you mean I should update this PR to use a different selector, or that this is good to ship as is? If the former, feel free to push to this branch. Or feel free to merge this and follow up. I'm a little under the weather today, so mostly AFK. |
I think it's okay to ship as is. The selector has already been updated by @andrewserong (231d06f) @andrewserong @stokesman Is my understanding correct? |
|
The latest commit recreates the same visual change when not zoomed out that I thought we should be wary of on the first try at this. 65967-visual-difference.mp4Also, I’d said this happens to fix #63519, but now it’s the opposite it makes that issue more prevalent because it appears even when not zoomed out. To see you have to be in the Post editor (on a post too, not a page). 65967-post-editor.mp4I don’t think we can keep that latest commit. Maintaining the "expand content area" feature is probably still possible by putting Yet, I’d prefer if we could forgo that feature for now as it’s current implementation is the cause of #63519 and the layout shift that this sought to fix (we still need the |
|
Thanks for clarifying @stokesman!
I wonder if we can have the rule at the |
It should work and prevent this PR from "regressing" the expand content feature. |
I just tried this out (committed in 79b8fe3), adding the rule to the body only if zoomed out. Unfortunately that adds back in a visual "jump" when activating the zoom out mode due to the change from margin collapse to no collapse (not as bad as on trunk, but it's still present): 2024-10-11.11.15.59.mp4So I'm not sure if that's the right approach, either, but thought it still worth committing to this branch so that we can test it. Unfortunately I'm not too sure which approach here would be best for now. In some ways I'm wondering if the original commit (786f8c9) is the best way to go for now... though as you've mentioned @stokesman that would mean regressing #59512, which would be unfortunate. What do you think @stokesman and @t-hamano? Apologies if my committing here has just made the conversation more complex! 😅 |
|
Flaky tests detected in 79b8fe3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11284055221
|
|
Update: I did some more hacking around such as attempting to add the following to fix the jump on the title wrapper in the video above: .block-editor-iframe__html.is-zoomed-out {
.editor-visual-editor__post-title-wrapper > .wp-block-post-title {
margin-top: 0;
}
}I think that would be a bit too hacky, though, and still doesn't address the issues of using So, I think I'd personally be in favour of returning to the original commit in this PR. As Mitchell mentions, if we do that, though, we would be removing the feature introduced in #59512 which expands the main content area to the size of the viewport when zoomed out. @richtabor and @MaggieCabrera — how important is that feature (expanding the main content area to the size of the viewport)? If it isn't too important then we could revert this PR to the original commit and ship it. If it is important, then I suppose the hack above might be okay to go with, though we'd still need to come up with a fix for #63519 — which the original commit in this PR resolved 🤔 I'm at the end of my week now. Sorry I didn't manage to get this into a neat shape! Happy to keep hacking away on Monday if this PR is still open by then. |
|
The added margin happens because it prevents margins from collapsing. So a margin on the header, and a margin above the header block wrapper, instead of collapsing they stack in a Flex context. In fact that same thing happened with the border fix I tried. Border, flex, abs-positioning, float, all of those stop collapsing. Going deeper, the shift happens in the first place because the zoomed version is flex, while the unzoomed version is not. So yet another alternate fix would be to remove the flex from the zoomed version. Yet, the margin collapsing has its own effect there; notice in this GIF how the content animates correctly, it's just now the header and footer backgrounds collapse.
Since that remains margin collapsing just on the body element, we can try and fix that in a no-flex world, by using borders. Here's with a
Whether that's better or not, unclear, but I went ahead and pushed such an alternative to #66041. |
|
Since #66041 has been merged, I'll remove the backport label for this PR. |
kevin940726
removed
the
Backport to WP 6.7 Beta/RC
|
As this is no longer targetting 6.7 I'm going to remove from the board. |
|
I think this can be closed out now since #66041 landed. I'll close it 🙂 |
What?
Alternative to #65915. Uses flex to prevent margin collapse instead of borders.
Why?
The body and title elements have margin collapsing in the shift from zoomed in to zoomed out, causing a jarring zoom experience in the post editor.
This PR fixes it by applying flex rules to the canvas container always, not just when zoomed in. Abs position, flex, grid, borders, and floats are the options we have to prevent margin collapsing.