Automatically generate sizes attribute for amp-img #20968

cathyxz · 2019-02-20T21:10:48Z

Experimentally implements #19513.
~~1. Does not add sizes as a style width if the tag is <amp-img>.~~
2. If a sizes attribute is not provided to <amp-img>, generate one in layoutCallback based on its current width.
3. In onMeasureChanged, if the image is resized to be larger, set a new sizes attribute.

Details at go/amp-img-sizes
Background at go/srcset-and-sizes

builtins/amp-img.js

…t in this class

jridgewell

Can you give an an ideal output for the image:

<amp-img alt="Hummingbird"
  src="images/hummingbird-wide.jpg"
  width="640"
  height="457"
  srcset="images/hummingbird-wide.jpg 640w,
            images/hummingbird-narrow.jpg 320w"
  sizes="auto">
</amp-img>

What should sizes be?

builtins/amp-img.js

src/custom-element.js

aghassemi · 2019-02-27T19:00:30Z

builtins/amp-img.js

@@ -63,6 +69,13 @@ export class AmpImg extends BaseElement {
    }
  }

+  /** @override */
+  onMeasureChanged() {


What was the conclusion on the question of:
"If viewport size does NOT change, but sizes attribute of an img is updated, do any browser fetch and render a new high res image"?

If no, then we can just do this in viewport-size change event instead of measureChanged

The answer to that question is yes for Firefox and Chrome, no for Safari (also sizes doesn't update on viewport change or image size change at all ever on Safari). The reason we have this in onMeasureChanged is because if the image changes size without a viewport change, updating the srcset will cause a refetch if the image size results in a different srcset being selected.

cathyxz · 2019-02-27T21:16:33Z

Re: your question:

Can you give an an ideal output for the image:

<amp-img alt="Hummingbird"
  src="images/hummingbird-wide.jpg"
  width="640"
  height="457"
  srcset="images/hummingbird-wide.jpg 640w,
            images/hummingbird-narrow.jpg 320w"
  sizes="auto">
</amp-img>

What should sizes be?

Great point. I think I need to rethink what happens when layout is not specified.

So, the value of sizes should really depend on its laid out dimensions, but automatically generated sizes should not effect the layout in any way.

So if we had

<amp-img alt="Hummingbird"
  src="images/hummingbird-wide.jpg"
  width="640"
  height="457"
  srcset="images/hummingbird-wide.jpg 640w,
            images/hummingbird-narrow.jpg 320w">
</amp-img>

This would be interpreted as Layout.FIXED, and sizes attribute would be (max-width: viewportWidth) 640px, 640px.

But if we had sizes=auto, under the current layout inference logic, we would interpret that as Layout.RESPONSIVE.

I think that is incorrect because I don't think sizes=auto should impact the actual layout or dimensions of the image. We can solve this problem in two ways:

Special case sizes='auto' so that we don't interpret this as Layout.RESPONSIVE.
Take out the syntax for sizes='auto', and make this auto-generate sizes feature a fallback that takes effect when developers don't specify this themselves.

UPDATE:
I'm currently going with option 2, we'll only autogenerate sizes when the sizes attribute isn't specified, and we'll leave sizes on <amp-img> alone, to behave consistently across all of AMP (sizes on AMP impacts layout).

googlebot · 2019-02-27T21:17:27Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

cathyxz · 2019-02-27T22:01:52Z

So sad. I love the commit suggested changes feature, but it doesn't play nice with our CLA bot. I'm going to ahem do some git history rewriting to fix that up now. T.T

googlebot · 2019-02-27T22:29:02Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

cathyxz · 2019-02-27T22:37:05Z

I know I have a merge conflict here, but it's pretty trivial to resolve the experiments.js file, and I only want to merge master once. So I'll fix that one at the end.

jridgewell

I think I'm confused because there's two separate issues going on:

What does the a regular img do with sizes? How is amp-img different?
What do we want to do with sizes="auto"?

We need to fix 1). But I don't think 2) has been well defined yet. At least, I don't know what the correct sizes output is for a given srcset input.

builtins/amp-img.js

cathyxz · 2019-02-28T21:10:29Z

Re: sizes="auto", I'd like to take that out for now (*).

The goal of this PR is as follows:

If the developer has defined srcset but not sizes, automatically calculate a sizes attribute for them.

A bit of background:

Browser treatment of `sizes` on `<img>`

I did a writeup on this at go/srcset-and-sizes. But the TLDR is it's a hint to the browser. Chrome behaves semi-intelligently when <img> has srcset but no sizes (it assumes that the image will be 100vw), and FF always chooses the largest src in the srcset when sizes is not present. Basically it acts like a hint to tell the browser, this is how big the image actually will be, don't assume it's 100vw, but empirically on my macbook, the browser picks one resolution higher than necessary. I suspect which resolution is actually selected also has something to do with screen resolution subject to browser discretion.

The main benefit here is that it is more accurate for Chrome instead of assuming 100vw, and it will have actual gains for FF so that it matches Chrome's behavior instead of automatically picking the largest src.

AMP's treatment of `sizes` on everything

@aghassemi and I discussed how this was super unfortunate naming, but AMP has its own interpretation of sizes, not specific to <amp-img>. Our documentation (https://www.ampproject.org/docs/design/amp-html-layout#sizes, https://www.ampproject.org/docs/design/responsive/control_layout#what-if-the-layout-attribute-isn%E2%80%99t-specified?) on this is inaccurate and confusing, but TLDR it's used for inferred layouts (when we supply sizes, we infer responsive layout) and sometimes sets element width.

We can't change this behavior due to backwards compatibility. Otherwise I would strongly advocate for changing it so that sizes means the same thing on <amp-img> as it means on <img>. But we'd still like developers to get the benefit of <img> sizes for responsive images, otherwise srcset is pointless (and even harmful) on Firefox. Therefore, we'd like to automatically generate a correct sizes attribute so people won't ever need to specify <img> sizes, and they can just interpret sizes on <amp-img> to mean whatever it means for the rest of AMP. We should document this very clearly.

Implementation Details

Two things resulted in this particular implementation as opposed to other alternatives considered in the DD.

Firefox by default always fetches a new img when the srcset calculation is different regardless of whether the img size increased or decreased. I think it's wasteful to fetch a lower res img when a higher res img is already downloaded and available.
We want to handle the case where the img changes (specifically increases) size due to CSS changes from amp-bind (without a viewport size change).

This resulted in the decision to put a specific pixel value in for sizes as opposed to one in vw, and the additional recalculation and reset-iff-larger logic in onMeasureChanged.

As to why we still propagate sizes when specified by developer

Since the logic happens in onMeasureChanged, it's somewhat expensive. If the developer sets sizes intending to specify an inferred layout per our current behavior (higher priority override of width), it should also result in a reasonable <img> sizes attribute. It won't handle the css-causes-img-size-change case, but should otherwise work well.

I'm taking out sizes="auto" because we currently have layout inference logic to set to Layout RESPONSIVE when sizes is specified. I'm not certain that we want sizes="auto" to result in Layout RESPONSIVE (even though this isn't terrible since Layout can always be overridden by the developer).

builtins/amp-img.js

jridgewell · 2019-02-28T21:23:09Z

builtins/amp-img.js

@@ -63,6 +69,13 @@ export class AmpImg extends BaseElement {
    }
  }

+  /** @override */
+  onMeasureChanged() {
+    if (isExperimentOn(this.getAmpDoc().win, 'amp-img-auto-sizes')) {


Store this in a private property in constructor.

Is it ok use this.getAmpDoc() in the constructor (it currently causes all amp-img tests to fail)? I can stub it if ok, but I've noticed that people generally put this line of code in buildCallback, but <amp-img> doesn't use buildCallback.

builtins/amp-img.js

cathyxz · 2019-03-01T00:33:11Z

I resolved merge conflicts and I believe tidied up all the lose ends on this PR. It should now be ready to merge when CI completes. The plan is to ramp this up gradually as an experiment over 4 weeks until it reaches 100% of prod, and then we'll update our documentation per #21191.

cathyxz · 2019-03-01T00:59:56Z

Sorry @jridgewell could you re-approve for the bundle size bot?

cathyxz · 2019-03-01T01:11:46Z

Thank you! xD

* Generate sizes if applicable for amp-img * Gate auto-generate sizes under an experiment flag * Add unit tests to for auto-sizes * Fix lint and type check * Fix isExperimentOn call parameter * Add logic to update the sizes attribute onMeasureChanged * Add a manual test case to verify amp-img with auto generated sizes * Fix x descriptor regex * Fix getAmpDoc stub in amp-img tests * Remove debugger statements * Fix regex * Stub ampdoc_ on custom element test because we are using an experiment in this class * Remove extra getLayoutWidth call * Address code review comments * Add early return and store experiment in constructor * Move experiment check to constructor * Unstub getAmpDoc in tests since its no longer used

googlebot added the cla: yes label Feb 20, 2019

cathyxz commented Feb 26, 2019

View reviewed changes

builtins/amp-img.js Show resolved Hide resolved

cathyxz added 13 commits February 26, 2019 12:55

Generate sizes if applicable for amp-img

f4a3a91

Gate auto-generate sizes under an experiment flag

bf510db

Add unit tests to for auto-sizes

06f7dea

Fix lint and type check

bf7408d

Fix isExperimentOn call parameter

c7196e1

Add logic to update the sizes attribute onMeasureChanged

9c635fc

Add a manual test case to verify amp-img with auto generated sizes

3eeb2e0

Fix x descriptor regex

6dda7b3

Fix getAmpDoc stub in amp-img tests

d70feaa

Remove debugger statements

2b3fb3d

Fix regex

5d36161

Stub ampdoc_ on custom element test because we are using an experimen…

fc2a44e

…t in this class

Remove extra getLayoutWidth call

ad83b0d

cathyxz force-pushed the feature/auto-sizes branch from 7e18c8d to ad83b0d Compare February 26, 2019 20:57

cathyxz marked this pull request as ready for review February 26, 2019 21:00

cathyxz requested a review from aghassemi February 26, 2019 21:00

amp-bundle-size bot requested a review from jridgewell February 26, 2019 21:24

jridgewell requested changes Feb 27, 2019

View reviewed changes

builtins/amp-img.js Outdated Show resolved Hide resolved

builtins/amp-img.js Outdated Show resolved Hide resolved

builtins/amp-img.js Outdated Show resolved Hide resolved

builtins/amp-img.js Show resolved Hide resolved

builtins/amp-img.js Show resolved Hide resolved

aghassemi reviewed Feb 27, 2019

View reviewed changes

googlebot added cla: no and removed cla: yes labels Feb 27, 2019

cathyxz force-pushed the feature/auto-sizes branch from 148cb51 to d7ecae6 Compare February 27, 2019 22:04

Address code review comments

d51622f

cathyxz force-pushed the feature/auto-sizes branch from 3344b28 to d51622f Compare February 27, 2019 22:28

googlebot added cla: yes and removed cla: no labels Feb 27, 2019

cathyxz requested a review from jridgewell February 27, 2019 22:36

ampproject deleted a comment from googlebot Feb 28, 2019

jridgewell reviewed Feb 28, 2019

View reviewed changes

builtins/amp-img.js Show resolved Hide resolved

jridgewell approved these changes Feb 28, 2019

View reviewed changes

cathyxz added 2 commits February 28, 2019 14:27

Add early return and store experiment in constructor

897be6d

Merge branch 'master' into feature/auto-sizes

cd0a143

aghassemi approved these changes Feb 28, 2019

View reviewed changes

jridgewell reviewed Feb 28, 2019

View reviewed changes

builtins/amp-img.js Outdated Show resolved Hide resolved

cathyxz added 2 commits February 28, 2019 16:10

Move experiment check to constructor

9feb7ae

Unstub getAmpDoc in tests since its no longer used

51ba1aa

cathyxz mentioned this pull request Mar 1, 2019

Document sizes for amp and amp-img correctly #21191

Closed

cathyxz requested a review from jridgewell March 1, 2019 01:00

jridgewell approved these changes Mar 1, 2019

View reviewed changes

cathyxz merged commit 141aca2 into ampproject:master Mar 1, 2019

cathyxz deleted the feature/auto-sizes branch March 1, 2019 01:11

This was referenced Mar 1, 2019

Launch amp-img-auto-sizes to 100% of canary #21202

Merged

Release amp-img-auto-sizes to 1% of production #21204

Closed

westonruter mentioned this pull request Mar 7, 2019

<amp-img> sizes attribute behavior seems different from non-AMP <img> #17053

Closed

cathyxz mentioned this pull request Apr 29, 2019

Wide images are overflowing with <amp-img> but not with non-AMP <img> #17084

Closed

CraigBusfield mentioned this pull request Jul 16, 2020

Support of imagesrcset and imagesizes attributes for preloading images #29302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically generate sizes attribute for amp-img #20968

Automatically generate sizes attribute for amp-img #20968

cathyxz commented Feb 20, 2019 •

edited

Loading

jridgewell left a comment •

edited

Loading

aghassemi Feb 27, 2019

cathyxz Feb 27, 2019

cathyxz commented Feb 27, 2019 •

edited

Loading

googlebot commented Feb 27, 2019

cathyxz commented Feb 27, 2019

googlebot commented Feb 27, 2019

cathyxz commented Feb 27, 2019

jridgewell left a comment

cathyxz commented Feb 28, 2019 •

edited

Loading

jridgewell Feb 28, 2019

cathyxz Feb 28, 2019 •

edited

Loading

cathyxz commented Mar 1, 2019 •

edited

Loading

cathyxz commented Mar 1, 2019

cathyxz commented Mar 1, 2019

Automatically generate sizes attribute for amp-img #20968

Automatically generate sizes attribute for amp-img #20968

Conversation

cathyxz commented Feb 20, 2019 • edited Loading

jridgewell left a comment • edited Loading

Choose a reason for hiding this comment

aghassemi Feb 27, 2019

Choose a reason for hiding this comment

cathyxz Feb 27, 2019

Choose a reason for hiding this comment

cathyxz commented Feb 27, 2019 • edited Loading

googlebot commented Feb 27, 2019

cathyxz commented Feb 27, 2019

googlebot commented Feb 27, 2019

cathyxz commented Feb 27, 2019

jridgewell left a comment

Choose a reason for hiding this comment

cathyxz commented Feb 28, 2019 • edited Loading

Browser treatment of sizes on <img>

AMP's treatment of sizes on everything

Implementation Details

As to why we still propagate sizes when specified by developer

jridgewell Feb 28, 2019

Choose a reason for hiding this comment

cathyxz Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

cathyxz commented Mar 1, 2019 • edited Loading

cathyxz commented Mar 1, 2019

cathyxz commented Mar 1, 2019

cathyxz commented Feb 20, 2019 •

edited

Loading

jridgewell left a comment •

edited

Loading

cathyxz commented Feb 27, 2019 •

edited

Loading

cathyxz commented Feb 28, 2019 •

edited

Loading

Browser treatment of `sizes` on `<img>`

AMP's treatment of `sizes` on everything

cathyxz Feb 28, 2019 •

edited

Loading

cathyxz commented Mar 1, 2019 •

edited

Loading