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
amp-img with layout intrinsic stretches to fill parent if display: block #21371
Comments
Another issue happens when an |
Thank you. Reduced that to an isolated repro here: https://codepen.io/cathyxz/pen/JzmLYE. |
After some research here, I think |
I think I have a fix for the original issue to deal with |
Actually, I thought about this some more, particularly in context of the
@westonruter would it be reasonable for your WP converter to add a |
@westonruter another option for that specific use case you provided is to add |
@cathyxz thank you. Those are helpful suggestions. I'll have to try them. |
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. |
…inc also get object-fit:contain This guards against image stretching. See <ampproject/amphtml#21371 (comment)>. Props @cathyxz
@cathyxz I believe I've worked out how to successfully incorporate your fix in the AMP plugin. See ampproject/amp-wp#2036 Is the change safe to include in a release of the plugin in the next 1-2 weeks? |
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 |
@cathyxz
Even when I've explicitly toggled |
I see. I think even though it visually looks right, if the underlying Some context on srcset and sizes behavior: go/srcset-and-sizes. |
@cathyxz oh, how can the experiment be forced on? |
Thanks for pointing out the experiment info for me over chat. The opt-in has been added! ampproject/amp-wp@034ac82 See in action here: https://2019-theme.amp-wp.org/bison-image-test/?amp |
…inc also get object-fit:contain This guards against image stretching. See <ampproject/amphtml#21371 (comment)>. Props @cathyxz
the |
@DrLightman Have you tried |
@westonruter thank you man, it works perfectly. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
This is obsolete since |
It's possible and sometimes common to have CSS overrides to set
amp-img
todisplay: block
. When this happens,<amp-img>
with layoutintrinsic
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. Whenbehaving as intended.display: inline
is applied to<amp-img>
with layout=intrinsic, the image fails to render.The text was updated successfully, but these errors were encountered: