Position scaled html within available container space #66034
Merged
+83
−30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
jeryj
added
[Type] Bug
jeryj
commented
Oct 10, 2024
Comment on lines
-15
to
-16
| // This is to offset the movement of the iframe when we open sidebars | ||
| margin-left: calc(-1 * (#{$prev-container-width} - #{$container-width}) / 2); |
There was a problem hiding this comment.
This was causing the scrollbar to be hidden off canvas.
Comment on lines
16
to
18
| $container-width: var(--wp-block-editor-iframe-zoom-out-container-width, 100vw); | ||
| // Apply an X translation to center the scaled content within the available space. | ||
| transform: scale(#{$scale}) translateX(calc(( #{$prev-container-width} - #{ $container-width }) / 2 / #{$scale})); |
There was a problem hiding this comment.
Move the scaled canvas to the right to occupy the space of the container.
There was a problem hiding this comment.
I think the translateX() could precede the scale() and the translation wouldn’t have to be manually counter-scaled but that’s a nit.
There was a problem hiding this comment.
I think the translateX() could precede the scale()
We tried that as well, but it seems like it's still necessary to apply the scaling to the translateX.
We could also use individual scale and translate properties
I'm going to try this, as I think we'll need to adjust the animating timings for this separately.
|
Size Change: +81 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in caba47c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11346746023
|
ajlende
reviewed
Oct 10, 2024
| @@ -9,9 +9,10 @@ | |||
| } | |||
|
|
|||
| .block-editor-iframe__scale-container.is-zoomed-out { | |||
| $container-width: var(--wp-block-editor-iframe-zoom-out-container-width, 100vw); | |||
There was a problem hiding this comment.
We should be able to remove this from the JS too since it's no longer used outside the iframe.
stokesman
reviewed
Oct 10, 2024
There was a problem hiding this comment.
This tests well for me. An added bonus is that it fixes the centering of the canvas on devices whose scrollbars are shown. I noticed there can be an odd transition when exiting zoom out:
66034-unzoom-width-sidebars.mp4
I don’t think it’s a big deal considering all the other odd things that can go on during the transition and this already seems to improve some of those.
Great idea on the approach. I had the same (rough) idea in the back of my mind and I’m glad to see it seemed simple enough.
Comment on lines
16
to
18
| $container-width: var(--wp-block-editor-iframe-zoom-out-container-width, 100vw); | ||
| // Apply an X translation to center the scaled content within the available space. | ||
| transform: scale(#{$scale}) translateX(calc(( #{$prev-container-width} - #{ $container-width }) / 2 / #{$scale})); |
There was a problem hiding this comment.
I think the translateX() could precede the scale() and the translation wouldn’t have to be manually counter-scaled but that’s a nit.
ciampo
reviewed
Oct 11, 2024
There was a problem hiding this comment.
Gave this a look, and it seems to work much better!
The timing of the animation can be improved, but I'm sure that it can be worked out.
I also found a bug (~36 seconds in the video below):
- open a teamplate
- enable zoomed out view
- click on the W logo to open side editor sidebar
- click on another template
- zoomed out view is already enabled, and the iframe is misplaced
Kapture.2024-10-11.at.09.56.24.mp4
I'll let other folks with a lot more familiarity on this part of Gutenberg review this PR in depth (@draganescu @MaggieCabrera @getdave )
|
There are still some glitches: satate-of-zoom-out-improvement.mp4
Update: I tested with a RTL lang setting and this PR works just fine. |
|
Based on @draganescu's review this feels close. @jeryj can you let me know if this PR becomes blocked or gets very much more complex? I just want to keep an eye on the scope of the fixes targeting 6.7 (which I assume this is?). |
|
This is very close, and it doesn't change the size of the canvas when there are more or less sidebars open. Great job here! |
Do you think this is similar to #65783? |
The "zoomed out mode is already enabled" part is the same as #65783, but the other part of the bug that I wanted to highlight is that the iframe/canvas is clipped and misplaced when it loads in zoomed-out mode |
ajlende
force-pushed
the
fix/iframe-zoom-container
branch
from
6be7c86
to
da3fc3e
Compare
jeryj
commented
Oct 11, 2024
| @@ -4,5 +4,4 @@ iframe[name="editor-canvas"] { | |||
| height: 100%; | |||
| display: block; | |||
| background-color: transparent; | |||
| @include editor-canvas-resize-animation; | |||
There was a problem hiding this comment.
This doesn't appear to have any effect.
|
EOD Notes: There's one more way to improve this that I couldn't dial in: We only want to animate the scaling when entering or exiting zoom out. We don't want to animate scale when it updates from sidebars opening. This is what causes it to fly out to random areas. To reproduce:
If we don't animate scale at that point, it's handled by the resizing of the iframe window and looks much smoother. @MaggieCabrera Do you have any ideas on how to do that efficiently? Also, I do think this is ready for review. I think overall this is an improvement over trunk. |
I need to test this more thoroughly but my first thought is that we are animating everything that happens to the div, maybe we want to target something specific like the scale and that would be enough? As a CSS only solution my only other suggestion is to have a class specific to the moments we want animated and then remove it, but I bet that's something that framer does on its own, so maybe that's another avenue to explore |
draganescu
force-pushed
the
fix/iframe-zoom-container
branch
from
d46d4e6
to
793f407
Compare
draganescu
approved these changes
Oct 14, 2024
| @@ -5,18 +5,20 @@ | |||
|
|
|||
| .block-editor-iframe__html { | |||
| transform-origin: top center; | |||
| @include editor-canvas-resize-animation; | |||
| transition: all 0.4s cubic-bezier(0.46, 0.03, 0.52, 0.96), transform 0s; | |||
There was a problem hiding this comment.
We could use a comment about the numbers even like, "this is what works" 😄
There was a problem hiding this comment.
It was just removing the mixin since it's only used once: https://github.com/WordPress/gutenberg/pull/66034/files#diff-3014a931b478b662a3c554c1306e3b548da2c2edd5b32114395b329404f8f50aL40
getdave
previously approved these changes
Oct 14, 2024
The scale container could be smaller than the available area if zoom out was initialized with a sidebar open, then closed. We were using prevContainerWidth in the CSS under the assumption it would always be the largest value, but that was not true. This renames prevContainerWidthRef to initialContainerWidth so it's clearer that this is the point when zoom out was initialized, then renames the CSS vars prev-container-width to outer-container-width, using the larger value of containerWidth or initialContainerWidth so it can be more consistently named.
draganescu
force-pushed
the
fix/iframe-zoom-container
branch
from
793f407
to
3cea814
Compare
getdave
reviewed
Oct 15, 2024
There was a problem hiding this comment.
The key aspects of this PR are working well and it resolves the original bug which was vertical toolbars being left "unpositioned" when toggling sidebars.
After updating to the latest changes since my last review, I still notice that we have animations happening on inserters and toolbars when toggling sidebars. These look a little funky but shouldn't block this PR merging.
Animations on toggle sidebars
Screen.Capture.on.2024-10-15.at.10-22-59.mp4
I recommend we merge this PR to improve the stability of the feature and then follow up to try and resolve the sidebar animations.
@kevin940726 @draganescu how do you feel about this and targetting 6.7?
This was referenced Oct 15, 2024
|
Hi @getdave, While testing the PR for the issue where the scrollbar gets hidden in zoom-out mode, I found that it’s working fine as shown in the attached video. However, I discovered another issue in this PR: when clicking on the Preview (Tablet, Mobile) modes, multiple scrollbars appear. Additionally, when switching to the pattern tab, it automatically goes into zoom-out mode. I’ve covered both the original issue and these new issues in the attached video. test-66034.mp4 |
ajlende
added
the
Backport to WP 6.7 Beta/RC
|
@dhruvang21 Thank you for testing! I went ahead and merged this as the issues you found are both present on
|
|
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk. |
|
@jeryj Could you take point on ensuring this PR cherry picks cleanly to the |
This was referenced Oct 15, 2024
jeryj
added a commit
that referenced
this pull request
Oct 15, 2024
* Positions the iframed HTML canvas via translateX rather than margin-left on the iframe in order to have the scrollbar for the iframe available instead of hidden off canvas. This also fixes issues with the vertical toolbar and inserters not rerendering to their new positions. * Renames prevContainerWidthRef to initialContainerWidth so it's clearer that this is the point when zoom out was initialized, then renames the CSS vars prev-container-width to outer-container-width, using the larger value of containerWidth or initialContainerWidth so it can be more consistently named. * Force largest available window size to scale from If you started with a sidebar open, then entered zoom out, then closed the sidebar, your scaled canvas would be too large. It should match the same size as if you start with no sidebars open, then enter zoom out. This also fixes an issue where scaling could be larger than 1. * Only animate scaling on entry and exit of zoom out mode to improve animations when opening and closing sidebars. * Known divergence: When starting from a smaller canvas (sidebars open), entering zoom out, then closing sidebars, there will be reflow on the canvas. --------- Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: dhruvang21 <dhruvang21@git.wordpress.org> Co-authored-by: AhmarZaidi <ahmarzaidi@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
ajlende
reviewed
Oct 16, 2024
Comment on lines
+381
to
+384
| // iframeDocument.documentElement.style.setProperty( | ||
| // '--wp-block-editor-iframe-zoom-out-outer-container-width', | ||
| // `${ Math.max( initialContainerWidth.current, containerWidth ) }px` | ||
| // ); |
There was a problem hiding this comment.
I don't think these comments were meant to be committed.
ajlende
reviewed
Oct 16, 2024
ajlende
reviewed
Oct 16, 2024
Comment on lines
+302
to
+312
| const handleZoomOutAnimationClassname = () => { | ||
| clearTimeout( zoomOutAnimationClassnameRef.current ); | ||
|
|
||
| iframeDocument.documentElement.classList.add( 'zoom-out-animation' ); | ||
|
|
||
| zoomOutAnimationClassnameRef.current = setTimeout( () => { | ||
| iframeDocument.documentElement.classList.remove( | ||
| 'zoom-out-animation' | ||
| ); | ||
| }, 400 ); // 400ms should match the animation speed used in components/iframe/content.scss | ||
| }; |
There was a problem hiding this comment.
This function should be defined inside the useEffect since that's the only place it's used.
ajlende
reviewed
Oct 16, 2024
| '--wp-block-editor-iframe-zoom-out-prev-container-width', | ||
| `${ prevContainerWidthRef.current }px` | ||
| '--wp-block-editor-iframe-zoom-out-outer-container-width', | ||
| `${ Math.max( initialContainerWidth.current, containerWidth ) }px` |
There was a problem hiding this comment.
I would make this a variable since it's used in three places where they are expected to always be the same.
ajlende
reviewed
Oct 16, 2024
| @@ -336,13 +374,16 @@ function Iframe( { | |||
| `${ containerWidth }px` | |||
| ); | |||
| iframeDocument.documentElement.style.setProperty( | |||
| '--wp-block-editor-iframe-zoom-out-prev-container-width', | |||
| `${ prevContainerWidthRef.current }px` | |||
| '--wp-block-editor-iframe-zoom-out-outer-container-width', | |||
There was a problem hiding this comment.
I would maybe rename this to --wp-block-editor-iframe-zoom-out-scale-container-width instead since this is the width of the block-editor-iframe__scale-container.
There was a problem hiding this comment.
We may not always call it scale container, so I'd be fine leaving this one named as is.
There was a problem hiding this comment.
Right, it may be changed, but for the current reader, the connection between the two would be more clear if the variable was named the same as the container that it's setting the width for.
There was a problem hiding this comment.
--wp-block-editor-iframe-scale-container-width would be acceptable too.
ajlende
pushed a commit
that referenced
this pull request
Oct 16, 2024
* Positions the iframed HTML canvas via translateX rather than margin-left on the iframe in order to have the scrollbar for the iframe available instead of hidden off canvas. This also fixes issues with the vertical toolbar and inserters not rerendering to their new positions. * Renames prevContainerWidthRef to initialContainerWidth so it's clearer that this is the point when zoom out was initialized, then renames the CSS vars prev-container-width to outer-container-width, using the larger value of containerWidth or initialContainerWidth so it can be more consistently named. * Force largest available window size to scale from If you started with a sidebar open, then entered zoom out, then closed the sidebar, your scaled canvas would be too large. It should match the same size as if you start with no sidebars open, then enter zoom out. This also fixes an issue where scaling could be larger than 1. * Only animate scaling on entry and exit of zoom out mode to improve animations when opening and closing sidebars. * Known divergence: When starting from a smaller canvas (sidebars open), entering zoom out, then closing sidebars, there will be reflow on the canvas. --------- Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: dhruvang21 <dhruvang21@git.wordpress.org> Co-authored-by: AhmarZaidi <ahmarzaidi@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
ajlende
pushed a commit
that referenced
this pull request
Oct 16, 2024
* Positions the iframed HTML canvas via translateX rather than margin-left on the iframe in order to have the scrollbar for the iframe available instead of hidden off canvas. This also fixes issues with the vertical toolbar and inserters not rerendering to their new positions. * Renames prevContainerWidthRef to initialContainerWidth so it's clearer that this is the point when zoom out was initialized, then renames the CSS vars prev-container-width to outer-container-width, using the larger value of containerWidth or initialContainerWidth so it can be more consistently named. * Force largest available window size to scale from If you started with a sidebar open, then entered zoom out, then closed the sidebar, your scaled canvas would be too large. It should match the same size as if you start with no sidebars open, then enter zoom out. This also fixes an issue where scaling could be larger than 1. * Only animate scaling on entry and exit of zoom out mode to improve animations when opening and closing sidebars. * Known divergence: When starting from a smaller canvas (sidebars open), entering zoom out, then closing sidebars, there will be reflow on the canvas. --------- Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: dhruvang21 <dhruvang21@git.wordpress.org> Co-authored-by: AhmarZaidi <ahmarzaidi@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
ajlende
pushed a commit
that referenced
this pull request
Oct 16, 2024
* Positions the iframed HTML canvas via translateX rather than margin-left on the iframe in order to have the scrollbar for the iframe available instead of hidden off canvas. This also fixes issues with the vertical toolbar and inserters not rerendering to their new positions. * Renames prevContainerWidthRef to initialContainerWidth so it's clearer that this is the point when zoom out was initialized, then renames the CSS vars prev-container-width to outer-container-width, using the larger value of containerWidth or initialContainerWidth so it can be more consistently named. * Force largest available window size to scale from If you started with a sidebar open, then entered zoom out, then closed the sidebar, your scaled canvas would be too large. It should match the same size as if you start with no sidebars open, then enter zoom out. This also fixes an issue where scaling could be larger than 1. * Only animate scaling on entry and exit of zoom out mode to improve animations when opening and closing sidebars. * Known divergence: When starting from a smaller canvas (sidebars open), entering zoom out, then closing sidebars, there will be reflow on the canvas. --------- Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: dhruvang21 <dhruvang21@git.wordpress.org> Co-authored-by: AhmarZaidi <ahmarzaidi@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #65188; fixes #65595
What?
Rework how the scaled canvas is centered to fix a few bugs.
Why?
How?
translateXto account for open sidebars.The gist is, rather than push the iframe around, push the scaled html element around.
Before: Margin-left to move the center of the iframe to line up with the container
After: Iframe takes up full available space, scale html, then push it over to be in the available area
Known Reflow Points
There will be some reflow when:
This is because our initial scale point started from a size smaller than our total available window space (all sidebars closed). In this one instance, I think reflow is acceptable. If we don't reflow, then it reflows when we exit zoom out, which I think is worse.
Most important points for reflow to NOT happen:
When it could be allowed:
One way we can prevent this is if beginning from a smaller state (sidebar open, enter zoom out) then add padding on the iframe to force the HTML to account for the extra space and stay scaled smaller. I'm not sure this complexity is worth it though.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Iframe before.
Note the left side is pushed over. This was to center the scaled canvas in the container. It worked to center the canvas, but the scrollbar is also pushed to the right.
iframe after
The iframe is the same size as the previous container. This allows the scrollbar to be visible. Then, the scaled content is pushed to the left to take up its available space.