GoogleChrome / samples

A repo containing samples tied to new functionality in each release of Google Chrome.
https://www.chromestatus.com/samples
Apache License 2.0
5.75k stars 2.38k forks source link

intrinsicsize conflicting with CSS object-fit #627

Open Schepp opened 5 years ago

Schepp commented 5 years ago

I recently added the intrinsicsize attribute to our news site when I was anyway working on our image component, as I liked the idea behind it and the provided layout stability helps our readers massively. Of course it was not yet active in browsers, but I wanted to be prepared for that day.

After a few weeks I switched the "Experimental Webplatform Features" flag on, as I wanted to demo new stuff in a meetup talk. Of course, I didn't turn the flag back off afterwards, so that intrinsicsize stayed activated.

Now, when I returned to our site, I discovered that suddenly certain images (but not all) where distorted. E.g.:

image

link

image

link

What the distorted images had in common was that they had the CSS property object-fit set - either to contain (1. example) or to cover (2. example). The <img>'s width and height attributes were set to the desired size of the area into which to fit the respective image. And intrinsicsize was being fed the exact same values. E.g.

<img src="image-with-unknown-aspect-ratio.jpg"
         width="400"
         height="300"
         intrinsicsize="400x300"
         style="object-fit: contain">

See this isolated demo.

At least for me this result was unexpected, as to my understanding, the job of the intrinsicsize attribute was to provide layout stability for when not both axis' dimensions are provided.

But why would it interfere with object-fit?

I know that in the explainer you write:

the image would raster at these dimensions

and that this is why the result comes together as it does.

(Although, to be precise, it does not raster at the given dimensions, but instead it just scales the original image in both axis until it meets the given size. Otherwise this image would be pixelated, which it is not)

My question/food for thought now is: should this behavior still apply for when object-fit is active? What real world use case is there to stick with the current state of implementation?

Because the way it is now, when serving an <img> element, I would need to know if some stylesheet applies the object-fit property and if so I would need to strip the intrinsicsize attribute from my HTML output. Which is not obvious to figure out from the HTML perspective.

object-fit should either overrule intrinsicsize or there should be an additional CSS property that overrides the HTML sibling, as do CSS'es width and height.

yoavweiss commented 5 years ago

/cc @loonybear

eeeps commented 5 years ago

@Schepp —

In the linked Iron Man example, you have marked the <img> up as having an intrinsicsize="400x300". This is incorrect, as the linked image file is actually 310×445; marking it up as intrinsicsize="310x445" fixes the issue.

In my mind, object-fit and intrinsicsize are not conflicting at all, here. What is conflicting is the intrinsicsize and the actual size of the image.

I don't think that any CSS should have the power to override, or invalidate, intrinsicsize. It operates at a lower level, telling the browser to treat the linked resource as if it had certain dimensions. All CSS sizing happens after that and layers cleanly on top of that.

What to do in cases like yours, where the intrinsicsize is wrong, is an interesting question. Is intrinsicsize a hint, which can be overridden by reality, in case the hint was a lie? I think there are arguments for this, but I also understand the decision to, in case of conflict, keep the intrinsicsize dimensions, and let them override images’ actual native dimensions for the purposes of intrinsic sizing:

kornelski commented 5 years ago

I'd prefer if browsers recovered from invalid intrinsic size. If it's only a hint, there's no risk of breaking pages when its used: if it's correct, pages are faster. If it's not correct, nothing has been lost compared to not having it.

I'm thinking about a use-case of a CDN that automatically adds intrinsicsize to all <img> elements.

HTML and images can have different cache times. Images could be from 3rd party origins that can modify them at any time. Even if HTML is modified to perfectly match images at particular time from particular perspective seen by the CDN, there's still no guarantee that this will match what the browser gets.

Of course authors are not supposed to do silly things like images that suddenly vary in size (or are UA sniffed or random or whatever), but tooling deployed at large scale inevitably runs into authors who do silly things.


It'd be fine if browser ignored off-by-one errors or 1% difference in aspect ratio (not noticeable by users). For example, if I use this together with Client-Hints I may give you a size for 1x images, and the 2x images might end up having a sliiighly different aspect ratio if their dimensions aren't evenly divisible by 2.

Schepp commented 5 years ago

I think I'm with @kornelski here, as the main motivation behind this attribute is to provide layout-stability, not to offer even more possibilities to manipulate images. The latter is just a side effect of the chosen way of implementation.

Also, the more I think about it, the more I wonder why suggestions of a CSS property named aspect-ratio were never picked up. See:

aspect-ratio CSS property (2012) css-aspect-ratio-proposal (2014) WICG/aspect-ratio (2016)

eeeps commented 5 years ago

@schepp Good news: CSS aspect-ratio is also under active development.

There's a bit in the middle of this article where I opine why aspect-ratio wouldn't be great for images that don't fit within pre-defined layout slots (tl;dr one-off images would require high-specificity inline styles, which sacrifice the separation of concerns).

There is, however, an interesting proposal within the aspect-ratio spec that proposes leveraging aspect-ratio in UA stylesheets in order to repurpose the ancient width and height attributes so that they’d provide the same layout stability wins that intrinsicsize does. Is it feasible? What are the tradeoffs vs intrinsicsize? Would universal implementation be harder or easier, vs intrinsicsize? I don't know, but I want to spend some time thinking about it.

eeeps commented 5 years ago

@loonybear @ojanvafai perhaps you can comment on @kornelski’s comment, and your decisions around the actual-intrinsic-size/ratio-is-different case?

[meta Qs: should we move this conversation to: https://github.com/ojanvafai/intrinsicsize-attribute/issues ?]

loonybear commented 5 years ago

I think one gain from intrinsic size is layout stability, which means the 'invalid intrinsic size' will not be corrected. As a result images might be distorted but this prevents contents from shifting around on the page.

  1. In responsive images case, all images in the src_set are expected to have same aspect-ratio, so setting intrinsic size on those images shouldn't end up with invalid intrinsic size.
  2. In non responsive image case, intrinsic size simply naturalWidth / naturalHeight (density corrected). And I'd encourage web developers to not put invalid value for intrinsicSize?

I am not that familiar with Client-Hints, is it allowed to have image sources with different aspect-ratio?

frank-dspeed commented 5 years ago

@loonybear maybe we need a general guide how to size images the right way in 2019 :+1: most people don't get the part with setting only width or height

loonybear commented 5 years ago

A general guide how to size images the right way in 2019 sounds like a fantastic idea. Thanks @frank-dspeed !

frank-dspeed commented 5 years ago

@loonybear https://github.com/WICG/intrinsicsize-attribute does that make sense i think this covers the most ? @kornelski @Schepp

kornelski commented 5 years ago

@frank-dspeed My concern is lack of error recovery in this feature. This means it's unsafe to set by a CDN, which has to deal with the fact that HTML and images have different cache times/UA-sniffing, etc., so a correct intrinsicsize can become an invalid intrinsicsize at any time for any client.

If it was safe to set inaccurate intrinsicsize, a CDN could make this an opportunistic optimization, even turned on by default for all its users.

But as it's specced currently, it can cause very visible layout breakage as a side effect. Currently this attribute is dangerous, and can't be set automatically for everyone. This makes it a niche feature — one more thing that authors have to remember to add, and add correctly, rather than have it fixed automatically for them.

frank-dspeed commented 5 years ago

@kornelski i am not sure which CDN provider rewrites HTML as opportunistic optimization i think that should be a bad idea anyway! as you pointed out also width height isn't save nothing is save in this situation i think this is not related to this feature alone.

if a cdn really rewrites HTML on Delivery then they need to track also if images did change. Thats my opinion

kornelski commented 5 years ago

@kornelski Cloudflare rewrites HTML for performance. I work for Cloudflare on a team responsible for this "bad idea".

We can rewrite HTML in complex ways. We already have minification, changing of scripts to be deferred/async, and we're going to allow arbitrary JS-based rewrites on-the-fly via Edge Workers.

We have ability to add intrinsicsize automatically, but I don't think we can, no matter how hard we try, to guarantee it'll be 100% correct 100% of the time. Varying cache times, possibility of the origin doing UA sniffing and Client Hints, and cost and complexity of keeping globally-distributed caches in perfect sync makes it unworkable. Even if we did at the CDN everything perfectly 100% of the time, images and HTML could still get out of sync due to browser's own cache evicting only some of the resources.

So the punishing nature of the implementation kills this feature for me. If it had only upsides, and no downsides, it'd be an obvious win. But it gives some upsides at cost of page-breaking downsides, and that is a very hard sell for risk-averse sites.

frank-dspeed commented 5 years ago

@kornelski oh i like cloudflare in general and your right they rewrite html in some cases. i have coded always my own cloudflare like ADC (Application Delivery Controler)

The way to solve this would be Tracking State then your rewrite should also version the rewrited assets example image.jpg?md5hash this way you can invalidate the cache of the client browser via setting a new hash.

Maybe your team is big enough to implament something like Cache invalidate headers and https://developer.mozilla.org/en-US/docs/Web/API/Cache/delete

Its a big topic and will need some man power overall but as this is your primary company goal you could put some resources at this.

you should be able to inject a extra cloudflare-cache.js into the html that does the client side managment of the cache.

but my overall conclusion about your case with intrensicsize is don't apply it for your customers. you can write a blog about it when they want to use it they use it. On the long run you need to implament maybe as payed extra feature client side cache managment for cloudflare in general.

phistuck commented 5 years ago

The way to solve this would be Tracking State then your rewrite should also version the rewrited assets example image.jpg?md5hash this way you can invalidate the cache of the client browser via setting a new hash.

Using query strings for static resources is not recommended due to proxies. Adding the hash in the path is more recommended, as far as I know.

frank-dspeed commented 5 years ago

@phistuck your probally right with that it was only a fast written concept with not much deep tought about it because of the fact that i don't get payed for that :D i only wanted to point him into the right direction because i like the product he is working on and i am passionated about HTML delivery i think he can handle that the right way :)

kornelski commented 5 years ago

Changing image URLs to some variant that guarantees content won't change solves the immediate problem of caches getting out of sync, but doesn't solve the overall problem of breaking customers' websites (if they do have images that change or have variants selected via server-side negotiation, preventing that from working is a breakage, too).

but my overall conclusion about your case with intrensicsize is don't apply it for your customers

That's my conclusion too, and I think it's a missed opportunity. There are tons of "best practices" to follow, and very few sites follow them all. Anything that could be safely automated for everyone could have much larger impact than just asking everyone to learn and put more effort in.

frank-dspeed commented 5 years ago

@kornelski i know i am a big fan of this goal too! Get a faster better web is really fantastic in general i was thinking a lot about how to do that my result about that is that you will need a lot of man power to apply tweaks for coders that can't code :) so teaching them is maybe more realistic then putting 100+ pro coders on a CDN Project that does it for the coders.

many people did try to apply that serverside you probally remember Google PageSpeed Module for nginx and apache and so on but its a pain in the ass with caching and all that.

dvoytenko commented 5 years ago

Can this be solved if a stylesheet had (with a very low specificity):

img[intrinsicsize] {
  object-fit: contain;
}

?

duracell80 commented 3 years ago

This was an interesting read and to contribute I noticed something ...

Intrinsicsize of 200x300 seems to be devoid of semantic meaning as there are no units. Avoiding the use of x in favour of a colon could allow ... "200pw:300ph" which may be more helpful to a browser as well as a human. ph meaning height in pixels and there's a possibility there to push ew as width in ems.

The other thing I noticed is that generally when ratio is preserved only one dimension should be given. The other woul then be 100% or auto.

Just a couple of observations on an enjoyable read.