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

sparhami · 2019-08-27T00:46:33Z

This allows specifying the sizes to use for an <img>, without getting
the sizing behavior of sizes in AMP (which adds a width based on which media query matches).

For #21736

This allows specifying the `sizes` to use for an image, without getting the layout sizing behavior of `sizes` AMP adds.

builtins/amp-img.js

honeybadgerdontcare · 2019-08-27T17:10:38Z

For applying layout, "server-side rendering (SSR)" does not remove boilerplate when a sizes attribute is encountered due to additional calculation that is currently not handled within SSR. Would the presence of img-sizes also necessitate the non removal of the boilerplate?

jridgewell · 2019-08-27T17:27:35Z

builtins/amp-img.md

@@ -125,6 +125,10 @@ For the `<img>` tag in `HTML`, the `sizes` attribute is used in conjunction with

 See [Responsive images with srcset, sizes & heights](https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/art_direction) for usage of `sizes` and `srcset`.

+**img-sizes**
+
+Like the `sizes` attribute, the value is applied to the underlying `<img>` as `sizes`. In contrast, this does not affect the width of the `<amp-img>` element and is only used to select one of the sources from the `srcset`.


I don't understand what this does.

Reworded, see updated text

I still don't understand why this is necessary. Why wouldn't they just use sizes? How does having <amp-img img-sizes="..."><img sizes="..."></amp-img> have a different behavior than <amp-img sizes="..."><img sizes="..."></amp-img>?

Because of what sizes does in AMP: https://amp.dev/documentation/guides-and-tutorials/learn/common_attributes/#sizes

Unfortunately, AMP sets the width of an element based on the value that the sizes attribute resolves to. This prevents developers from using sizes, since they want it for picking the source, but do not want to actually set the <amp-img> to that width.

Since this behavior already exists, we cannot remove it (from amp-img or any other element).

For context, we discussed this in design review some time ago: #11575 (comment). I did not implement img-sizes because we thought that auto-sizes would suffice for most cases (and it did for WP), but from #21736, it seems like we do need a way to explicitly set <img> sizes without accidentally also setting AMP sizes. Sizes always has a side effect in AMP, and we could not remove that side effect due to backwards compatibility issues, hence the unfortunate introduction of a new attribute.

I made a few demos:

Auto sizes

Manual Sizes

Proposed img-sizes

img-sizes and manual sizes

Given the img-sizes and manual sizes are incompatible together, I think we need to ban using both at the same time. And if we do that, we could probably use a boolean attribute instead, saying, "behave like regular sizes".

I think it comes down to which one of these is syntactically less confusing:

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

(Src: #11575 (comment))

Either will achieve the intended purpose.

sparhami · 2019-08-27T17:48:50Z

For applying layout, "server-side rendering (SSR)" does not remove boilerplate when a sizes attribute is encountered due to additional calculation that is currently not handled within SSR. Would the presence of img-sizes also necessitate the non removal of the boilerplate?

@honeybadgerdontcare The new img-sizes doesn't affect layout like sizes does. This simply forwards the value to the img tag, and is only used for the browser selecting a src from the srcset.

cathyxz

The new img-sizes doesn't affect layout like sizes does. This simply forwards the value to the img tag, and is only used for the browser selecting a src from the srcset.

Could we double check and verify this for layout=intrinsic and layout=fixed-height? I think all other layouts override the width already with runtime CSS.

We had an issue in #23453, where the developer applied width: auto, height: auto, along with layout=intrinsic, and I think with position: absolute. In this case, auto-generating srcset and sizes broke their intended styling so we ended up removing auto-sizes for layout intrinsic.

It's possible that developer styling (e.g. width: auto), combined with layout=intrinsic may force the image size to be dependent on the natural size of the image, which would change depending on which image url was downloaded (which is impacted by img-sizes). So at least in that case, we may need to keep the boilerplate.

cathyxz

I think the changes in this PR look good to me. I don't have strong opinions on the syntax of how to represent this (whether we introduce a new img-sizes sizes attribute or a cancel-layout boolean attribute) since they enable the same thing. Maybe it would be good to get more general feedback on naming, since this is a component with extremely high usage, and once we introduce new attributes, it will be very difficult to rename them?

archon810 · 2019-08-29T23:13:23Z

From the engineering point of view, a parameter that changes the behavior makes a whole lot more sense here than a duplicate set of confusingly named sizes parameters.

dreamofabear · 2019-09-11T21:05:59Z

+1 to @dvoytenko's suggestion for a boolean attribute that disables AMP magic in images.

Maybe we can even include the "don't optimize image in cache" behavior under this flag, which IIRC blocked Cloudinary from using plain amp-img in their AMP integrations (#21500).

archon810 · 2019-12-06T04:35:22Z

Hi, any updates here? We are dying for this functionality.

CrystalOnScript · 2019-12-06T21:00:48Z

builtins/amp-img.md

@@ -125,6 +125,10 @@ For the `<img>` tag in `HTML`, the `sizes` attribute is used in conjunction with

 See [Responsive images with srcset, sizes & heights](https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/art_direction) for usage of `sizes` and `srcset`.

+**img-sizes**
+
+Like `sizes` above, sets the [`sizes`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#attr-sizes) attribute on the underlying `img`. Unlike `sizes`, this does not change the width of the `amp-img` and is only used by the browser for picking which source to use from the `srcset`.


I think this is trying to say:

The img-sizes attribute is used by the browser to pick up which source to from the srcset, unlike the above sizes attribute, it does not change with width of the amp-img.

amp-owners-bot · 2019-12-06T21:00:56Z

Hey @ampproject/wg-caching, these files were changed:

validator/validator-main.protoascii

honeybadgerdontcare · 2019-12-06T21:21:55Z

validator/validator-main.protoascii

@@ -5210,6 +5210,8 @@ tags: {  # <amp-img>
  attrs: { name: "object-fit" }
  attrs: { name: "object-position" }
  attrs: { name: "placeholder" }
+  # Used to set sizes on the <img> without AMP's sizes behavior
+  attrs: { name: "img-sizes" }


please keep attributes alphabetized by attribute name

honeybadgerdontcare · 2019-12-16T21:10:45Z

For applying layout, "server-side rendering (SSR)" does not remove boilerplate when a sizes attribute is encountered due to additional calculation that is currently not handled within SSR. Would the presence of img-sizes also necessitate the non removal of the boilerplate?

Any consideration around this?

sparhami · 2019-12-17T00:40:51Z

For applying layout, "server-side rendering (SSR)" does not remove boilerplate when a sizes attribute is encountered due to additional calculation that is currently not handled within SSR. Would the presence of img-sizes also necessitate the non removal of the boilerplate?

Any consideration around this?

Just as a status update for this PR, we're exploring using a boolean attribute to disable the layout portion rather than propagating img-sizes. As part of that boolean attribute, we could allow the boilerplate to be removed, as long as all the elements with a sizes attribute also have the boolean attribute for ignoring the layout implications of sizes.

Will keep this PR open for further discussion, but it looks like we will be moving to an alternate approach.

/cc @dvoytenko @jridgewell @caroqliu

mrjoro · 2020-06-11T17:51:39Z

Closing this PR for now since the last comment indicates we'll be going with a different approach. Please reopen if needed.

archon810 · 2020-06-11T18:55:57Z

@mrjoro @sparhami Where is the replacement PR we can follow though?

caroqliu · 2020-06-11T19:15:33Z

@archon810 #27083 was the alternative approach. Feel free to continue the discussion in #21736 which led to both this and that PR.

Add img-sizes attribute to amp-img.

e8c3e59

This allows specifying the `sizes` to use for an image, without getting the layout sizing behavior of `sizes` AMP adds.

googlebot added the cla: yes label Aug 27, 2019

Add img-sizes documentation.

31ddaf4

sparhami requested review from cathyxz and CrystalOnScript August 27, 2019 00:59

sparhami marked this pull request as ready for review August 27, 2019 00:59

sparhami mentioned this pull request Aug 27, 2019

amp-lightbox-gallery: Different images for thumbnail and detail #21736

Closed

cathyxz reviewed Aug 27, 2019

View reviewed changes

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

jridgewell requested changes Aug 27, 2019

View reviewed changes

Review feedback.

b5fcd3d

cathyxz reviewed Aug 28, 2019

View reviewed changes

Fix lint.

464f260

cathyxz approved these changes Aug 29, 2019

View reviewed changes

sparhami mentioned this pull request Sep 9, 2019

Design Review 2019-09-11 20:00 UTC (wg-performance, reviewer bot selection, email viewer, amp-img source) #23990

Closed

sparhami requested a review from dvoytenko September 11, 2019 21:00

CrystalOnScript reviewed Dec 6, 2019

View reviewed changes

honeybadgerdontcare reviewed Dec 6, 2019

View reviewed changes

twifkak added WG: caching and removed WG: caching labels Dec 6, 2019

amaltas assigned Gregable Jan 6, 2020

Gregable added the WG: caching label Jan 8, 2020

mrjoro closed this Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sparhami commented Aug 27, 2019 •

edited

Loading

honeybadgerdontcare commented Aug 27, 2019

jridgewell Aug 27, 2019

sparhami Aug 27, 2019

jridgewell Aug 27, 2019

sparhami Aug 27, 2019

cathyxz Aug 28, 2019

jridgewell Aug 28, 2019

cathyxz Aug 28, 2019

sparhami commented Aug 27, 2019

cathyxz left a comment •

edited

Loading

cathyxz left a comment

archon810 commented Aug 29, 2019

dreamofabear commented Sep 11, 2019

archon810 commented Dec 6, 2019

CrystalOnScript Dec 6, 2019

amp-owners-bot bot commented Dec 6, 2019

honeybadgerdontcare Dec 6, 2019

honeybadgerdontcare commented Dec 16, 2019

sparhami commented Dec 17, 2019

mrjoro commented Jun 11, 2020

archon810 commented Jun 11, 2020

caroqliu commented Jun 11, 2020

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

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

Conversation

sparhami commented Aug 27, 2019 • edited Loading

honeybadgerdontcare commented Aug 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparhami commented Aug 27, 2019

cathyxz left a comment • edited Loading

Choose a reason for hiding this comment

cathyxz left a comment

Choose a reason for hiding this comment

archon810 commented Aug 29, 2019

dreamofabear commented Sep 11, 2019

archon810 commented Dec 6, 2019

Choose a reason for hiding this comment

amp-owners-bot bot commented Dec 6, 2019

Choose a reason for hiding this comment

honeybadgerdontcare commented Dec 16, 2019

sparhami commented Dec 17, 2019

mrjoro commented Jun 11, 2020

archon810 commented Jun 11, 2020

caroqliu commented Jun 11, 2020

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

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

sparhami commented Aug 27, 2019 •

edited

Loading

cathyxz left a comment •

edited

Loading