Try Zoom Out improvements #63390
Try Zoom Out improvements #63390
Conversation
|
Size Change: -404 B (-0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
stokesman
force-pushed
the
try/zoom-out-improvements
branch
4 times, most recently
from
0e07d1a
to
94aa467
Compare
stokesman
added
[Feature] Zoom Out
[Type] Enhancement
|
I am pinging some folks who’ve worked on Zoom Out but not asking for a complete review. I’d appreciate any testing or any cursory opinions on how worthwhile this looks. @ajlende, @ellatrix, @scruffian, @MaggieCabrera please ping anyone else if you feel they’d have interest. There’s been a few e2e tests that have failed consistently on this branch but so far they’re mysterious to me. The tests also fail locally but so far I can’t reproduce with manual testing of the same scenarios. It’s also hard to see how the changes here would affect them anyway. I’ll continue looking into it. |
|
Thanks for this PR, @stokesman ! I'm unsure about the margin at the bottom when there's enough content to scroll. It feels like it's cutoff. I'll let @richtabor give design feedback on that though |
|
The overflow is cutting off things like the focus borders of the section and messing with the position of the toolbar. This screenshot is FF on a mac with dark mode enabled and always visible scrollbars, and it looks like this: 
|
|
This is looking good to me. It would be good to get a review from someone who is more familiar with the code. |
Thanks for highlighting this. I thought it would be a good thing in that it better aligns Zoom Out with Device Previews which do the same: UPDATE: 
The toolbar position would probably have to be a follow up because it requires changing |
stokesman
force-pushed
the
try/zoom-out-improvements
branch
from
94aa467
to
b3e4278
Compare
|
Do you think it's work adding the |
Good call, it might be what we need here |
Thanks for the suggestion. I tried it but am uncertain of how advantageous it is in this case. There any several details to consider so I’ll start with my overall take-away. For this PR, I think the best thing would be to remove the current customization of the scrollbar and not add the mixin. That way there are fewer factors to evaluate. The sole purpose of the current customization was to prevent a shrunken scrollbar when transforming the iframe. Yet it only worked in Chrome and when the scrollbar is not overlaid. Finally, a shrunken scrollbar shouldn’t be a blocker (especially in contrast to the inaccessible scrollbar in trunk). Now to detail what I found trying the mixin. Here’s what it does that needs overridden for this case and/or creates a tradeoff (Though none of this seems to apply for overlaid scrollbars):
Here’s a video to help illustrate: zoom-out-custom-scrollbars-on-hover.mp4Oh, one last tidbit. It seems to really have our way with the scrollbar it would require a library like simplebar but the compromise there is a small bundle size increase. |
|
A few notes: 1. Wide zoom feels offWhen I zoom out, I don't expect the site to be fullwidth while in zoom out. It negates the effect of zooming a bit, utilizing space that is unnecessary to cover. The perception of content and spacing seems off, when it can be so wide:
2. Allow full scrollingI don't think we should loose the space above the site when zoomed out, but allow the site to scroll like in trunk, into that space:
3. Distracting omnipresent scrollbarThis scrollbar should not be omnipresent. It is positioned better, but never goes away and is distracting.
4. Improved scalingThe scaling of content is better (testing with this pattern). Trunk has a few oddities, as seen below: Trunk: CleanShot.2024-07-19.at.11.10.03.mp4PR: CleanShot.2024-07-19.at.11.08.36.mp4 |
stokesman
force-pushed
the
try/zoom-out-improvements
branch
from
b3e4278
to
628a357
Compare
|
Thanks for testing this Rich! Pardon the pause on getting back to this one.
Both of those are addressed. The latter had been on my todo anyway.
This one is tricky on this branch. Here are some ways I've tried doing it. I put screen recordings in the details. Put the vertical spacing inside the iframe. This is like how it is in trunk but it makes for an odd scrollbar on this branch. It can be somewhat alleviated by also putting the horizontal spacing inside the iframe but that helps only as long as the viewport is narrower than canvas maximum width.zoom-out-frame-spacing-inside.mp4Shift the canvas vertically per document scroll progress. This one comes close in feeling but it’s not the same thing.zoom-out-parallax-frame.mp4After exploring those, I did have another idea for how it can be made just like trunk and did some dev on it. There is promise but it’s pretty involved and would be better left to a separate PR. There’s also room to try simpler ideas like reducing the vertical spacing or removing it from the bottom edge. |
|
@stokesman I like the big PR for pulling ideas together, but is there any part of this that can be abstracted and potentially pushed as a standalone PR?
I'm thinking this specifically. |
|
I'm missing the fluidity of what's in trunk, when zoom out is engaged: CleanShot.2024-07-31.at.16.55.42.mp4 |
I think this is a good idea. This PR is really big which makes it hard to see how all the changes interact. Maybe if you could break it into smaller PRs it would be easier to review. |
I think the restoration of the maximum width made it quite obvious the iframe no longer had a transition. I restored the transition now. Be on the look out to see if it reintroduced oddities when scaling the content (in regards to "The scaling of content is better"). The transition may be prone to oddities until the final scale can be specified the instant that zoom out is engaged. As of now (and in trunk), the scale is calculated multiple times in response to the the container width changes and each one initiates a transition that’s interrupted by the next until the final one.
I intend to break it up as much as possible. I’ll go ahead and look at what can be isolated but I think it’ll hard to determine and/or limited without knowing how we’ll decide on the pivotal thing here—whether Zoom Out is to keep scaling the |
stokesman
force-pushed
the
try/zoom-out-improvements
branch
from
c1bba0a
to
eafcf8f
Compare
What?
Improvements to “Zoom Out”.
Why?
Initially this was made to improve the centering of the canvas, have the scrollbar visible and stop having the scale change during window resizing. Along the way I found ways to simplify and noticed this may improve some other things like the scroll position and layout parity.
How?
iframeinstead of thehtmlThis wasn’t necessary for most of the simplification or improvements but I realized that with the
transformon thehtml, having the scrollbar stay inside the visible area made the effective breakpoint different when changing between zoomed/unzoomed views. This is because the iframe’s width determines the breakpoint even when its document is adjusted to appear the same width. The only ways I can see to have unchanged breakpoints is to either do like before where the iframe’s width is preserved and thus scrollbars are pushed outside the visible area or to do as in this PR and scale the iframe itself.Testing Instructions
TL:DR; Have the Zoom Out experiment enabled and test everything. You may find something I've missed. What follows are some specific scenarios that should be tested.
vhunitsThere was a time when Zoom Out did scale the iframe and that was changed in #59334 apparently due to an issue with
vhunits #49299 causing the iframe editor content to infinitely grow in height. This branch doesn’t regress the issue from my testing. Here are the testing instructions from that issue.Note the iframe’s content height is stable in this branch
Parity of layout from unzoomed to zoomed
#61424 didn’t have an accompanying issue but it was made to:
In my testing this branch improves layout parity.
No double scrollbars
From issue #61093
This should not be reproducible on this branch. I reverted a bit of code from the PR merged to "fix" that issue.
Maintained scroll position
Drag chip positioning over scaled iframe
Scaling the iframe does throw off the drag chip position when dragging over the iframe and required some changes to compensate. This was a thing experimented with previously #56889 when the scaling was applied to the iframe.
Expansion of main content area in Zoom Out
See #59512
General canvas scrolling
This PR removes some wrapping elements whose introduction seem to have been the cause of some recent canvas scrollbar issues across different modes of the editor. They all seem to have been fixed so it’s important to test all permutations to make sure all content is scrollable and there are no extraneous scrollbars.
wp.data.dispatch( 'core/notices' ).createInfoNotice( 'Notice' );)Testing Instructions for Keyboard
Screenshots or screencast
Layout parity
trunk-zoom-out-max-width-layout.mp4
63390-zoom-out-max-width-layout.mp4