ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Discussion: Allow lazy-loading img element #24233

Closed cramforce closed 4 years ago

cramforce commented 5 years ago

Chromium recently shipped the loading=lazyattribute for img and other browsers seem to be on track to gain support as well.

With that, I'm wondering if we are ready to make image elements of the form

<img 
    loading=lazy 
    decoding=async 
    width=$width 
    height=$height 
    src=… 
    srcset=…
    sizes=…>

valid?

CC @Gregable @dvoytenko

Gregable commented 5 years ago

@honeybadgerdontcare

Gregable commented 5 years ago

I assume this is only in <noscript>?

sparhami commented 5 years ago

We added support for these in noscript images in #23765.

In addition to loading="lazy", we would want to require using intrinsicsize to ensure layout stability, otherwise we could only support fixed/fill layouts.

cramforce commented 5 years ago

@Gregable Nope, specifically outside of noscript. It would requires rewriting of src/srcset to the AMP Cache like on amp-img, of course.

The thesis is basically: If the native image element behaves like we want it to, then we should allow it.

@sparhami We don't want to require it, I think. But we should allow it. Unfortunately the spec discussion on intrinsicsize hasn't conclusively ended yet.

jridgewell commented 5 years ago

I think intrinsicsize is dead.

Gregable commented 5 years ago

Ah, ok.

Would this need any special handling for layout attributes? Lightbox-gallery? Would we also want to allow in <amp-list> templates?

cramforce commented 5 years ago

Let's start simple. But autolight-box is a good point to consider. CC @nainar We should probably catalogue all features of amp-img and see which ones

cramforce commented 5 years ago

Another point on intrinsicsize: If we require width and height folks can still make responsive images, just using the old padding-top hack. If there is a nicer platform solution that can just be added.

nainar commented 5 years ago

@alanorozco for auto lightbox.

dvoytenko commented 5 years ago

I think if we have lazy support and unsized media feature policy - we could allow native img. Before then - it doesn't solve our key requirements yet.

cramforce commented 5 years ago

@dvoytenko If we require width and height we don't need unsized media policy, right?

dvoytenko commented 5 years ago

That's true. As long as they are both specified and both are valid numbers.

cramforce commented 5 years ago

Yeah, we still have the validator. That is why we don't need the browser policy.

cramforce commented 5 years ago

Although I could also see to require application of the policy in meta tag and then not requiring width and height.

dvoytenko commented 5 years ago

@cramforce I think we can definitely start with width and height as you suggested above. Supporting unsized media policy would go much deeper with more validator changes.

cramforce commented 5 years ago

It seems that consensus is now moving towards inferring aspect ratio from width and height attributes (the same thing that AMP does) https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/hbhKRuBzZ4o/pmehGGHKAAAJ

One difference compared to AMP is that if those values are wrong, things will still jump. That is definitely a reason to concern. E.g. width=1 height=1 on every img would be valid, but jumpy.

dvoytenko commented 5 years ago

@cramforce Yeah. That would take a metric to report and resolve.

cramforce commented 5 years ago

I'm actually not 100% certain my interpretation is correct. I'll ask on the blink-dev thread.

cramforce commented 5 years ago

OK, this was clarified. Current intent is to give the image priority, so this would be a jank vector.

alanorozco commented 5 years ago

RE: auto-lightbox, we would only need to change how the extension scans. We also might need to change how lightbox-gallery scans and picks images.

dvoytenko commented 5 years ago

@cramforce Do we have any concern that loading=lazy might break preloading guarantees if we ever allowed non-cache src for images on cache?

cramforce commented 5 years ago

We couldn't allow those,

On Mon, Sep 16, 2019 at 11:26 AM Dima Voytenko notifications@github.com wrote:

@cramforce https://github.com/cramforce Do we have any concern that loading=lazy might break preloading guarantees if we ever allowed non-cache src for images on cache?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ampproject/amphtml/issues/24233?email_source=notifications&email_token=AAAV4TYNQ7NGRXGUN73MSILQJ7FV7A5CNFSM4IQHDL62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD62CEWY#issuecomment-531898971, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAV4T3RTMJG3B27E7BQCQ3QJ7FV7ANCNFSM4IQHDL6Q .

westonruter commented 4 years ago

Aside: Allowing img[loading=lazy] would also open the door to allowing the picture element. This would be helpful for serving existing web content in AMP since it can be difficult/impossible to translate picture into amp-img. We've tried in the context of the AMP plugin: https://github.com/ampproject/amp-wp/issues/1316.

cramforce commented 4 years ago

Closing in favor of #29786