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.

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 out: fix scaling issues #65998

Merged
merged 12 commits into from
Oct 10, 2024
10 changes: 6 additions & 4 deletions packages/block-editor/src/components/iframe/content.scss
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
}

.block-editor-iframe__html {
border: 0 solid $gray-300;
transform-origin: top center;
@include editor-canvas-resize-animation;
}
Expand All @@ -19,14 +18,17 @@

background-color: $gray-300;

padding: calc(#{$frame-size} / #{$scale}) 0;

// Chrome seems to respect that transform scale shouldn't affect the layout size of the element,
// so we need to adjust the height of the content to match the scale by using negative margins.
$extra-content-height: calc(#{$content-height} * (1 - #{$scale}));
$total-frame-height: calc(2 * #{$frame-size});
$total-frame-height: calc(2 * #{$frame-size} / #{$scale});
$total-height: calc(#{$extra-content-height} + #{$total-frame-height} + 2px);
margin-bottom: calc(-1 * #{$total-height});
// Add the top/bottom frame size. We use scaling to account for the left/right, as
// the padding left/right causes the contents to reflow, which breaks the 1:1 scaling
// of the content.
padding-top: calc(#{$frame-size} / #{$scale});
padding-bottom: calc(#{$frame-size} / #{$scale});

body {
min-height: calc((#{$inner-height} - #{$total-frame-height}) / #{$scale});
Expand Down
8 changes: 7 additions & 1 deletion packages/block-editor/src/components/iframe/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,13 +306,19 @@ function Iframe( {
iframeDocument.documentElement.classList.add( 'is-zoomed-out' );

const maxWidth = 750;
// This scaling calculation has to happen within the JS because CSS calc() can
// only divide and multiply by a unitless value. I.e. calc( 100px / 2 ) is valid
// but calc( 100px / 2px ) is not.
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-scale',
scale === 'default'
? Math.min( containerWidth, maxWidth ) /
? ( Math.min( containerWidth, maxWidth ) -
parseInt( frameSize ) * 2 ) /
Copy link
Contributor

@stokesman stokesman Oct 10, 2024

Choose a reason for hiding this comment

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

This treats any frameSize value as a pixel. That’s probably not a big problem or could even be argued that it creates some compensation for a non-pixel value even if it won’t be accurate. The approach in #65814 was different by avoiding factoring in a non-pixel frameSize but I think @ajlende mistook it for leaving out the frameSize entirely and called it breaking change #65814 (comment). Then seemingly didn’t see my reply trying to clarify.

Anyhow, my point is this is practically very similar yet at least in my PR the code acknowledges what’s inaccurate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyhow, my point is the is practically very similar yet at least in my PR the code acknowledges what’s inaccurate.

Yes - you figured out the main bug. That was a huge help. 🙏🏻 You were supposed to be credited in this PR because of that. This PR takes a slightly different direction by removing the border entirely and just account for the frame size scaling that you figured out.

This treats any frameSize value as a pixel. hat’s probably not a big problem or could even be argued that it creates some compensation for a non-pixel value even if it won’t be accurate.

I'm fine forcing pixels for frameSize, as I don't think we can accurately account for non-pixel sizes in everything we need due to css calc limitations. I thought it was going to be possible to accurately account for any value, but now I'm convinced it isn't possible 😅

at least in my PR the code acknowledges what’s inaccurate.
Do you mean this comment?. I tried to explain it via this comment, but maybe I didn't explain it in-depth enough to be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries about the credit. I do love that this ultimately does make the styles simpler. I thought I’d tried removing the border with my PR but seems like it was making it so the inline frame spacing wasn’t maintained through resizes.

I thought it was going to be possible to accurately account for any value, but now I'm convinced it isn't possible 😅

Yeah, not without rendering another element to measure with getComputedStyle and some compromises. Trying to put the calculation in CSS was a great thought.

but maybe I didn't explain it in-depth enough to be useful.

Oh, if that comment is meant to do that it seems misplaced. It’s detached from the JS calculation.

Copy link
Contributor

@ajlende ajlende Oct 10, 2024

Choose a reason for hiding this comment

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

This treats any frameSize value as a pixel. That’s probably not a big problem or could even be argued that it creates some compensation for a non-pixel value even if it won’t be accurate. The approach in #65814 was different by avoiding factoring in a non-pixel frameSize but I think @ajlende mistook it for leaving out the frameSize entirely and called it breaking change #65814 (comment). Then seemingly didn’t see my reply trying to clarify.

FWIW this approach also causes the same breaking change that I commented on in your PR. I wasn't ready for this one to be merged so soon. I think still we need to document the requirement of a number representing pixels or a pixel string for the API of this component. Based on this argument that I shared on https://github.com/WordPress/gutenberg/pull/65814/files#r1795825607:

I suppose a loophole for the backwards compatibility argument could be that nothing about the prop was documented, so you could argue that the previous API accepted only numbers or pixel strings rather than any CSS unit.

And sorry for the slow response btw, I wanted to make sure that I understood your perspective and make sure the suggestions that I was about to share were valid and made sense which ended up taking all day for me yesterday. One thing that can annoy me is half-baked responses that seem dismissive or suggestions that won't work because they haven't fully been thought out, and I didn't want to do that to you. I have finally posted it https://github.com/WordPress/gutenberg/pull/65814/files#r1795825607, but it seems that ended up being too late.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry I rushed merging this one! and also about not adding proper credit, I take full responsibility for that. I shouldn't have jumped on the merge button like that

prevContainerWidthRef.current
: scale
);

// frameSize has to be a px value for the scaling and frame size to be computed correctly.
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-frame-size',
typeof frameSize === 'number' ? `${ frameSize }px` : frameSize
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/iframe/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@
$container-width: var(--wp-block-editor-iframe-zoom-out-container-width, 100vw);
$prev-container-width: var(--wp-block-editor-iframe-zoom-out-prev-container-width, 100vw);
width: $prev-container-width;
// This is to offset the movement of the iframe when we open sidebars
margin-left: calc(-1 * (#{$prev-container-width} - #{$container-width}) / 2);
}
Loading