Remove native srcset experiment and update fallback logic

builtins/amp-img.js

@@ -15,20 +15,17 @@
  */
 
 import {BaseElement} from '../src/base-element';
-import {isExperimentOn} from '../src/experiments';
+import {dev} from '../src/log';
 import {isLayoutSizeDefined} from '../src/layout';
+import {listen} from '../src/event-helper';
 import {registerElement} from '../src/service/custom-element-registry';
-import {srcsetFromElement, srcsetFromSrc} from '../src/srcset';
 
 /**
  * Attributes to propagate to internal image when changed externally.
  * @type {!Array<string>}
  */
 const ATTRIBUTES_TO_PROPAGATE = ['alt', 'title', 'referrerpolicy', 'aria-label',
-  'aria-describedby', 'aria-labelledby'];
-
-const EXPERIMENTAL_ATTRIBUTES_TO_PROPAGATE = ATTRIBUTES_TO_PROPAGATE
-    .concat(['srcset', 'src', 'sizes']);
+  'aria-describedby', 'aria-labelledby','srcset', 'src', 'sizes'];
 
 export class AmpImg extends BaseElement {
 
@@ -45,46 +42,21 @@ export class AmpImg extends BaseElement {
     /** @private {?Element} */
     this.img_ = null;
 
-    /** @private {?../src/srcset.Srcset} */
-    this.srcset_ = null;
+    /** @private {?UnlistenDef} */
+    this.unlistenLoad_ = null;
 
-    /** @private @const {boolean} */
-    this.useNativeSrcset_ = isExperimentOn(this.win, 'amp-img-native-srcset');
+    /** @private {?UnlistenDef} */
+    this.unlistenError_ = null;
   }
 
   /** @override */
   mutatedAttributesCallback(mutations) {
-    let mutated = false;
-    if (!this.useNativeSrcset_) {
-      if (mutations['srcset'] !== undefined) {
-        // `srcset` mutations take precedence over `src` mutations.
-        this.srcset_ = srcsetFromElement(this.element);
-        mutated = true;
-      } else if (mutations['src'] !== undefined) {
-        // If only `src` is mutated, then ignore the existing `srcset` attribute
-        // value (may be set automatically as cache optimization).
-        this.srcset_ = srcsetFromSrc(this.element.getAttribute('src'));
-        mutated = true;
-      }
-      // This element may not have been laid out yet.
-      if (mutated && this.img_) {
-        this.updateImageSrc_();
-      }
-    }
-
     if (this.img_) {
-      const propAttrs = this.useNativeSrcset_ ?
-        EXPERIMENTAL_ATTRIBUTES_TO_PROPAGATE :
-        ATTRIBUTES_TO_PROPAGATE;
-      const attrs = propAttrs.filter(
+      const attrs = ATTRIBUTES_TO_PROPAGATE.filter(
           value => mutations[value] !== undefined);
       this.propagateAttributes(
           attrs, this.img_, /* opt_removeMissingAttrs */ true);
-
-      if (this.useNativeSrcset_) {
-        this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
-      }
-
+      this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
     }
   }
 
@@ -102,7 +74,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);
@@ -128,9 +100,6 @@ export class AmpImg extends BaseElement {
     if (this.img_) {
       return;
     }
-    if (!this.useNativeSrcset_ && !this.srcset_) {
-      this.srcset_ = srcsetFromElement(this.element);
-    }
     // If this amp-img IS the fallback then don't allow it to have its own
     // fallback to stop from nested fallback abuse.
     this.allowImgLoadFallback_ = !this.element.hasAttribute('fallback');
@@ -156,17 +125,9 @@ export class AmpImg extends BaseElement {
         'be correctly propagated for the underlying <img> element.');
     }
 
-
-    if (this.useNativeSrcset_) {
-      this.propagateAttributes(EXPERIMENTAL_ATTRIBUTES_TO_PROPAGATE,
-          this.img_);
-      this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
-    } else {
-      this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
-    }
-
+    this.propagateAttributes(ATTRIBUTES_TO_PROPAGATE, this.img_);
+    this.guaranteeSrcForSrcsetUnsupportedBrowsers_();
     this.applyFillContent(this.img_, true);
-
     this.element.appendChild(this.img_);
   }
 
@@ -175,11 +136,6 @@ export class AmpImg extends BaseElement {
     return this.isPrerenderAllowed_;
   }
 
-  /** @override */
-  isRelayoutNeeded() {
-    return true;
-  }
-
   /** @override */
   reconstructWhenReparented() {
     return false;
@@ -188,18 +144,26 @@ export class AmpImg extends BaseElement {
   /** @override */
   layoutCallback() {
     this.initialize_();
-    let promise = this.updateImageSrc_();
+    const img = dev().assertElement(this.img_);
+    this.unlistenLoad_ = listen(img, 'load', () => this.hideFallbackImg_());
+    this.unlistenError_ = listen(img, 'error', () => this.onImgLoadingError_());
+    if (this.getLayoutWidth() <= 0) {
+      return Promise.resolve();
+    }
+    return this.loadPromise(img);
+  }
 
-    // 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;
+  /** @override */
+  unlayoutCallback() {
+    if (this.unlistenError_) {
+      this.unlistenError_();
+      this.unlistenError_ = null;
     }
-    return promise;
+    if (this.unlistenLoad_) {
+      this.unlistenLoad_();
+      this.unlistenLoad_ = null;
+    }
+    return true;
   }
 
   /**
@@ -221,51 +185,33 @@ export class AmpImg extends BaseElement {
   }
 
   /**
-   * @return {!Promise}
    * @private
    */
-  updateImageSrc_() {
-    if (this.getLayoutWidth() <= 0) {
-      return Promise.resolve();
-    }
-
-    if (!this.useNativeSrcset_) {
-      const src = this.srcset_.select(
-          // The width should never be 0, but we fall back to the screen width
-          // just in case.
-          this.getViewport().getWidth() || this.win.screen.width,
-          this.getDpr());
-      if (src == this.img_.getAttribute('src')) {
-        return Promise.resolve();
-      }
-
-      this.img_.setAttribute('src', src);
+  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;
+    }
   }
 }