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

Update Zoom Out for document width parity #65814

Closed
wants to merge 7 commits into from
Closed

Update Zoom Out for document width parity #65814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3cd5c19
Factor `frameSize` into scaling and document width styling
stokesman Oct 2, 2024
167d5f2
Avoid decimals for pixel precision
stokesman Oct 2, 2024
f86a7cc
Fix testing for number type of `frameSize`
stokesman Oct 7, 2024
7a24f82
Revise comment about `frameSize` in scale calculation
stokesman Oct 7, 2024
1374d0b
Restore `frameSize` counter-scaling for non-pixel values
stokesman Oct 8, 2024
b542ae4
Remove unused CSS variable
stokesman Oct 10, 2024
696c0d5
Revise comment again
stokesman 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
6 changes: 3 additions & 3 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 @@ -32,22 +32,22 @@
$frame-size: var(--wp-block-editor-iframe-zoom-out-frame-size);
$inner-height: var(--wp-block-editor-iframe-zoom-out-inner-height);
$content-height: var(--wp-block-editor-iframe-zoom-out-content-height);
$prev-container-width: var(--wp-block-editor-iframe-zoom-out-prev-container-width);

transform: scale(#{$scale});

background-color: $gray-300;

// Firefox and Safari don't render margin-bottom here and margin-bottom is needed for Chrome
// layout, so we use border matching the background instead of margins.
border: calc(#{$frame-size} / #{$scale}) solid $gray-300;
border: $frame-size solid $gray-300;

// 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 * (#{$scale} * #{$frame-size}));
$total-height: calc(#{$extra-content-height} + #{$total-frame-height} + 2px);
margin-bottom: calc(-1 * #{$total-height});
margin-inline: calc(-1 * #{$frame-size});

body {
min-height: calc((#{$inner-height} - #{$total-frame-height}) / #{$scale});
Expand Down
32 changes: 20 additions & 12 deletions 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,16 +306,31 @@ function Iframe( {
iframeDocument.documentElement.classList.add( 'is-zoomed-out' );

const maxWidth = 750;
const frameSizeIsNumber = typeof frameSize === 'number';
// `frameSize` has to be factored into default scale calculation because it narrows the
// content width. If it’s a non-pixel value it’s still applied in the style but can’t be
// directly factored into the scale. A consumer could pass a `scale` that has factored in
// their non-pixel `frameSize` if they need to. Core relies only on pixel values.
let frameWidth = 0;
if ( frameSizeIsNumber || frameSize.endsWith( 'px' ) ) {
frameWidth =
Copy link
Contributor

Choose a reason for hiding this comment

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

when are we not in any of these conditions?

Copy link
Contributor Author

@stokesman stokesman Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We, as in core, never. frameSize is a public prop and given that VisualEditor specifies it as as'48px' some third-party could conceivably have the notion that any CSS unit could be used and be passing '2em'. I revised the comment preceding this to hopefully clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

The IFrame component is a public API, so it could have, in theory, already been used by a third-party with a non-pixel value. Making this a breaking change.

Copy link
Contributor Author

@stokesman stokesman Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for commenting. I think this is not a breaking change. The frameSize never gets left out of the applied styles, it just doesn’t get factored into the "default" scale calculation when it can’t be treated as pixels. That’s how it was for any value before this.

I did, thanks to your comment, reexamine everything and realized the second commit here was preventing a non-pixel frameSize from being counter-scaled in the styles. So I pushed a 1374d0b to restore that. I’d be pleased to find a way to improve the poetry here and will look for a better refactor.

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.

The IFrame component is a public API, so it could have, in theory, already been used by a third-party with a non-pixel value. Making this a breaking change.

I think this is not a breaking change. The frameSize never gets left out of the applied styles, it just doesn’t get factored into the "default" scale calculation when it can’t be treated as pixels. That’s how it was for any value before this.

I think I see what you're saying, but the effect for consumers that were using non-pixel units is still broken with this PR as it no longer adds horizontal padding for non-pixel units like trunk, thus the breaking change.

Trunk #65814
48px Trunk 48px PR 48px
8em Trunk 8em PR 8em

I did, thanks to your comment, reexamine everything and realized the second commit here was preventing a non-pixel frameSize from being counter-scaled in the styles. So I pushed a 1374d0b to restore that. I’d be pleased to find a way to improve the poetry here and will look for a better refactor.

My comment was mostly just a gut feeling from having read the code which is why it didn't have that much context. You had me reexamine my assumptions too, so I do appreciate that.

I figured we could move your frameWidth calculation to CSS to get around the issue, so I dug into that and found out that CSS could not actually handle the math for units; my assumption was wrong.

The division operator in CSS calc() doesn't do the dimensional analysis of first converting units to pixels and then getting the ratio of them.

I've been wracking my brain to think of ways to support units so that it isn't a breaking change because including the frameSize is important for calculating the scale.

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.

I think I'm okay with that as long as we add documentation for the px requirement now even if it will break non-px usage that may exist.

2 * ( frameSizeIsNumber ? frameSize : parseInt( frameSize ) );
}
const usedWidth = Math.min( containerWidth, maxWidth ) - frameWidth;
const usedScale =
scale === 'default'
? usedWidth / prevContainerWidthRef.current
: scale;
const integerFrameSize = Math.round( frameWidth / 2 / usedScale );
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-scale',
scale === 'default'
? Math.min( containerWidth, maxWidth ) /
prevContainerWidthRef.current
: scale
usedScale
);
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-frame-size',
typeof frameSize === 'number' ? `${ frameSize }px` : frameSize
frameWidth !== 0
? `${ integerFrameSize }px`
: `calc( ${ frameSize } / ${ usedScale } )`
);
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-content-height',
Expand All @@ -329,10 +344,6 @@ function Iframe( {
'--wp-block-editor-iframe-zoom-out-container-width',
`${ containerWidth }px`
);
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-prev-container-width',
`${ prevContainerWidthRef.current }px`
);

return () => {
iframeDocument.documentElement.classList.remove( 'is-zoomed-out' );
Expand All @@ -352,9 +363,6 @@ function Iframe( {
iframeDocument.documentElement.style.removeProperty(
'--wp-block-editor-iframe-zoom-out-container-width'
);
iframeDocument.documentElement.style.removeProperty(
'--wp-block-editor-iframe-zoom-out-prev-container-width'
);
};
}, [
scale,
Expand Down
Loading