Refactor Zoom Out to scale iframe
#63390
Conversation
|
Size Change: -679 B (-0.04%) 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
|
Thanks for working on this! From some initial testing, scaling the iframe does seem to address a lot of issues. Some issues I see from this over trunk:
I'm not sure which items are going to be the most important, or if we can get all of the things without making some concessions. |
|
Hi folks, pardon my slow response here. I’ve not thought this should be urgent so I’ve let it sit. @getdave, thanks for having a look and reading the prior comments.
I wonder how that’s concluded. As far as I can tell, the current state is a happy accident (first appearing in #59334) not a design decision. That’s not to argue it doesn’t have value but that it would seem open to consideration.
To clarify, my comment was not about whether the canvas is an iframe. It was about applying @MaggieCabrera, thanks for testing again and finding those issues.
So far I haven’t been able to reproduce.
I fixed the horizontal scrollbar. I removed the shadow since it’s not a thing on trunk; was causing issues when I tested in Firefox and does jar with the inserter separator’s current intended appearance as a gap. The scrollbar’s part in that at least is largely not an issue on devices that only show them when in use.
I’m glad you mention this. Back when the PR was first made Zoom Out completely hid the scrollbars so the shrunken scrollbar seemed less bad in comparison. Now, on trunk the scrollbar can still end up hidden so I'm not sure there’s a clear winner on this point.
Originally I started this to keep the scrollbar visible and improve the centering of the canvas when the scrollbar is not overlaid. The first commit does that without changing where the From there, I discovered it facilitates fixing or allows simplifying additional things, among these:
@jeryj, Thanks for giving this a look.
This is concerning, but I'm not seeing it. Granted my local branch is diverged so maybe it was fixed. I’m pushing my latest changes soon.
These two are technically tied together and I ❤️ that you mention the latter point. It’s the feature I would most wish to keep. Yet I'm not sure I see it as essential. It’s one thing you can say that comes for free with scaling the |
stokesman
force-pushed
the
try/zoom-out-improvements
branch
from
3506374
to
3300cee
Compare
stokesman
force-pushed
the
try/zoom-out-improvements
branch
from
3300cee
to
79ba58a
Compare
|
I've been playing with this and looking into it with @MaggieCabrera and @ajlende. I think the scrollbar being within the iframe instead of being able to scroll the gray area is a dealbreaker. I do appreciate that moving the scaling to the iframe does make a lot of other things simpler though. Open to ideas on how to sync up that scrolling with the outer wrapper, but it feels like that will add a lot of complexity and get us in a similar spot to where we are now. We're focused on trying to get smaller fixes to the existing issues with the current implementation. Your PR for fixing the scaling issue is really promising, so we'll take a look at that now. Let's keep working on finding the best solution for this overall after the 6.7 release though. This |
I do appreciate this concern and, as I said, having the whole gray area activate scrolling is my favorite thing about the current state. Yet I think it’s worth recognizing that it was not designed that way. It was a happy accident that came about with #59334 so it doesn’t seem like a must have. Consider also that none of the other "framed" views in the editor scroll like that (Device previews or other focused editing views) and making them do so is certainly non-trivial.
That’s understandable and why I’d mostly been letting this one sleep. |
Zoom out is not a device preview. We even moved it out of the dropdown because it was confusing, so they shouldn't behave the same and we don't want to change how device previews work right now, so they can stay the way they are. They are emulating a smaller screen just like browser inspectors do. Zoom out is something else and it should be closer to the site editing experience. Thanks for all your hard work here, this is a really complex problem. |
Right and my point does not conflate them nor say their scrolling has to work the same. It’s largely to make the point that if device previews don’t need to scroll like that then it is not an essential thing for zoom out either. More generally, it’s something to keep in mind for broader perspective and especially with regards to whether zoom out’s "assembly" mode functions are to be forever tied to the view scaling as that seems to be under consideration. I think the view scaling has a potential to be useful outside of that. Anyway, some of this is just going to take time to sort out. I don’t wish to debate any of this right now. |
Didn't read all comments here, but did you check 100vh elements? If I remember correctly the reason we switched to It worth nothing that zoom out is not purely a scaling of the viewport ( |
Yes, and the first thing in testing instructions is
Pardon me but I can’t seem to grasp what you mean. If it has to do with media queries then I think scaling the contents is certainly not simpler see #65595 and #66034. |
If you adjust the |
|
Whereas with scaling the content, the viewport ratio remains the exact same. |
Okay, I do see what you mean. The width is not narrowed but the height does increase and thus On the other hand, consider that |
Yes, I think that's a problem with this approach tbh. In trunk it would be exactly 1:1 |
In the part you quoted, I was saying in trunk the zoomed out view is not 1:1 with the frontend view. Still, your point that this can be seen as a problem with scaling the iframe stands. I think it’s relatively small and debatable yet we don’t need to contend with it now. Most of the issues this PR tackles are solved in trunk now. I may try and refresh this at some point in the interest of seeing how much simplification it allows. Additionally, for me, the zoomed out view could be applicable in more scenarios than an assembly mode and in such cases it should support sticky or fixed position elements and that’s something this does that trunk doesn’t (and probably can’t without some heavy-handed tactics). |
|
@stokesman Oh sorry, I didn't understand it then. What exactly is not 1:1 currently? Could you give an example? As far as I know vh units are accurate. |
Visually |
What?
A refactor with fixes and improvements to “Zoom Out”.
Why?
iframeinstead of itshtmlelement. Most notably Zoom Out breaks sticky positioned elements #64117 but this also fixes some things for which no issues exist:How?
iframeinstead of thehtmlTesting 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