From e8c3e595baa72810d2260a2192f9466db81d2463 Mon Sep 17 00:00:00 2001 From: Sepand Parhami Date: Mon, 26 Aug 2019 17:41:56 -0700 Subject: [PATCH 1/4] Add `img-sizes` attribute to `amp-img`. This allows specifying the `sizes` to use for an image, without getting the layout sizing behavior of `sizes` AMP adds. --- builtins/amp-img.js | 34 ++++++++++++-- test/unit/test-amp-img.js | 94 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 3 deletions(-) diff --git a/builtins/amp-img.js b/builtins/amp-img.js index 1593ab8ba730..9e4eeff85142 100644 --- a/builtins/amp-img.js +++ b/builtins/amp-img.js @@ -27,7 +27,8 @@ import {registerElement} from '../src/service/custom-element-registry'; const TAG = 'amp-img'; /** - * Attributes to propagate to internal image when changed externally. + * Attributes to propagate to internal image when changed externally. The + * `sizes` attribute is handled separately to prioritize `img-sizes`. * @type {!Array} */ const ATTRIBUTES_TO_PROPAGATE = [ @@ -39,7 +40,6 @@ const ATTRIBUTES_TO_PROPAGATE = [ 'aria-labelledby', 'srcset', 'src', - 'sizes', ]; export class AmpImg extends BaseElement { @@ -69,6 +69,30 @@ export class AmpImg extends BaseElement { this.sizesWidth_ = 0; } + /** + * @return {?string} The sizes value to use for the img, preferring + * `img-sizes` and falling back to `sizes`. + */ + getSizesValueToUse_() { + return ( + this.element.getAttribute('img-sizes') || + this.element.getAttribute('sizes') + ); + } + + /** + * If present, propagates the `img-sizes` or `sizes` value to the internal + * img. + */ + propagateSizes() { + const sizes = this.getSizesValueToUse_(); + if (sizes === null) { + this.img_.removeAttribute('sizes', sizes); + } else { + this.img_.setAttribute('sizes', sizes); + } + } + /** @override */ mutatedAttributesCallback(mutations) { if (this.img_) { @@ -97,6 +121,9 @@ export class AmpImg extends BaseElement { this.img_, /* opt_removeMissingAttrs */ true ); + if (mutations['img-sizes'] || mutations['sizes']) { + this.propagateSizes(); + } guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_); } } @@ -176,6 +203,7 @@ export class AmpImg extends BaseElement { } this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_); + this.propagateSizes(); guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_); this.maybeGenerateSizes_(); this.applyFillContent(this.img_, true); @@ -194,7 +222,7 @@ export class AmpImg extends BaseElement { return; } // No need to generate sizes if already present. - const sizes = this.element.getAttribute('sizes'); + const sizes = this.getSizesValueToUse_(); if (sizes) { return; } diff --git a/test/unit/test-amp-img.js b/test/unit/test-amp-img.js index 0c25f28225f3..504f989e64fb 100644 --- a/test/unit/test-amp-img.js +++ b/test/unit/test-amp-img.js @@ -188,6 +188,83 @@ describe('amp-img', () => { }); }); + it('should propagate srcset and img-sizes', async () => { + const ampImg = await getImg({ + src: '/examples/img/sample.jpg', + srcset: SRCSET_STRING, + 'img-sizes': '(max-width: 320px) 640px, 100vw', + width: 320, + height: 240, + }); + const img = ampImg.querySelector('img'); + expect(img.getAttribute('srcset')).to.equal(SRCSET_STRING); + expect(img.getAttribute('sizes')).to.equal( + '(max-width: 320px) 640px, 100vw' + ); + }); + + it('should prefer img-sizes to sizes', async () => { + const ampImg = await getImg({ + src: '/examples/img/sample.jpg', + srcset: SRCSET_STRING, + sizes: '(max-width: 320px) 640px, 100vw', + 'img-sizes': '640px', + width: 320, + height: 240, + }); + const img = ampImg.querySelector('img'); + expect(img.getAttribute('srcset')).to.equal(SRCSET_STRING); + expect(img.getAttribute('sizes')).to.equal('640px'); + }); + + it('should update sizes on mutation', async () => { + const ampImg = await getImg({ + src: '/examples/img/sample.jpg', + srcset: SRCSET_STRING, + sizes: '640px', + width: 300, + height: 200, + }); + const impl = ampImg.implementation_; + + ampImg.setAttribute('sizes', '50vw'); + impl.mutatedAttributesCallback({sizes: '50vw'}); + + expect(impl.img_.getAttribute('sizes')).to.equal('50vw'); + }); + + it('should update sizes on img-sizes mutation', async () => { + const ampImg = await getImg({ + src: '/examples/img/sample.jpg', + srcset: SRCSET_STRING, + 'img-sizes': '640px', + width: 300, + height: 200, + }); + const impl = ampImg.implementation_; + + ampImg.setAttribute('img-sizes', '50vw'); + impl.mutatedAttributesCallback({'img-sizes': '50vw'}); + + expect(impl.img_.getAttribute('sizes')).to.equal('50vw'); + }); + + it('should ignore sizes mutations when img-sizes is used', async () => { + const ampImg = await getImg({ + src: '/examples/img/sample.jpg', + srcset: SRCSET_STRING, + 'img-sizes': '640px', + width: 300, + height: 200, + }); + const impl = ampImg.implementation_; + + ampImg.setAttribute('sizes', '50vw'); + impl.mutatedAttributesCallback({sizes: '50vw'}); + + expect(impl.img_.getAttribute('sizes')).to.equal('640px'); + }); + describe('#fallback on initial load', () => { let el; let impl; @@ -530,6 +607,23 @@ describe('amp-img', () => { }); }); + it('should not generate sizes for amp-imgs that already have img-sizes', async () => { + const ampImg = await getImg({ + src: '/examples/img/sample.jpg', + srcset: SRCSET_STRING, + 'img-sizes': '50vw', + width: 300, + height: 200, + }); + + const impl = ampImg.implementation_; + impl.buildCallback(); + await impl.layoutCallback(); + + const img = impl.img_; + expect(img.getAttribute('sizes')).to.equal('50vw'); + }); + it('should not generate sizes for amp-imgs with layout intrinsic', () => { let impl; return getImg({ From 31ddaf470d3d7ede07bfb51c2136e864dd6cfd20 Mon Sep 17 00:00:00 2001 From: Sepand Parhami Date: Mon, 26 Aug 2019 17:56:18 -0700 Subject: [PATCH 2/4] Add `img-sizes` documentation. --- builtins/amp-img.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builtins/amp-img.md b/builtins/amp-img.md index fa0b36b6315d..fd1021d95627 100644 --- a/builtins/amp-img.md +++ b/builtins/amp-img.md @@ -125,6 +125,10 @@ For the `` 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 `` as `sizes`. In contrast, this does not affect the width of the `` element and is only used to select one of the sources from the `srcset`. + **alt** A string of alternate text, similar to the `alt` attribute on `img`. From b5fcd3d4455e029edc536bea446723d7415980bb Mon Sep 17 00:00:00 2001 From: Sepand Parhami Date: Tue, 27 Aug 2019 10:43:38 -0700 Subject: [PATCH 3/4] Review feedback. --- builtins/amp-img.js | 2 +- builtins/amp-img.md | 2 +- test/unit/test-amp-img.js | 16 ---------------- validator/validator-main.protoascii | 2 ++ 4 files changed, 4 insertions(+), 18 deletions(-) diff --git a/builtins/amp-img.js b/builtins/amp-img.js index 9e4eeff85142..2126dca54093 100644 --- a/builtins/amp-img.js +++ b/builtins/amp-img.js @@ -121,7 +121,7 @@ export class AmpImg extends BaseElement { this.img_, /* opt_removeMissingAttrs */ true ); - if (mutations['img-sizes'] || mutations['sizes']) { + if (mutations['sizes']) { this.propagateSizes(); } guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_); diff --git a/builtins/amp-img.md b/builtins/amp-img.md index fd1021d95627..ef0676e79fa2 100644 --- a/builtins/amp-img.md +++ b/builtins/amp-img.md @@ -127,7 +127,7 @@ See [Responsive images with srcset, sizes & heights](https://amp.dev/documentati **img-sizes** -Like the `sizes` attribute, the value is applied to the underlying `` as `sizes`. In contrast, this does not affect the width of the `` element and is only used to select one of the sources from the `srcset`. +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`. **alt** diff --git a/test/unit/test-amp-img.js b/test/unit/test-amp-img.js index 504f989e64fb..0958ae107230 100644 --- a/test/unit/test-amp-img.js +++ b/test/unit/test-amp-img.js @@ -233,22 +233,6 @@ describe('amp-img', () => { expect(impl.img_.getAttribute('sizes')).to.equal('50vw'); }); - it('should update sizes on img-sizes mutation', async () => { - const ampImg = await getImg({ - src: '/examples/img/sample.jpg', - srcset: SRCSET_STRING, - 'img-sizes': '640px', - width: 300, - height: 200, - }); - const impl = ampImg.implementation_; - - ampImg.setAttribute('img-sizes', '50vw'); - impl.mutatedAttributesCallback({'img-sizes': '50vw'}); - - expect(impl.img_.getAttribute('sizes')).to.equal('50vw'); - }); - it('should ignore sizes mutations when img-sizes is used', async () => { const ampImg = await getImg({ src: '/examples/img/sample.jpg', diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii index 347fce175197..2d526f298df8 100644 --- a/validator/validator-main.protoascii +++ b/validator/validator-main.protoascii @@ -5210,6 +5210,8 @@ tags: { # attrs: { name: "object-fit" } attrs: { name: "object-position" } attrs: { name: "placeholder" } + # Used to set sizes on the without AMP's sizes behavior + attrs: { name: "img-sizes" } # attrs: { name: "[alt]" } attrs: { name: "[attribution]" } From 464f2604bb8d9a56d568e7953febbf0b52af0564 Mon Sep 17 00:00:00 2001 From: Sepand Parhami Date: Wed, 28 Aug 2019 13:29:39 -0700 Subject: [PATCH 4/4] Fix lint. --- builtins/amp-img.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtins/amp-img.js b/builtins/amp-img.js index 2126dca54093..8cc334756607 100644 --- a/builtins/amp-img.js +++ b/builtins/amp-img.js @@ -87,7 +87,7 @@ export class AmpImg extends BaseElement { propagateSizes() { const sizes = this.getSizesValueToUse_(); if (sizes === null) { - this.img_.removeAttribute('sizes', sizes); + this.img_.removeAttribute('sizes'); } else { this.img_.setAttribute('sizes', sizes); }