Update Zoom Out for document width parity #65814
Conversation
|
Size Change: +88 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2b67bda. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11138792718
|
stokesman
force-pushed
the
update/zoom-out-document-width-parity
branch
from
2b67bda to
3cd5c19
Compare
|
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. |
stokesman
added
[Type] Bug
There was a problem hiding this comment.
This does fix the problem for me. Left two questions about the code. @stokesman is the fix is just in the CSS and the JS updates are just for the px rounding?
| // Core usage is pixels only and supporting any unit would be complicated. | ||
| let frameWidth = 0; | ||
| if ( frameSizeIsNumber || frameSize.endsWith( 'px' ) ) { | ||
| frameWidth = |
There was a problem hiding this comment.
when are we not in any of these conditions?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The
IFramecomponent 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
frameSizenever 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 |  |
 |
| 8em |  |
 |
I did, thanks to your comment, reexamine everything and realized the second commit here was preventing a non-pixel
frameSizefrom 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.
No the fix does require some JS and that’s solely in the first commit 3cd5c19. I debated including the second commit with the px rounding as it’s possibly too minute. Without it though the zoomed out width is often nearly a whole pixel different from unzoomed. I suppose there’s some chance the px rounding improves the horizontal aspect of #65744 but I'm just guessing. |
|
Since this is related to document width changes due to added borders for Firefox (correct me if I'm wrong), would changing the |
I believe I was trying such along the way here. I’m up for trying again though. |
|
|
Since EDIT: I feel stupid. I'm mixing up the |
|
@stokesman it seems @richtabor landed #66012 which apparently fixed the issue via a one line udpate in css. |
|
Later edit - let's wait out on this one as removing the padding causes unexpected effects it seems |
Just saw this. I was going to say that I revisited the |
|
#65998 did this in an overall nicer way since it removed the border (instead of negating it with |
What?
Updating zoom out so its scaling/styling preserves document width.
Why?
Document width is off due to lack of accounting for the added space around canvas.
Resolves #65757 #63519
How?
borderandmargin-inlineto avoid sub-pixel document widthsTesting Instructions
htmlelementUsing Chrome’s dev tools the
Computedtab will show the width as if it were not scaled so it’s handy for this. Though if you have thehtmlelement selected during the transition to zoom out, it may fail to update completely so deselecting and reselectinghtmlelement is recommended.Testing Instructions for Keyboard
Screenshots or screencast
zoom-out-width-parity.mp4