Prevent images from appearing squished when only one dimension is set#70575
Conversation
|
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. |
3142a24 to
e81e430
Compare
|
Update after rebasing on trunk. This has been rebased and reworked since the original submission. Main changes:
|
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for the work on this @lschuyler 👍
This is testing pretty nicely for me.
For others looking at the deprecation, it's worth noting that the structure in this PR is correct. As older deprecations would match on their outdated markup, they'd get resaved to acquire the height: auto inline style as needed. Given there is no migration of attributes required, only the new v9 deprecation is needed.
While reviewing the deprecations I think I spotted one issue with v7.migrate(). With the unconditional template literal, a block with width: 300, height: undefined migrates to height: "undefinedpx". The new current save() then serializes that literally as style="height: undefinedpx".
The v6.migrate() function handles this better.
The bug there predates this PR but this one does make it worse. So I think we should fix it up now. Additionally, it's just nice whenever adding a new version for a block to ensure every past version is migrated correctly to the latest and greatest.
One other thing, is the isEligible check actually needed? It's conventionally for attribute schema only migrations where the block markup is technically still valid but the attributes still need migrating. I feel isEligible here might be redundant and slightly misleading.
More practically: after a block is migrated and re-saved with the new format, the height attribute is still undefined (it's never stored), so isEligible would keep returning true on every subsequent load. With no migrate() this is a no-op, but it means the deprecation system needlessly processes the block on every parse. Worth dropping isEligible from v9 entirely since the save() match handles it.
|
Thanks for the thorough review @aaronrobertshaw! I pushed another commit to address your feedback:
|
aaronrobertshaw
left a comment
There was a problem hiding this comment.
The tweaks to the deprecations look good, thanks @lschuyler 🚀
This PR is testing as advertised for me.
| Before | After |
|---|---|
![]() |
![]() |
LGTM 🚢
When a user sets only a width (or only a height) on an image block, theme CSS can override the other dimension and squish the image. This change sets the unspecified dimension to `auto` via inline style so it takes precedence over any theme stylesheet. A v9 deprecation is added so that images saved without the explicit `height: auto` are still recognized as valid and migrated on the next save.
…gible v7.migrate() was unconditionally wrapping width and height in template literals, producing "undefinedpx" when either dimension was not set. Match the safer pattern used in v6.migrate(), which guards with typeof ... === 'number' before appending 'px'. Remove isEligible from v9. Since height is never stored in block attributes when left unset, isEligible would return true on every parse of a block with only width set, causing the deprecation system to needlessly reprocess it. The save() match handles detection correctly without isEligible.
ff94bca to
538fea2
Compare
|
@aaronrobertshaw any reason this was not merged when the PR was approved? |
|
I just re-tested this. Thanks for rebasing @lschuyler It's working well for me. I can merge it. |
Apologies for the slow reply while away. I have a vague recollection of re-running a failed test. Looks like I missed hitting auto merge 😬 |



This PR addresses the issue of images in the editor appearing squashed when only one dimension is set.
Problem
When you set only one of the image block's dimensions (width or height) in the editor and leave the other blank (auto), Gutenberg serializes just that single value as an inline style, like this:
In this example, the blue-shape.png image was uploaded at 228 x 228. In the block editor UI, the width was set to 64px, and the height was left as auto.
Because many themes (like the one we were using when we discovered this) still define a fixed CSS height (in our case, 76px), the browser will render the image at 64 x 76, making it appear distorted.
In this example, this was the existing CSS:
This Fix
This PR adds similar logic in two places to ensure that whenever only one dimension is set by the user, the other dimension is explicitly set to auto, to preserve the aspect ratio.
This logic is added in two places:
In each file, a small style object is built that:
By guaranteeing
height: autowhenever height isn't defined, this prevents themes' CSS heights from squashing the image. A theme's default width remains in effect when the user hasn't adjusted the width in the editor.Handling Existing Content
A new deprecation (v9) is added to handle image blocks that were saved before this fix with only
widthset and noheightin the inline style. When one of those blocks is next saved, it will be migrated so the inline style includesheight: auto. Integration test fixtures are included covering parse and serialisation of this migration path.Testing Instructions
64) and leave the height blank.image-testclass.Screenshots (inside the Editor)
Before the fix (64 x 76):

After the fix (64 x 64):
