Automatically generate sizes attribute for amp-img (ampproject#20968)

* 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
bramanudom · Mar 22, 2019 · c063d2c · c063d2c
1 parent 027442e
commit c063d2c
Show file tree

Hide file tree

Showing 5 changed files with 323 additions and 3 deletions.
diff --git a/builtins/amp-img.js b/builtins/amp-img.js
@@ -15,10 +15,10 @@
  */
 
 import {BaseElement} from '../src/base-element';
+import {Layout, isLayoutSizeDefined} from '../src/layout';
 import {dev} from '../src/log';
 import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../src/utils/img';
 import {isExperimentOn} from '../src/experiments';
-import {isLayoutSizeDefined} from '../src/layout';
 import {listen} from '../src/event-helper';
 import {registerElement} from '../src/service/custom-element-registry';
 import {setImportantStyles} from '../src/style';
@@ -50,6 +50,15 @@ export class AmpImg extends BaseElement {
 
     /** @private {?UnlistenDef} */
     this.unlistenError_ = null;
+
+    /** @private {boolean} */
+    this.autoGenerateSizes_ = isExperimentOn(this.win, 'amp-img-auto-sizes');
+
+    /**
+     * The current width used by the automatically generated sizes attribute
+     * @private {number}
+     * */
+    this.sizesWidth_ = 0;
   }
 
   /** @override */
@@ -63,6 +72,13 @@ export class AmpImg extends BaseElement {
     }
   }
 
+  /** @override */
+  onMeasureChanged() {
+    if (this.autoGenerateSizes_) {
+      this.maybeGenerateSizes_();
+    }
+  }
+
   /** @override */
   preconnectCallback(onLayout) {
     // NOTE(@wassgha): since parseSrcset is computationally expensive and can
@@ -132,10 +148,69 @@ export class AmpImg extends BaseElement {
 
     this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
     guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_);
+    if (this.autoGenerateSizes_) {
+      this.maybeGenerateSizes_();
+    }
     this.applyFillContent(this.img_, true);
+
     this.element.appendChild(this.img_);
   }
 
+  /**
+   * This function automatically generates sizes for amp-imgs without
+   * the sizes attribute.
+   * @private
+   */
+  maybeGenerateSizes_() {
+    if (!this.img_) {
+      return;
+    }
+    // No need to generate sizes if already present.
+    const sizes = this.element.getAttribute('sizes');
+    if (sizes) {
+      return;
+    }
+    // Sizes is useless without the srcset attribute or if the srcset
+    // attribute uses the x descriptor.
+    const srcset = this.element.getAttribute('srcset');
+    if (!srcset || /[0-9]+x(?:,|$)/.test(srcset)) {
+      return;
+    }
+
+    const width = this.getLayoutWidth();
+    if (!this.shouldSetSizes_(width)) {
+      return;
+    }
+
+    const viewportWidth = this.getViewport().getWidth();
+
+    const entry = `(max-width: ${viewportWidth}px) ${width}px, `;
+    let defaultSize = width + 'px';
+
+    if (this.getLayout() !== Layout.FIXED) {
+      const ratio = Math.round(width * 100 / viewportWidth);
+      defaultSize = Math.max(ratio, 100) + 'vw';
+    }
+
+    const generatedSizes = entry + defaultSize;
+
+    this.mutateElement(() => {
+      this.img_.setAttribute('sizes', generatedSizes);
+    });
+    this.sizesWidth_ = width;
+  }
+
+  /**
+   * @param {number} newWidth
+   * @private
+   */
+  shouldSetSizes_(newWidth) {
+    if (!this.img_.hasAttribute('sizes')) {
+      return true;
+    }
+    return newWidth > this.sizesWidth_;
+  }
+
   /** @override */
   prerenderAllowed() {
     return this.prerenderAllowed_;

diff --git a/test/manual/amp-img-sizes.amp.html b/test/manual/amp-img-sizes.amp.html
@@ -0,0 +1,66 @@
+<!doctype html>
+<html ⚡>
+<head>
+  <meta charset="utf-8">
+  <title>AMP #0</title>
+  <link rel="canonical" href="amps.html" >
+  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+  <link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'>
+  <style amp-custom>
+    .large {
+      width: 600px;
+    }
+
+    .small {
+      width: 200px;
+    }
+
+    #img2 {
+      width: 50vw;
+    }
+
+    #img3 {
+      width: 50vw;
+    }
+  </style>
+  <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>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+  <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
+
+</head>
+<body>
+
+    <amp-state id="data">
+        <script type="application/json">
+          "small"
+        </script>
+      </amp-state>
+
+
+  <h1>Srcset and Sizes</h1>
+
+  <h2>Change size on bind</h2>
+  <button on="tap:AMP.setState({data: data == 'large' ? 'small' : 'large'})">Toggle Image Size</button>
+
+  <div id="container" class="small" [class]="data">
+    <amp-img srcset="/examples/img/hero@1x.jpg 641w,
+                    /examples/img/hero@2x.jpg 1282w"
+      width=641 height=480 layout=responsive noloading>
+    </amp-img>
+  </div>
+
+  <h2>Change size on viewport change</h2>
+  <h3>Layout RESPONSIVE</h3>
+  <amp-img id="img2" srcset="/examples/img/hero@1x.jpg 641w,
+    /examples/img/hero@2x.jpg 1282w"
+  width=641 height=480 layout=responsive noloading>
+  </amp-img>
+
+  <h2>Layout INTRINSIC</h2>
+  <amp-img id="img3" srcset="https://picsum.photos/400/400 400w,
+    https://picsum.photos/800/800 800w"
+    width=641 height=480 layout=intrinsic noloading>
+  </amp-img>
+
+</body>
+</html>
diff --git a/test/unit/test-amp-img.js b/test/unit/test-amp-img.js
@@ -16,11 +16,11 @@
 
 import {AmpImg, installImg} from '../../builtins/amp-img';
 import {BaseElement} from '../../src/base-element';
-import {LayoutPriority} from '../../src/layout';
+import {Layout, LayoutPriority} from '../../src/layout';
 import {Services} from '../../src/services';
 import {createCustomEvent} from '../../src/event-helper';
 import {createIframePromise} from '../../testing/iframe';
-import {toggleExperiment} from '../../src/experiments';
+import {isExperimentOn, toggleExperiment} from '../../src/experiments';
 
 describe('amp-img', () => {
   let sandbox;
@@ -367,6 +367,9 @@ describe('amp-img', () => {
 
     el.getPlaceholder = sandbox.stub();
     const impl = new AmpImg(el);
+    impl.getAmpDoc = function() {
+      return window.AMP.ampdoc;
+    };
     impl.buildCallback();
     impl.layoutCallback();
     const img = el.querySelector('img');
@@ -447,4 +450,173 @@ describe('amp-img', () => {
       expect(impl.togglePlaceholder).to.have.been.calledWith(false);
     });
   });
+
+  describe('auto-generate sizes', () => {
+
+    function getStubbedImg(attributes, layoutWidth) {
+      const el = document.createElement('amp-img');
+      for (const key in attributes) {
+        el.setAttribute(key, attributes[key]);
+      }
+      el.getResources = () => Services.resourcesForDoc(document);
+      el.getPlaceholder = sandbox.stub();
+      const impl = new AmpImg(el);
+      impl.createdCallback();
+      sandbox.stub(impl, 'getLayoutWidth').returns(layoutWidth);
+      sandbox.stub(impl, 'getLayout').returns(attributes['layout']);
+      el.toggleFallback = function() {};
+      el.togglePlaceholder = function() {};
+
+      impl.mutateElement = fn => fn();
+      impl.getViewport = function() {
+        return {
+          getWidth: () => windowWidth,
+        };
+      };
+      return impl;
+    }
+
+    beforeEach(() => {
+      toggleExperiment(window, 'amp-img-auto-sizes', true, true);
+    });
+
+    it('should not generate sizes for amp-imgs that already have sizes', () => {
+      let impl;
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      return getImg({
+        src: '/examples/img/sample.jpg',
+        srcset: SRCSET_STRING,
+        sizes: '50vw',
+        width: 300,
+        height: 200,
+      }).then(ampImg => {
+        impl = ampImg.implementation_;
+        impl.buildCallback();
+        return impl.layoutCallback();
+      }).then(() => {
+        const img = impl.img_;
+        expect(img.getAttribute('sizes')).to.equal('50vw');
+      });
+    });
+
+    it('should not generate sizes for amp-imgs without srcset', () => {
+      let impl;
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      return getImg({
+        src: '/examples/img/sample.jpg',
+        width: 300,
+        height: 200,
+      }).then(ampImg => {
+        impl = ampImg.implementation_;
+        impl.buildCallback();
+        return impl.layoutCallback();
+      }).then(() => {
+        const img = impl.img_;
+        expect(img.getAttribute('sizes')).to.be.null;
+      });
+    });
+
+    it('should not generate sizes for amp-imgs with x descriptors', () => {
+      let impl;
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      return getImg({
+        srcset: '/examples/img/hero@1x.jpg, /examples/img/hero@2x.jpg 2x',
+        width: 300,
+        height: 200,
+      }).then(ampImg => {
+        impl = ampImg.implementation_;
+        impl.buildCallback();
+        return impl.layoutCallback();
+      }).then(() => {
+        const img = impl.img_;
+        expect(img.getAttribute('sizes')).to.be.null;
+      });
+    });
+
+    it('should generate correct sizes for layout fixed', () => {
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      const impl = getStubbedImg({
+        layout: Layout.FIXED,
+        src: 'test.jpg',
+        srcset: 'large.jpg 2000w, small.jpg 1000w',
+        width: 300,
+        height: 200,
+      }, 300);
+      impl.buildCallback();
+      impl.initialize_();
+      const img = impl.img_;
+      expect(impl.getViewport().getWidth()).to.equal(320);
+      expect(img.getAttribute('sizes')).to
+          .equal('(max-width: 320px) 300px, 300px');
+    });
+
+    it('should generate correct sizes for layout responsive', () => {
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      const impl = getStubbedImg({
+        layout: Layout.RESPONSIVE,
+        src: 'test.jpg',
+        srcset: 'large.jpg 2000w, small.jpg 1000w',
+        width: 300,
+        height: 200,
+      }, 160);
+      impl.buildCallback();
+      impl.initialize_();
+      const img = impl.img_;
+      expect(impl.getViewport().getWidth()).to.equal(320);
+      expect(img.getAttribute('sizes')).to
+          .equal('(max-width: 320px) 160px, 100vw');
+    });
+
+    it('should generate correct sizes for layout fixed-height', () => {
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      const impl = getStubbedImg({
+        layout: Layout.FIXED_HEIGHT,
+        src: 'test.jpg',
+        srcset: 'large.jpg 2000w, small.jpg 1000w',
+        width: 300,
+        height: 200,
+      }, 160);
+      impl.buildCallback();
+      impl.initialize_();
+      const img = impl.img_;
+      expect(impl.getViewport().getWidth()).to.equal(320);
+      expect(img.getAttribute('sizes')).to
+          .equal('(max-width: 320px) 160px, 100vw');
+    });
+
+    it('should generate correct sizes for layout fill', () => {
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      const impl = getStubbedImg({
+        layout: Layout.FILL,
+        src: 'test.jpg',
+        srcset: 'large.jpg 2000w, small.jpg 1000w',
+        width: 300,
+        height: 200,
+      }, 160);
+      impl.buildCallback();
+      impl.initialize_();
+      const img = impl.img_;
+      expect(impl.getViewport().getWidth()).to.equal(320);
+      expect(img.getAttribute('sizes')).to
+          .equal('(max-width: 320px) 160px, 100vw');
+    });
+
+    it('should generate correct sizes for layout flex-item', () => {
+      expect(isExperimentOn(window, 'amp-img-auto-sizes')).to.be.true;
+      const impl = getStubbedImg({
+        layout: Layout.FLEX_ITEM,
+        src: 'test.jpg',
+        srcset: 'large.jpg 2000w, small.jpg 1000w',
+        width: 300,
+        height: 200,
+      }, 160);
+      impl.buildCallback();
+      impl.initialize_();
+      const img = impl.img_;
+      expect(impl.getViewport().getWidth()).to.equal(320);
+      expect(img.getAttribute('sizes')).to
+          .equal('(max-width: 320px) 160px, 100vw');
+    });
+
+  });
 });
diff --git a/test/unit/test-custom-element.js b/test/unit/test-custom-element.js
@@ -1042,6 +1042,7 @@ describes.realWin('CustomElement', {amp: true}, env => {
         matchMedia.withArgs('(min-width: 1111111px)').returns(
             {matches: false});
         element2 = new ElementClass();
+        element2.ampdoc_ = env.ampdoc;
       });
 
       it('should apply media condition', () => {

diff --git a/tools/experiments/experiments.js b/tools/experiments/experiments.js
@@ -407,6 +407,12 @@ const EXPERIMENTS = [
     spec: 'https://github.com/ampproject/amphtml/issues/20964',
     cleanupIssue: 'https://github.com/ampproject/amphtml/issues/20965',
   },
+  {
+    id: 'amp-img-auto-sizes',
+    name: 'Automatically generates sizes for amp-img if not given',
+    spec: 'https://github.com/ampproject/amphtml/issues/19513',
+    cleanupIssue: 'https://github.com/ampproject/amphtml/issues/20517',
+  },
   {
     id: 'amp-autocomplete',
     name: 'AMP Autocomplete provides a set of suggestions to' +