Refactor the toggleFallback logic to load and error listeners instead…

… of loadPromise
ampproject · Jun 28, 2018 · 67fdf73 · 67fdf73
1 parent 1bfaa98
commit 67fdf73
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 51 deletions.
diff --git a/builtins/amp-img.js b/builtins/amp-img.js
@@ -15,7 +15,9 @@
  */
 
 import {BaseElement} from '../src/base-element';
+import {dev} from '../src/log';
 import {isLayoutSizeDefined} from '../src/layout';
+import {listen} from '../src/event-helper';
 import {registerElement} from '../src/service/custom-element-registry';
 
 /**
@@ -50,10 +52,6 @@ export class AmpImg extends BaseElement {
       this.propagateAttributes(
           attrs, this.img_, /* opt_removeMissingAttrs */ true);
       this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
-
-      if (mutations['src'] || mutations['srcset']) {
-        this.removeFallbackIfImageLoaded_();
-      }
     }
   }
 
@@ -71,7 +69,7 @@ export class AmpImg extends BaseElement {
         return;
       }
       // We try to find the first url in the srcset
-      const srcseturl = /https?:\/\/\S+/.exec(srcset);
+      const srcseturl = /\S+/.exec(srcset);
       // Connect to the first url if it exists
       if (srcseturl) {
         this.preconnect.url(srcseturl[0], onLayout);
@@ -133,11 +131,6 @@ export class AmpImg extends BaseElement {
     return this.isPrerenderAllowed_;
   }
 
-  /** @override */
-  isRelayoutNeeded() {
-    return true;
-  }
-
   /** @override */
   reconstructWhenReparented() {
     return false;
@@ -146,18 +139,13 @@ export class AmpImg extends BaseElement {
   /** @override */
   layoutCallback() {
     this.initialize_();
-    let promise = this.removeFallbackIfImageLoaded_();
-
-    // We only allow to fallback on error on the initial layoutCallback
-    // or else this would be pretty expensive.
-    if (this.allowImgLoadFallback_) {
-      promise = promise.catch(e => {
-        this.onImgLoadingError_();
-        throw e;
-      });
-      this.allowImgLoadFallback_ = false;
+    const img = dev().assertElement(this.img_);
+    listen(img, 'load', () => this.hideFallbackImg_());
+    listen(img, 'error', () => this.onImgLoadingError_());
+    if (this.getLayoutWidth() <= 0) {
+      return Promise.resolve();
     }
-    return promise;
+    return this.loadPromise(img);
   }
 
   /**
@@ -179,38 +167,33 @@ export class AmpImg extends BaseElement {
   }
 
   /**
-   * @return {!Promise}
    * @private
    */
-  removeFallbackIfImageLoaded_() {
-    if (this.getLayoutWidth() <= 0) {
-      return Promise.resolve();
+  hideFallbackImg_() {
+    if (!this.allowImgLoadFallback_
+      && this.img_.classList.contains('i-amphtml-ghost')) {
+      this.getVsync().mutate(() => {
+        this.img_.classList.remove('i-amphtml-ghost');
+        this.toggleFallback(false);
+      });
     }
-
-    return this.loadPromise(this.img_).then(() => {
-      // Clean up the fallback if the src has changed.
-      if (!this.allowImgLoadFallback_ &&
-          this.img_.classList.contains('i-amphtml-ghost')) {
-        this.getVsync().mutate(() => {
-          this.img_.classList.remove('i-amphtml-ghost');
-          this.toggleFallback(false);
-        });
-      }
-    });
   }
 
   /**
-   * If the image fails to load, show a placeholder instead.
+   * If the image fails to load, show a fallback or placeholder instead.
    * @private
    */
   onImgLoadingError_() {
-    this.getVsync().mutate(() => {
-      this.img_.classList.add('i-amphtml-ghost');
-      this.toggleFallback(true);
-      // Hide placeholders, as browsers that don't support webp
-      // Would show the placeholder underneath a transparent fallback
-      this.togglePlaceholder(false);
-    });
+    if (this.allowImgLoadFallback_) {
+      this.getVsync().mutate(() => {
+        this.img_.classList.add('i-amphtml-ghost');
+        this.toggleFallback(true);
+        // Hide placeholders, as browsers that don't support webp
+        // Would show the placeholder underneath a transparent fallback
+        this.togglePlaceholder(false);
+      });
+      this.allowImgLoadFallback_ = false;
+    }
   }
 }
 

diff --git a/test/functional/test-amp-img.js b/test/functional/test-amp-img.js
@@ -124,21 +124,20 @@ describe('amp-img', () => {
     windowWidth = 320;
     screenWidth = 4000;
     return getImg({
-      srcset: 'bad.jpg 2000w, /examples/img/sample.jpg 1000w',
+      srcset: SRCSET_STRING,
       width: 300,
       height: 200,
     }).then(ampImg => {
       const img = ampImg.querySelector('img');
       expect(img.tagName).to.equal('IMG');
-      expect(img.getAttribute('srcset'))
-          .to.equal('bad.jpg 2000w, /examples/img/sample.jpg 1000w');
+      expect(img.getAttribute('srcset')).to.equal(SRCSET_STRING);
       expect(img.hasAttribute('referrerpolicy')).to.be.false;
     });
   });
 
   it('should preconnect to the the first srcset url if src is not set', () => {
     return getImg({
-      srcset: 'http://google.com/bad.jpg 2000w, /examples/img/sample.jpg 1000w',
+      srcset: SRCSET_STRING,
       width: 300,
       height: 200,
     }).then(ampImg => {
@@ -147,7 +146,7 @@ describe('amp-img', () => {
       impl.preconnectCallback(true);
       expect(impl.preconnect.url.called).to.be.true;
       expect(impl.preconnect.url).to.have.been.calledWith(
-          'http://google.com/bad.jpg'
+          '/examples/img/hero@1x.jpg'
       );
     });
   });

diff --git a/test/manual/amp-img-fallback.amp.html b/test/manual/amp-img-fallback.amp.html
@@ -22,7 +22,7 @@
 </head>
 <body>
   <h1>amp-image</h1>
-  <h2>Unsupported formats</h2>
+  <!-- <h2>Unsupported formats</h2>
   <h3>Smaller image is webp</h3>
   <amp-img srcset="/examples/img/hero@1x.webp 1282w,
   /examples/img/hero@2x.jpg 1923w" width=400 height=300 layout=responsive noloading
@@ -37,7 +37,7 @@ <h3>Larger image is webp</h3>
       sizes="(max-width: 320px) 320px, 100vw">
     <div placeholder>Placeholder</div>
     <amp-img fallback src="/examples/img/sample.jpg" width=400 height=300 layout=responsive noloading></amp-img>
-  </amp-img>
+  </amp-img> -->
 
   <h2>404</h2>
   <h3>Smaller image is 404</h3>