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.

Sign up for GitHub

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

Jump to bottom

Zoom out: fix scaling issues #65998

Merged
merged 12 commits into from
Oct 10, 2024
Merged

Zoom out: fix scaling issues #65998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6dcea75
changed from using borders to use pseudo elements
MaggieCabrera Oct 9, 2024
53fe5ab
added comments
MaggieCabrera Oct 9, 2024
22587a1
Include frame size in the scaling calculation
jeryj Oct 9, 2024
7dfed11
Account for scaling in the frame height
jeryj Oct 9, 2024
abe17a2
Calculation in CSS
ajlende Oct 9, 2024
c70f39a
Revert "Calculation in CSS"
jeryj Oct 10, 2024
a444cdb
Add comment about why the calculation for scaling needs to happen in …
jeryj Oct 10, 2024
7af85bd
Force px value for frameSize
jeryj Oct 10, 2024
4146bd3
Restore previous frameSize var setting
jeryj Oct 10, 2024
28e4dd0
Add back line break
jeryj Oct 10, 2024
03ed997
Use padding top/bottom instead of before/after for top/bottom frame
jeryj Oct 10, 2024
47b31ed
Remove more before/after css since we are using padding
jeryj Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/block-editor/src/components/iframe/content.scss
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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
View file
Open in desktop
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