Amphtml: Expose `object-fit:cover|contain|etc..` as an attribute on `<amp-img>`, `<amp-anim>` and `<amp-video>`

[aghassemi]

Nobody likes images with messed up aspect ratios but that can easily happen with layout fill and fixed on amp-img (and other situations where a fill layout is enforced, like items in a carousel).

I believe layout and object-fit are dependent enough for images that I believe it justifies exposing object-fit as an attribute.

The following part about the default value of this attribute is under discussion and should _not be implemented without consensus_

The idea here is to have a default that's reasonable (probably contain) and an explicit attribute to make it easily configurable and obvious.

This will be a breaking change, but I believe it will break things with a better and more positive side-effect.

Ideally we apply this attribute in CSS with low-specificity so if there is an existing user-defined amp-img > img { object-fit: somethingOtherThanDefault } theirs won't be overridden.

We can put the change behind an experiment and test for a while.

Alternatively we can just do this in our CSS without exposing an attribute and expect authors to override it in CSS instead of using an attribute (although I like the explicitness of the attribute better).

@cramforce @jridgewell @dvoytenko thoughts?

[cramforce]

I don't think we can change the default here, independent of whether that would be better or not. I'd also like to go back and make layout=responsive the default for images.

We really need to do a better job documenting this, but I really think that people understand what they get with fixed and that fill is basically "I know what I'm doing" and we can expect people to set object-fit.

[aghassemi]

ok, if I read this correctly, you are not opposed to exposing it as an attribute as long as we don't change the default behaviour?

[jridgewell]

Nobody likes images with messed up aspect ratios but that can easily happen with layout fill and fixed on amp-img

Isn't that layout=responsive?

[aghassemi]

There are cases where layout=responsive isn't enough though and it makes senses to want to do fill without stretching the image.

For example assume you wanna build a thumbnail gallery:

• Parent of the image is a square 200px * 200px box
• Image itself has a 2w * 1h ratio

Doing just layout=responsive will leave the bottom half of the thumbnail box empty. Doing layout=fill will mess up the aspect ratio. What you would want is layout=fill + object-fit=cover.

Of course this is possible right now, but I believe we have a small usability issue here where you are doing part of the work in an attribute and part of it in a custom CSS rule. layout and object-fit are dependent enough for images that I believe it justifies exposing object-fit as an attribute.

[cramforce]

I'm definitely a fan of adding an attribute.

On Wed, Sep 14, 2016 at 11:20 AM, Ali Ghassemi [email protected]
wrote:

There are cases where layout=responsive isn't enough though and it make
senses to want to do fill without stretching the image.

For example assume you wanna build a thumbnail gallery:

• Parent of the image is a square 200px * 200px box
• Image itself has a 2w * 1h ratio

Doing just layout=responsive will leave the bottom half of the thumbnail
box empty. Doing layout=fill will mess up the aspect ratio. What you
would want is layout=fill + object-fit=cover.

Of course this is possible right now, but I believe we have a small
usability issue here where you are doing part of the work in an attribute
and part of it in a custom CSS rule. layout and object-fit are dependent
enough for images that I believe it justifies exposing object-fit as an
attribute.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/issues/4988#issuecomment-247107709,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT8Pnh_8_J9xVoKyRJcRTVbuwp-RIks5qqDrUgaJpZM4J8bEN
.

[dvoytenko]

One approach we could take is to default object-fit:contain only for fill and flex-item layouts.

[aghassemi]

http://www.washingtonpost.com/amphtml/world/as-the-west-pays-tribute-to-peres-many-arabs-recall-a-legacy-of-destruction/2016/09/28/c7525d3a-8581-11e6-b57d-dd49277af02f_story.html has been in top carousel for a day now with a badly stretched image. The issue is user defined width/height ratio on amp-img is wrong but if we had defaulted to object-fit: contain at least it would have looked okay (we would have ended up with top/bottom whitespace rather than an stretched image)

[dev7ch]

+ 1 for: Expose object-fit:cover|contain|etc..

[conwaydev]

Also would love this. Currently in the docs (https://ampbyexample.com/advanced/how_to_support_images_with_unknown_dimensions/#object-fit-to-the-rescue) it states to use css such as:

 amp-img.contain img {
   object-fit: contain;
 }

However, any sort of UnCSS tool (including the PHP based one the WordPress plugin uses), will strip out that style because the img doesn't actually exist in the DOM. Happy to hear about other solutions people have, but it's frustrating for us to put an invisible <div class="dn cover"><img src="data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="></div> just so it keeps that CSS rule.

[cramforce]

We should probably track fixing the UnCSS tools separately.
CC @amedina