AMP's srcset/sizes is different from browser's native implementation #11575

kraftpixel · 2017-10-06T07:43:23Z

What's the issue?

AMP's sizes attribute doesn't work in the same way as a browser's native implementation. AMP-IMG tag doesn't seem to be respecting the width of the element set by SIZES attribute and ends up loading the image based on the entire viewport width.

This was pointed out by our client. The test was done on a desktop browser. This does not happen normally as users will generally only land on AMP pages from a mobile device and these devices generally have a width below 600px. I'am mentioning the issue here just in case it's possible to address this difference.

How do we reproduce the issue?

Please disable caching through 'Developer Tools' > 'Network' tab > 'Disable Cache' then try out the following links on desktop to observe the difference.
Note : sizes="300px" in both the following cases.
Native Implementation : https://codepen.io/KraftPixel/pen/JrMQaJ
AMP Implementation : https://codepen.io/KraftPixel/pen/JrMQpN
For reference : Here is a screenshot of the output on a 1920x1080 -> https://i.imgur.com/g2jFaxu.png

What browsers are affected?

Tested on Google Chrome (Desktop)

Which AMP version is affected?

1506977814714

aghassemi · 2017-10-06T17:47:16Z

/cc @dvoytenko

dvoytenko · 2017-10-06T18:09:20Z

@cramforce is this the way you wanted this in the last PR?

cramforce · 2017-10-06T18:54:53Z

I did not realize there was the action-at-a-distance of handling the sizes attribute in framework land and srcset on the image itself.

I suppose we could revert to old behavior if the sizes attribute is present on the amp-img element as a quick fix?

I still think that going forward we should just use the native implementation here.

dvoytenko · 2017-10-06T20:48:02Z

@cramforce I suppose either way. Maybe we should just copy srcset/sizes from amp-img to img and leave the current implementation in place as a polyfill for those that do not support it?

cramforce · 2017-10-07T16:18:26Z

+1

ampprojectbot · 2017-10-30T20:06:48Z

This is a high priority issue but it hasn't been updated in awhile. Do you have any updates?

ampprojectbot · 2017-11-20T21:04:45Z

This is a high priority issue but it hasn't been updated in awhile. Do you have any updates?

adelinamart · 2017-11-20T21:26:32Z

Since is a high priority I'm assigning it to you @dvoytenko Feel free to re-assign.

ampprojectbot · 2017-12-04T21:12:40Z

This is a high priority issue but it hasn't been updated in awhile. @dvoytenko Do you have any updates?

ampprojectbot · 2017-12-25T21:13:20Z

This is a high priority issue but it hasn't been updated in awhile. @dvoytenko Do you have any updates?

ampprojectbot · 2018-01-15T21:19:37Z

This is a high priority issue but it hasn't been updated in awhile. @dvoytenko Do you have any updates?

tbll · 2018-01-27T19:39:33Z

Here is an example:

<!doctype html>

<html amp lang="en">
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
    <script async src="https://cdn.ampproject.org/v0.js"></script>
    <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
    <style amp-custom>
      img, amp-img {
        width: 10vw;
        height: auto;
      }
    </style>
  </head>
  <body>
    <amp-img src='http://placehold.it/720x440'
      width='720' height='480'
      sizes="10vw"
      srcset="http://placehold.it/360x240 360w, http://placehold.it/720x480 720w, http://placehold.it/1080x720 1080w,  http://placehold.it/1440x960 1440w">
    </amp-img>
    <img src='http://placehold.it/720x440'
      width='720' height='480'
      sizes="10vw"
      srcset="http://placehold.it/360x240 360w, http://placehold.it/720x480 720w, http://placehold.it/1080x720 1080w,  http://placehold.it/1440x960 1440w">
  </body>
</html>

tbll · 2018-01-27T19:47:34Z

If I understand correctly, <amp-img> assumes sizes=100vw not matter what. This totally defeat the purpose of <amp-img>. All my amp pages download the highest resolution of my images :(

amphtml-team · 2018-01-27T20:09:26Z

Yeah. Clearly wrong. We'll fix asap.

…

On Jan 27, 2018 11:47 AM, "Almen Munsel" ***@***.***> wrote: If I understand correctly, <amp-img> assumes sizes=100vw not matter what. This totally defeat the purpose of <amp-img>. All my amp pages download the highest resolution of my images :( — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#11575 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANd2kBK2uB7tTeA-KxxDrbkxlXkVtQ8Pks5tO31egaJpZM4PwHNU> .

cramforce · 2018-01-28T21:08:34Z

Sizes should not be ignored. My first change was just bringing it towards native srcset. Lets fix, but primarily lets just use native srcset and be done with it. There are no longer any browser issues. We only need to dynamically create a missing sizes when needed.

aghassemi · 2018-01-29T22:43:07Z

Logging some offline discussions:

1- We will move to full native support and remove AMP-specific logic from processing srcset
2- We may add a new opt-in to evaluate srcset based on container width rather than viewport width ( useful for cases such as thumbnail grids). This might endup being a "best-effort" and ignored in certain situations (e.g. hero images).

aghassemi · 2018-01-30T17:02:50Z

/cc @ericlindley-g

ampprojectbot · 2018-02-19T21:38:55Z

This is a high priority issue but it hasn't been updated in awhile. @dvoytenko Do you have any updates?

aghassemi · 2018-02-22T00:51:22Z

so we will either do 2 or 3. Alternative name suggestions:
for (2): sizes-behavior=<native, layout>, use-native-sizes , ??
for (3): native-sizes

cramforce · 2018-04-27T16:13:49Z

Any update here?

Not having this is actually a problem for our webpackaging project.

Eventually @twifkak's preload generation needs to mimick the same algorithm.

@adelinamart Lets track this as a dependency for WPK.

aghassemi · 2018-05-02T01:24:59Z

@cathyxz has started working on a fix. Thanks Cathy!

cathyxz · 2018-05-23T22:25:29Z

Just an update on this. We're going to ramp up this experiment on a weekly basis: 1%, 10%, 30%, 50%, 100%. Assuming no P0/rollbacks, we'll have this 100% in production in roughly 4 weeks and I'll cleanup the experiment in week 5. If anyone wants to opt in for testing, this is currently 100% in canary (dev channel), and under the experiment 'amp-img-native-srcset'.

One thing missing here is I think that we need to generate a sizes attribute if it is not present. This generated sizes attribute should select a src from the srcset based on the screen width.

^ I'm tracking this, and will start ramping that up when we're a bit more certain that 'amp-img-native-srcset' isn't breaking anyone.

cramforce added this to the Pending Triage milestone Oct 7, 2017

cramforce added P1: High Priority Category: Presentation Type: Bug labels Oct 7, 2017

ampprojectbot modified the milestones: Pending Triage, Backlog Bugs Oct 9, 2017

adelinamart assigned dvoytenko Nov 20, 2017

tbll mentioned this issue Jan 27, 2018

amp-img: wrong source is picked #13084

Closed

cramforce assigned aghassemi Jan 28, 2018

This was referenced Jan 31, 2018

amp-img: Support both native (viewport based) and container-based srcset selection #13178

Closed

Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' solution ampproject/amp-wp#921

Merged

kienstra mentioned this issue Feb 6, 2018

Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout ampproject/amp-wp#937

Merged

aghassemi mentioned this issue Feb 22, 2018

Status Update 2018-02-19 #13549

Closed

cathyxz mentioned this issue Feb 22, 2018

amp-image-lightbox thumbnail with extra zoom image #12724

Closed

aghassemi modified the milestones: Backlog Bugs, Sprint H2 February Feb 26, 2018

aghassemi modified the milestones: Sprint H2 February, Sprint H1 March 2018 Mar 5, 2018

cathyxz mentioned this issue Apr 26, 2018

Anything with a thumbnail and click to enlarge #14900

Closed

cathyxz assigned cathyxz and unassigned dvoytenko and aghassemi Apr 30, 2018

aghassemi modified the milestones: Sprint H1 March 2018, Sprint H1 May 2018 May 2, 2018

cathyxz mentioned this issue May 2, 2018

Update srcset / sizes to match browser behavior experiment #14975

Merged

10 tasks

cathyxz mentioned this issue May 10, 2018

Ramp up amp-img-native-srcset experiment to 1% #15200

Merged

cathyxz mentioned this issue May 21, 2018

Ramp up native srcset experiment to 10% #15459

Merged

This was referenced May 30, 2018

Up native srcset experiment threshold to 30% #15667

Merged

Ramp up native srcset experiment to 70% #15754

Merged

ampprojectbot modified the milestones: Sprint H1 May 2018, Backlog Bugs Jun 5, 2018

cathyxz mentioned this issue Jun 15, 2018

Ramp up native srcset experiment to 100% #16063

Merged

This was referenced Jun 29, 2018

Remove native srcset experiment and update fallback logic #16404

Merged

[amp-lightbox-gallery] upgrade srcset on open or zoom to handle thumbnail usecase #12929

Closed

cathyxz closed this as completed in #16404 Jul 9, 2018

aghassemi mentioned this issue Nov 29, 2018

[MASTER FEATURE] <amp-img> autogenerate sizes #19513

Closed

cathyxz mentioned this issue Aug 28, 2019

✨Add img-sizes attribute to amp-img. #24223

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AMP's srcset/sizes is different from browser's native implementation #11575

AMP's srcset/sizes is different from browser's native implementation #11575

kraftpixel commented Oct 6, 2017 •

edited

Loading

aghassemi commented Oct 6, 2017

dvoytenko commented Oct 6, 2017

cramforce commented Oct 6, 2017

dvoytenko commented Oct 6, 2017

cramforce commented Oct 7, 2017

ampprojectbot commented Oct 30, 2017

ampprojectbot commented Nov 20, 2017

adelinamart commented Nov 20, 2017

ampprojectbot commented Dec 4, 2017

ampprojectbot commented Dec 25, 2017

ampprojectbot commented Jan 15, 2018

tbll commented Jan 27, 2018

tbll commented Jan 27, 2018

amphtml-team commented Jan 27, 2018 via email

cramforce commented Jan 28, 2018

aghassemi commented Jan 29, 2018

aghassemi commented Jan 30, 2018

ampprojectbot commented Feb 19, 2018

aghassemi commented Feb 22, 2018

cramforce commented Apr 27, 2018

aghassemi commented May 2, 2018

cathyxz commented May 23, 2018

AMP's srcset/sizes is different from browser's native implementation #11575

AMP's srcset/sizes is different from browser's native implementation #11575

Comments

kraftpixel commented Oct 6, 2017 • edited Loading

What's the issue?

How do we reproduce the issue?

What browsers are affected?

Which AMP version is affected?

aghassemi commented Oct 6, 2017

dvoytenko commented Oct 6, 2017

cramforce commented Oct 6, 2017

dvoytenko commented Oct 6, 2017

cramforce commented Oct 7, 2017

ampprojectbot commented Oct 30, 2017

ampprojectbot commented Nov 20, 2017

adelinamart commented Nov 20, 2017

ampprojectbot commented Dec 4, 2017

ampprojectbot commented Dec 25, 2017

ampprojectbot commented Jan 15, 2018

tbll commented Jan 27, 2018

tbll commented Jan 27, 2018

amphtml-team commented Jan 27, 2018 via email

cramforce commented Jan 28, 2018

aghassemi commented Jan 29, 2018

aghassemi commented Jan 30, 2018

ampprojectbot commented Feb 19, 2018

aghassemi commented Feb 22, 2018

cramforce commented Apr 27, 2018

aghassemi commented May 2, 2018

cathyxz commented May 23, 2018

kraftpixel commented Oct 6, 2017 •

edited

Loading