-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Image Aspect Ratio: Rely only on width to define image sizes when aspect ratio is set #53225
Conversation
height: elt.offsetHeight, | ||
aspectRatio: undefined, | ||
height: 'auto', | ||
aspectRatio: aspectRatio ? aspectRatio : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this wasn't set before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drag handles set both the width
and the height
so the aspectRatio
needed to be cleared since it isn't valid with both width
and height
.
@@ -689,8 +697,8 @@ export default function Image( { | |||
onResizeStop(); | |||
setAttributes( { | |||
width: elt.offsetWidth, | |||
height: elt.offsetHeight, | |||
aspectRatio: undefined, | |||
height: 'auto', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change. We are now relying on the width and aspect ratio to infer the height.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems strange to me that dragging the bottom handle would set the width of the image. That interaction feels to me like it should be setting the height, but maybe that's just me.
There was someone who requested modifier keys for proportional resizing in #51843, that sort of interaction is what makes the most sense to me.
imgStyle.height = 'auto'; | ||
imgStyle.width = '100%'; | ||
} | ||
if ( width ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will override the setting above.
objectFit: scale, | ||
...borderProps.style, | ||
}; | ||
if ( aspectRatio ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for the case where there is no width/height defined.
Size Change: +81 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
imgStyle.width = width; | ||
} | ||
|
||
if ( height && ! isNaN( height ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why might height be NaN
? We should try to prevent that from happening rather than checking isNaN()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Height can be 'auto', so maybe we should just check for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could avoid editing save.js so we don't have to add a block deprecation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this will require a deprecation anyway...
@@ -547,6 +547,21 @@ export default function Image( { | |||
const borderProps = useBorderProps( attributes ); | |||
const isRounded = attributes.className?.includes( 'is-style-rounded' ); | |||
|
|||
const imgStyle = { | |||
aspectRatio: aspectRatio ? aspectRatio : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the ternary needed here? What falsy values do we need to convert to undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably not needed it was just for my own ease of reading.
width={ width } | ||
height={ height } | ||
height={ isNaN( height ) ? undefined : height } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to prevent layout shifts when height
is 'auto'
, we need to ensure that this property contains a real value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could keep the height for the image attributes and just use auto in the CSS
Closing in favour of #53274 |
What?
When setting aspect ratio for an image, we define both a width and a height. This is unnecessary, as the browser can infer the height based on a width and an aspect ratio. This PR removes the reliance on the height attribute for images that have aspect ratio defined, to allow the browser to calculate the height. Fixes #52739
Why?
When the browser is smaller than the width/height defined in the CSS, the image is stretched. See #52739
How?
When the user resizes the image and aspect ratio is defined, then don't update the height attribute, only the width.
Testing Instructions
Screenshots or screencast
Co-authored-by: Maggie 3593343+MaggieCabrera@users.noreply.github.com