Amphtml: amp-img with layout intrinsic stretches to fill parent if display: block

[cathyxz]

It's possible and sometimes common to have CSS overrides to set amp-img to display: block. When this happens, <amp-img> with layout intrinsic ends up stretching to fill the entire parent container instead of maintaining its correct aspect ratio.

Here's a minimal repro: https://codepen.io/cathyxz/pen/PLJmyQ. When display: block !important is applied to <amp-img>, the image stretches to fill the container width.

~Related: https://codepen.io/cathyxz/pen/JzmLYE. When display: inline is applied to <amp-img> with layout=intrinsic, the image fails to render.~ behaving as intended.

[westonruter]

Another issue happens when an amp-img gets display: inline: the image fails to render entirely. See https://github.com/ampproject/amp-wp/issues/1803

[cathyxz]

Thank you. Reduced that to an isolated repro here: https://codepen.io/cathyxz/pen/JzmLYE.
Layout intrinsic with display: inline doesn't display the image (<img> tag sized to 0x0).

[cathyxz]

After some research here, I think display: inline won't work with any of the amp layouts other than CONTAINER, since <amp-img> is basically a <div> that contains some sizing elements and then an <img> that is sized to fit its parent subject to margin constraints created from sizer elements. display: inline basically ignores height and width CSS properties, which means none of our sizing strategies for layouts will work (other than container and none).

[cathyxz]

I think I have a fix for the original issue to deal with display: block overstretching the original image. I just need to do a bit of comprehensive testing and cross check with our spec.

[cathyxz]

Actually, I thought about this some more, particularly in context of the layout=INTRINSIC spec:

The element takes the space available to it and resizes its height automatically to the aspect ratio given by the width and height attributes until it reaches the element's natural size or reaches a CSS constraint (e.g., max-width). The width and height attributes must be present. This layout works very well for most AMP elements, including amp-img, amp-carousel, etc. The available space depends on the parent element and can also be customized using max-width CSS. This layout differs from responsive by having an intrinsic height and width. This is most apparent inside a floated element where a responsive layout will render 0x0 and an intrinsic layout will inflate to the smaller of its natural size or any CSS constraint.

@westonruter would it be reasonable for your WP converter to add a max-width constraint based on the width attribute of the <amp-img> if display: block? This is basically what my proposed fix does under the hood, and I'm wondering if it makes more sense to do it at the developer level instead of the AMP layout level.

[cathyxz]

@westonruter another option for that specific use case you provided is to add object-fit: contain to amp-img[layout=intrinsic] > img. This is not a solution we can apply at the amp layout level, but could be viable for developers to enforce layout=INTRINSIC behavior with display: block.

[westonruter]

@cathyxz thank you. Those are helpful suggestions. I'll have to try them.

[cathyxz]

Sounds great. Basically, there isn't an easy fix for this issue that does not result in some other kind of tradeoff that changes the current behaviour and potentially breaks somebody else. At the present, I think this is more easily solved with the above-mentioned developer workarounds than a code change in AMP, but we can revisit if this becomes blocking for more folks.

[westonruter]

@cathyxz I believe I've worked out how to successfully incorporate your fix in the AMP plugin. See https://github.com/ampproject/amp-wp/pull/2036

Is the change safe to include in a release of the plugin in the next 1-2 weeks?

[cathyxz]

The object-fit contains change is fine since that should not depend on any changes in AMP and should also not have any side effects (unless the object-fit property for that particular selector is being overridden by theme developers). I believe you are also using doc-level opt-in for amp-img-auto-sizes, so I don't see any issues with removing sizes either (I haven't looked super closely at the other parts of your PR so I can't speak to that). If you want to err on the side of caution and wait until amp-img-auto-sizes goes to production, that experiment is currently at 10% in production, and should go to 100% of production in 3-4 weeks, depending on whether any issues are discovered as we ramp this up.

[westonruter]

@cathyxz ~actually, I didn't include doc-level opt-in for amp-img-auto-sizes in this PR. But I just added an opt-in via URL parameter to test:~

• ~✅ Non-AMP: https://2019-theme.amp-wp.org/bison-image-test/~
• ~✅ AMP _with_ sizes removed: https://2019-theme.amp-wp.org/bison-image-test/?amp&temp_amp_remove_img_sizes=false~
• ~⛔️ AMP _without_ sizes removed: https://2019-theme.amp-wp.org/bison-image-test/?amp&temp_amp_remove_img_sizes=true (the images have the incorrect dimensions here)~

Even when I've explicitly toggled amp-img-auto-sizes off, the removal of sizes seems to have the desired effect. I fully admit that I'm not an expert in responsive images, so I'm not entirely comfortable sizes even knowing what to look for. But visually it looks better to remove the attribute regardless of the experiment. 🤔 Thoughts?

[cathyxz]

I see. I think even though it visually looks right, if the underlying img tag does not have a sizes attribute, there will be a perf hit because you unnecessarily downloading images that are too large (esp for Firefox and Safari, both of which will always pick the largest src in the srcset if no sizes attribute is given, Chrome does semi reasonable things). So I think it's a good idea to force the experiment on for performance reasons if you are going to remove the sizes attribute (or you can just wait for it to go to 100% production).

Some context on srcset and sizes behavior: go/srcset-and-sizes.

[westonruter]

@cathyxz oh, how can the experiment be forced on?

[westonruter]

Thanks for pointing out the experiment info for me over chat.

The opt-in has been added! https://github.com/ampproject/amp-wp/pull/2036/commits/034ac82d3307413a422f9d53d14a274b6f785713

See in action here: https://2019-theme.amp-wp.org/bison-image-test/?amp

[DrLightman]

the object-fit: contain tweak seems to work in regard to aspect ratio but the image stays centered whereas I need it aligned on the left.

[westonruter]

@DrLightman Have you tried object-position: left center?

[DrLightman]

@westonruter thank you man, it works perfectly.