Remove native srcset experiment and update fallback logic #16404

cathyxz · 2018-06-26T23:20:36Z

This PR deletes the experiment specific code for amp-img-native-srcset so that we're running native srcset permanently instead of doing our own srcset logic (closes #11575). The experiment / prod/canary-config.json files now have to be edited as a separate PR per new build rules. Turns out that it broke the fallback image logic (which was dependent on checking whether the src should change on resize), so I refactored updateImageSrc into two listeners for on load and error. Annotations inline.

Testing:

In Summary

We don’t show image fallbacks after first load due to performance reasons, per feature(amp-img): add amp-img fallback #2575.
Since we no longer manually set the src, whether or not we show a fallback, solely depends on whether the image loaded correctly (i.e. whether loadPromise(this.img_) rejects), no more checking for whether selected src equals this.img_.src.
There is a problem with solely using loadPromise for this check. Namely, that there can be a race condition between the resize which calls layoutCallback and tries to check for error / load in order to toggle the fallback, and the load / error event itself. This causes problematic behavior on Firefox/Safari/Chrome where they don't show the fallback image.
To solve this problem, I refactored the loadPromise then/catch block into two listeners for load and error on the underlying <img>.

Not Addressed in this PR

Illogical behavior of fallbacks / placeholders per amp-img placeholder and fallback changes behavior after viewport rotation #10321
Possibly revisiting the behavior of not toggling fallback images after first load. When resizing browsers, it seems like better UX to bring back the fallback image instead of showing a blank. But then again, I'm not sure what the performance concerns are, as well as how often people really play around with resizing browsers (or re-orienting their phones for that matter). Nonetheless, this is a bit of an edge case and will probably depend on actual usage.

Further details on the different behaviors of srcset per browser at: go/amp-img-srcset

cathyxz · 2018-06-26T23:22:53Z

test/functional/test-amp-img.js

@@ -317,7 +279,7 @@ describe('amp-img', () => {
      });
    });

-    it('should fallback only once', () => {
+    it('should fallback once and remove fallback once image loads', () => {


I combined this with the next test because the behavior here changed. Since we no longer set src based on the result of parseSrcset, the only thing that determines whether an image should show or hide its fallback is whether or not the image loaded, i.e. the result of loadPromise.

cathyxz · 2018-06-27T20:57:57Z

builtins/amp-img.js

@@ -188,18 +139,13 @@ export class AmpImg extends BaseElement {
  /** @override */
  layoutCallback() {
    this.initialize_();
-    let promise = this.updateImageSrc_();


This logic block was refactored into the load and error listeners on the right.

cathyxz · 2018-06-27T20:58:45Z

builtins/amp-img.js

-      }
-
-      this.img_.setAttribute('src', src);
+  hideFallbackImg_() {


This logic is directly taken from the then block of then original updateImageSrc and should be unchanged.

cathyxz · 2018-06-27T21:00:38Z

builtins/amp-img.js

-      // Would show the placeholder underneath a transparent fallback
-      this.togglePlaceholder(false);
-    });
+    if (this.allowImgLoadFallback_) {


Moved the if condition here to encompass the listener in a single function.

Update tests

… of loadPromise

codecov-io · 2018-06-29T05:04:38Z

Codecov Report

Merging #16404 into master will increase coverage by 1.03%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master   #16404      +/-   ##
==========================================
+ Coverage   77.15%   78.18%   +1.03%     
==========================================
  Files         547      552       +5     
  Lines       39934    40344     +410     
==========================================
+ Hits        30812    31544     +732     
+ Misses       9122     8800     -322

Flag	Coverage Δ
#integration_tests	`34.96% <30%> (?)`
#unit_tests	`77.25% <93.33%> (?)`

Impacted Files	Coverage Δ
builtins/amp-img.js	`78.08% <93.33%> (+9.95%)`	⬆️
extensions/amp-auto-ads/0.1/amp-auto-ads.js	`82.85% <0%> (-2.86%)`	⬇️
...xtensions/amp-story/1.0/amp-story-store-service.js	`83.67% <0%> (-1.44%)`	⬇️
src/base-element.js	`91.17% <0%> (-0.99%)`	⬇️
extensions/amp-iframe/0.1/amp-iframe.js	`86.99% <0%> (-0.98%)`	⬇️
extensions/amp-story/0.1/related-articles.js	`6.25% <0%> (-0.9%)`	⬇️
3p/facebook.js	`51.42% <0%> (-0.88%)`	⬇️
src/log.js	`97.67% <0%> (-0.56%)`	⬇️
src/service/video/autoplay.js	`58.26% <0%> (-0.36%)`	⬇️
...eclick-impl/0.1/amp-ad-network-doubleclick-impl.js	`88.97% <0%> (-0.2%)`	⬇️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8056021...b11bbf6. Read the comment docs.

jridgewell

This will need docs changes. /cc @bpaduch

Can we remove all the other SrcSet uses yet?

cathyxz · 2018-06-29T18:04:18Z

Based on the history on the original issue and the <amp-img> documentation, it seems like the main issue was that our documentation said we were "same as srcset attribute on the img tag", but our implementation did not match the documentation. I've clarified that we no longer have a polyfill (IE is the only browser that does not support native srcset at this point) and changed the wording from "AMP runtime" to "browser". The other main references to srcset that I can search for on our documentation website seem to be accurate descriptions of the browser spec (e.g. https://www.ampproject.org/docs/design/responsive/art_direction#srcset).

@bpaduch are there other references to amp-img srcset that we need to update on our documentation? TLDR the change here is to remove our polyfill and propagate srcset on amp-img, so that srcset on <amp-img> will follow native browser behavior.

cathyxz · 2018-06-29T18:14:19Z

I think <amp-anim> also has a srcset polyfill. I can address that as a separate PR--the change should be very similar to what we did with <amp-img> here already.

jridgewell

I'm satisfied with the code.

/to @bpaduch to look over the docs.

aghassemi · 2018-07-02T20:30:42Z

test/manual/amp-img-fallback.amp.html

+</head>
+<body>
+  <h1>amp-image</h1>
+  <!-- <h2>Unsupported formats</h2>


cathyxz · 2018-07-09T17:21:51Z

ping @bpaduch can we merge?

jridgewell · 2018-07-09T22:23:25Z

@cathyxz: Go ahead, we can review docs later.

cathyxz · 2018-07-09T22:28:14Z

Filed #16625 to track docs update. Waiting for Travis (there have been a couple of flakes on this PR today).

cathyxz added the PR use: Work in Progress label Jun 26, 2018

googlebot added the cla: yes label Jun 26, 2018

cathyxz commented Jun 26, 2018

View reviewed changes

cathyxz force-pushed the feature/remove-srcset-experiment branch 4 times, most recently from 806282e to 83f18dd Compare June 27, 2018 02:39

cathyxz commented Jun 27, 2018

View reviewed changes

cathyxz changed the title ~~[WIP] Remove native srcset experiment~~ Remove native srcset experiment and update fallback logic Jun 27, 2018

cathyxz added 5 commits June 28, 2018 11:33

Remove experiment from amp-img

f237d07

Update tests per srcset experiment removal

3205cd9

Update tests

Update use of updateImgSrc

525b6b0

Add a image fallback test and optimize images

0a419da

Update tests

1bfaa98

cathyxz force-pushed the feature/remove-srcset-experiment branch from 83f18dd to 67fdf73 Compare June 28, 2018 18:38

cathyxz added 2 commits June 28, 2018 21:52

Refactor the toggleFallback logic to load and error listeners instead…

fc7f879

… of loadPromise

Fix unit tests per refactor

76420b3

cathyxz force-pushed the feature/remove-srcset-experiment branch from f0b81d1 to 76420b3 Compare June 29, 2018 04:53

cathyxz requested review from jridgewell, erwinmombay and aghassemi June 29, 2018 05:23

jridgewell requested changes Jun 29, 2018

View reviewed changes

cathyxz removed the PR use: Work in Progress label Jun 29, 2018

Update documentation for native srcset

749a3f9

jridgewell approved these changes Jun 29, 2018

View reviewed changes

cathyxz requested a review from a user July 2, 2018 18:23

aghassemi approved these changes Jul 2, 2018

View reviewed changes

test/manual/amp-img-fallback.amp.html Outdated

</head>

<body>

<h1>amp-image</h1>

<!-- <h2>Unsupported formats</h2>

Copy link

Contributor

aghassemi Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uncomment

aghassemi mentioned this pull request Jul 7, 2018

amp-img fallback breaks on resize if srcset is present #16588

Closed

Uncomment manual tests

b11bbf6

This was referenced Jul 9, 2018

Verify and clarify docs around removal of amp-img srcset polyfill #16625

Closed

amp-carousel / amp-img fallback failure on Safari #16615

Closed

cathyxz merged commit 7d34e9e into ampproject:master Jul 9, 2018

cathyxz deleted the feature/remove-srcset-experiment branch July 9, 2018 23:39

This was referenced Aug 7, 2019

amp-bind: Binding to amp-img[src] also requires [srcset] on cache #10207

Closed

amp-img: Remove [srcset] when [src] is mutated #23779

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove native srcset experiment and update fallback logic #16404

Remove native srcset experiment and update fallback logic #16404

cathyxz commented Jun 26, 2018 •

edited

Loading

cathyxz Jun 26, 2018

cathyxz Jun 27, 2018

cathyxz Jun 27, 2018

cathyxz Jun 27, 2018

codecov-io commented Jun 29, 2018 •

edited

Loading

jridgewell left a comment

cathyxz commented Jun 29, 2018 •

edited

Loading

cathyxz commented Jun 29, 2018

jridgewell left a comment

aghassemi Jul 2, 2018

cathyxz commented Jul 9, 2018

jridgewell commented Jul 9, 2018

cathyxz commented Jul 9, 2018

Remove native srcset experiment and update fallback logic #16404

Remove native srcset experiment and update fallback logic #16404

Conversation

cathyxz commented Jun 26, 2018 • edited Loading

In Summary

Not Addressed in this PR

cathyxz Jun 26, 2018

Choose a reason for hiding this comment

cathyxz Jun 27, 2018

Choose a reason for hiding this comment

cathyxz Jun 27, 2018

Choose a reason for hiding this comment

cathyxz Jun 27, 2018

Choose a reason for hiding this comment

codecov-io commented Jun 29, 2018 • edited Loading

Codecov Report

jridgewell left a comment

Choose a reason for hiding this comment

cathyxz commented Jun 29, 2018 • edited Loading

cathyxz commented Jun 29, 2018

jridgewell left a comment

Choose a reason for hiding this comment

aghassemi Jul 2, 2018

Choose a reason for hiding this comment

cathyxz commented Jul 9, 2018

jridgewell commented Jul 9, 2018

cathyxz commented Jul 9, 2018

cathyxz commented Jun 26, 2018 •

edited

Loading

codecov-io commented Jun 29, 2018 •

edited

Loading

cathyxz commented Jun 29, 2018 •

edited

Loading