Allows amp-img.js to check if it has a blurred placeholder child. #17132

addieachan · 2018-07-26T18:49:59Z

Addresses issue Improve blurred background performance by using CSS painters #15146
Uses an experimental flag

addieachan · 2018-07-26T18:54:10Z

I know there are some lint errors that I will try and ask @rsimha about. Just wanted to get out the basic implementation

dreamofabear · 2018-07-26T19:04:11Z

builtins/amp-img.js

@@ -47,6 +48,12 @@ export class AmpImg extends BaseElement {

    /** @private {?UnlistenDef} */
    this.unlistenError_ = null;
+
+    /** @private @const {boolean} */
+    this.useBlurryPlaceholder_ = isExperimentOn(this.win, 'blurry-placeholder');


Ivar not needed, just inline this call below.

dreamofabear · 2018-07-26T19:05:58Z

builtins/amp-img.js

+    const placeholder = this.getPlaceholder();
+    if (placeholder) {
+      const classes = placeholder.getAttribute('class').split(' ');
+      if (classes.find(function(el) {


Use classList.contains() instead.

dreamofabear · 2018-07-26T19:07:23Z

builtins/amp-img.js

+      if (classes.find(function(el) {
+        return el == 'i-amphtml-blur';
+      })) {
+        this.hasBlurredPlaceHolder_ = true;


Let's add the code that actually uses this boolean. This PR is still quite small.

So just to clarify, that would mean add the code to trigger the loading fade-in animation when the image is loaded, right?Or is it something else?

addieachan · 2018-07-31T15:46:07Z

builtins/amp-img.js

@@ -16,6 +16,8 @@

 import {BaseElement} from '../src/base-element';
 import {dev} from '../src/log';
+import {isExperimentOn} from '../src/experiments';
+import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../src/utils/img';


This and other calls to guaranteeSrcForSrcsetUnsupportedBrowsers were not implemented by me, but are in the current version of amp-img.js. It's just a two lines, but idk how important it is to ensure the changes that show up are only yours, so just wanted to point it out if you had advice on fixing it. Otherwise, I will try and solution as well!

Edit: I think it was resolved thanks to @choumx 's help!

addieachan · 2018-08-01T20:50:09Z

test/functional/test-amp-img.js

+      impl.buildCallback();
+      impl.layoutCallback();
+      expect(impl.element.classList.contains(
+          'i-amphtml-blur-loading')).to.be.true;


Sorry if these expect statements are hard to read -I had to break them up to satisfy the lint rule of being <80 chars on a line. If there is a better place for me to break it/an alternate solution, then please let me know!

addieachan · 2018-08-01T20:58:11Z

@choumx, @gmajoulet , @alanorozco I have the amp-img side of things functional for the image blur, and I feel like this PR is at a stage for a preliminary look so PTAL!

dreamofabear · 2018-08-02T17:58:59Z

builtins/amp-img.js

@@ -26,7 +27,7 @@ import {registerElement} from '../src/service/custom-element-registry';
 * @type {!Array<string>}
 */
 const ATTRIBUTES_TO_PROPAGATE = ['alt', 'title', 'referrerpolicy', 'aria-label',
-  'aria-describedby', 'aria-labelledby','srcset', 'src', 'sizes'];
+  'aria-describedby', 'aria-labelledby','srcset', 'src', 'sizes', 'blur'];


Revert this change.

dreamofabear · 2018-08-02T17:59:37Z

builtins/amp-img.js

@@ -146,7 +156,7 @@ export class AmpImg extends BaseElement {
  layoutCallback() {
    this.initialize_();
    const img = dev().assertElement(this.img_);
-    this.unlistenLoad_ = listen(img, 'load', () => this.hideFallbackImg_());
+    this.unlistenLoad_ = listen(img, 'load', () => this.renderImg_());


Nit: onImgLoad_

dreamofabear · 2018-08-02T18:00:39Z

builtins/amp-img.js

@@ -167,6 +177,31 @@ export class AmpImg extends BaseElement {
    return true;
  }

+  /**
+   * Checks to see if there is a placeholder that is blurred


Be more specific. "If a blurry placeholder exists, hides the child until it's fully loaded."

dreamofabear · 2018-08-02T18:00:52Z

builtins/amp-img.js

+  }
+
+  /**
+   * @private


Nit: Move annotations like @private below the comment.

dreamofabear · 2018-08-02T18:42:59Z

css/amp.css

@@ -392,7 +410,7 @@ i-amphtml-sizer {
  display: block;
 }

-.i-amphtml-element > [placeholder].hidden,
+.i-amphtml-element:not(.i-amphtml-blur-loaded, .i-amphtml-blur-loading) > [placeholder].hidden,


Add a comment here explaining this.

dreamofabear · 2018-08-02T18:50:14Z

test/functional/test-amp-img.js

+      impl.unlayoutCallback();
+    });
+
+    function getImgWithBlur(addPlacholder, addBlurClass) {


Typo: addPlaceholder

dreamofabear · 2018-08-02T18:50:53Z

builtins/amp-img.js

+   */
+  renderImg_() {
+    if (this.hasBlurredPlaceHolder_) {
+      this.element.classList.add('i-amphtml-blur-loaded');


We should remove i-amphtml-blur-loading right?

Oh yeah! I think I thought that the blur would disappear before the fade in would complete, but I realize that wasn't the case. Thanks!

dreamofabear · 2018-08-02T18:51:19Z

builtins/amp-img.js

+   * placeholder if one exists, or hiding the fallback if not
+   */
+  renderImg_() {
+    if (this.hasBlurredPlaceHolder_) {


You won't need this ivar if you just check this.classList.contains('i-amphtml-blur-loading').

dreamofabear · 2018-08-02T18:54:37Z

test/functional/test-amp-img.js

+      const loadEvent = createCustomEvent(iframe.win, 'load');
+      impl.img_.dispatchEvent(loadEvent);
+      expect(impl.element.classList.contains(
+          'i-amphtml-blur-loaded')).to.be.true;


Try using expect(el.classList).includes('i-amphtml-blur-loaded'). This will yield a better error message on failure -- currently it'll look like "expected false to be true".

That makes sense! As a tangentially related question, is there a stylistic preference between using 'el.classList' vs 'impl.element.classList'?

We already have a reference to the element so just use it. :)

ok cool!

Also btw, when I tried the 'expect(el.classList).includes('i-amphtml-blur-loaded')', the test failed because it said the object being tested was a domtokenlist which wasn't supported by the "includes()" call. Instead, I just did 'expect(el).to.have.class('i-amphtml-blur-loaded')' which seems to give the right error. Please let me know if this is a problem!

Even better!

dreamofabear · 2018-08-02T18:55:05Z

test/functional/test-amp-img.js

+      impl = new AmpImg(el);
+      impl.buildCallback();
+      impl.layoutCallback();
+      expect(impl.hasBlurredPlaceHolder_).to.be.false;


Check CSS classes instead of ivars.

addieachan · 2018-08-06T03:34:27Z

@choumx, @alanorozco, and @gmajoulet PTAL as I talked to Will on Friday, and over the weekend, I consolidated the blur implementation since we noticed that it was actually being rendered on top of the image so it had to be faded out. I also had to remove one test (that was testing the different classes given to the amp-img regarding hiding/unhiding of the image as it loads since it is now unnecessary to hide it). If there are any new tests that need to be added, then please let me know :)

dreamofabear · 2018-08-06T19:04:29Z

css/amp.css

@@ -780,4 +791,4 @@ amp-story .i-amphtml-loader {
 */
 [amp-fx^="fly-in"] {
  visibility: hidden;
-}
+}


Restore trailing newline.

dreamofabear · 2018-08-06T19:04:41Z

css/amp.css

+  transition: opacity 1s 1s !important;
+}
+
+.i-amphtml-blur{


Nit: Space before {.

dreamofabear · 2018-08-06T19:04:49Z

css/amp.css

@@ -23,7 +23,7 @@
 * to be computed to `auto` on both the `body` and `html` elements so they both
 * potentially get a scrolling box. See #3108 for more details.
 */
-html {
+ html {


dreamofabear · 2018-08-06T19:11:49Z

css/amp.css

@@ -167,6 +167,16 @@ html.i-amphtml-singledoc.i-amphtml-ios-embed-sd > body {
  display: inline-block;
 }

+.i-amphtml-fade-out .i-amphtml-blur {


i-amphtml-layout class is added after layoutCallback(), so can we avoid a custom CSS class altogether and do instead:

.i-amphtml-layout > .i-amphtml-blur { opacity: 0 !important; } .i-amphtml-blur { transition: ...; filter: ...; transform: ...; }

dreamofabear · 2018-08-06T19:11:58Z

css/amp.css

@@ -167,6 +167,16 @@ html.i-amphtml-singledoc.i-amphtml-ios-embed-sd > body {
  display: inline-block;
 }

+.i-amphtml-fade-out .i-amphtml-blur {
+  opacity: 0 !important;
+  transition: opacity 1s 1s !important;


Why have a transition-delay of 1s? Also, let's use ease timing function.

transition: opacity 1s ease !important;

Yeah I did it because I thought it looked better on fast connections where the image looked like it flashed since the placeholder wasn't seen. But the 'ease' function solves that problem so thanks!

I'm not aware of other AMP animation curves, but the material guidelines would recommend a "decelerating curve" here, either ease-out or the actual material easing: cubic-bezier(0.0, 0.0, 0.2, 1).

dreamofabear · 2018-08-06T19:15:02Z

builtins/amp-img.js

+    const placeholder = this.getPlaceholder();
+    if (isExperimentOn(this.win, 'blurry-placeholder') && placeholder && 
+      placeholder.classList.contains('i-amphtml-blur')) {
+      this.element.classList.add('i-amphtml-fade-out');


With my comment below, we actually might not need any changes here.

So when I take out the overriding of fistLayoutCompleted(), the blur doesn't show up since the togglePlaceholder(false) is hiding it quickly not allowing for the fade out. Is it ok to keep the overriding just to make sure images with blurred placeholders don't call togglePlaceholder(false)?

alanorozco · 2018-08-06T19:37:13Z

builtins/amp-img.js

+  firstLayoutCompleted() {
+    const placeholder = this.getPlaceholder();
+    if (isExperimentOn(this.win, 'blurry-placeholder') && placeholder && 
+      placeholder.classList.contains('i-amphtml-blur')) {


I'm a little confused, how is i-amphtml-blur set?

The devtool/cache generates it.

gmajoulet · 2018-08-06T21:22:08Z

builtins/amp-img.js

+  /** @override */
+  firstLayoutCompleted() {
+    const placeholder = this.getPlaceholder();
+    if (isExperimentOn(this.win, 'blurry-placeholder') && placeholder && 


Tiny improvement: isExperimentOn could read the cookies, which is an operation that's a bit more expensive than checking a class. Maybe it'd be worth moving the isExperimentOn() check last?

gmajoulet · 2018-08-06T21:36:21Z

css/amp.css

@@ -167,6 +167,16 @@ html.i-amphtml-singledoc.i-amphtml-ios-embed-sd > body {
  display: inline-block;
 }

+.i-amphtml-fade-out .i-amphtml-blur {
+  opacity: 0 !important;
+  transition: opacity 1s 1s !important;


I'm not aware of other AMP animation curves, but the material guidelines would recommend a "decelerating curve" here, either ease-out or the actual material easing: cubic-bezier(0.0, 0.0, 0.2, 1).

dreamofabear · 2018-08-07T14:37:30Z

builtins/amp-img.js

+  firstLayoutCompleted() {
+    const placeholder = this.getPlaceholder();
+    if (!(placeholder && placeholder.classList.contains('i-amphtml-blur') &&
+      isExperimentOn(this.win, 'blurry-placeholder'))) {


Nit: Flip the conditional to avoid the need for !.

dreamofabear · 2018-08-07T14:37:47Z

css/amp.css

@@ -167,6 +167,13 @@ html.i-amphtml-singledoc.i-amphtml-ios-embed-sd > body {
  display: inline-block;
 }

+
+.i-amphtml-blur{


Nit: Space before {.

dreamofabear · 2018-08-07T14:43:30Z

test/functional/test-amp-img.js

+     * @param {boolean} addBlurClass Whether the child should have the
+     *     class that allows it to be a blurred placeholder.
+     */
+    function getImgWithBlur(addPlaceholder, addBlurClass) {


This test code would be easier to follow if this helper was idempotent. I.e. change it to return the AmpImg instead of modifying existing variables.

it('my test', () => { const impl = getImgWithBlur(...); const el = impl.element; });

dreamofabear · 2018-08-07T14:45:22Z

test/functional/test-amp-img.js

+      }
+      el.appendChild(img);
+      el.getResources = () => Services.resourcesForDoc(document);
+      el.getPlaceholder = sandbox.stub();


Shouldn't this return img if addPlaceholder is true? I.e. () => img.

dreamofabear · 2018-08-07T14:48:19Z

test/functional/test-amp-img.js

+      impl.layoutWidth_ = 200;
+    }
+
+    it('should correctly detect if a blurry image child exists', () => {


This test name is out of date. How about "should set blurry placeholder opacity to 0 on image load"?

dreamofabear · 2018-08-07T14:48:37Z

test/functional/test-amp-img.js

+      impl.buildCallback();
+      impl.layoutCallback();
+      impl.firstLayoutCompleted();
+      expect(img.style.opacity).to.be.equal('');


Let's also verify that togglePlaceholder(false) continues to be called in the default case, here and below.

dreamofabear · 2018-08-07T14:51:04Z

test/functional/test-amp-img.js

+      expect(img.style.opacity).to.be.equal('');
+      const loadEvent = createCustomEvent(iframe.win, 'load');
+      impl.img_.dispatchEvent(loadEvent);
+      impl.firstLayoutCompleted();


Dispatching the event is no longer necessary. You can probably just remove this test.

addieachan · 2018-08-07T20:10:04Z

PTAL whenever you guys are free!

dreamofabear · 2018-08-07T20:14:10Z

css/amp.css

  filter: blur(20px) !important;
  transform: scale(1.1) !important;
-  transition: opacity 1s ease !important;
+  transition: opacity 1s cubic-bezier(0.0, 0.0, 0.2, 1) !important;


Why cubic-bezier and not ease?

I actually asked for it in the previous review, that's the material deceleration curve.
However, I believe the animation should not last more than 300ms

dreamofabear · 2018-08-07T20:14:44Z

test/functional/test-amp-img.js

      if (addPlaceholder) {
        img.setAttribute('placeholder', '');
+        el.getPlaceholder = () => {return img;};


Minor: You can use closure shorthand here. e.getPlaceholder = () => img;

dreamofabear · 2018-08-07T20:15:54Z

test/functional/test-amp-img.js

      impl.layoutWidth_ = 200;
+
+      impl.togglePlaceholder = sandbox.stub();
+      toggleExperiment(impl.win, 'blurry-placeholder', true, true);


You only need to call this once in beforeEach. impl.win == window, so just do:

toggleExperiment(window, 'blurry-placeholder', true, true);

dreamofabear · 2018-08-07T20:16:44Z

test/functional/test-amp-img.js

      impl.buildCallback();
      impl.layoutCallback();
      impl.firstLayoutCompleted();
+      let el = impl.element;
+      let img = el.firstChild;
      expect(img.style.opacity).to.equal('0');



Add an expectation that togglePlaceholder was not called here.

dreamofabear · 2018-08-07T20:17:05Z

test/functional/test-amp-img.js

+      el = impl.element;
+      img = el.firstChild;
+      expect(impl.togglePlaceholder).to.have.been.calledWith(false);
+      impl.unlayoutCallback();


Why call unlayoutCallback here?

dreamofabear · 2018-08-07T20:18:11Z

test/functional/test-amp-img.js

+      el.appendChild(img);
+      el.getResources = () => Services.resourcesForDoc(document);
+      const impl = new AmpImg(el);
+      impl.layoutWidth_ = 200;


Stub getLayoutWidth() instead.

sandbox.stub(impl, 'getLayoutWidth').returns(200);

addieachan · 2018-08-07T22:34:14Z

css/amp.css

+.i-amphtml-blur {
+  filter: blur(20px) !important;
+  transform: scale(1.1) !important;
+  transition: opacity 0.3s cubic-bezier(0.0, 0.0, 0.2, 1) !important;


We decided to use this cubic-bezier as it is what is suggested for easing elements in per the material design doc:
https://material.io/design/motion/speed.html#easing

addieachan · 2018-08-08T13:06:12Z

If one of you with merge access can help me merge this, that would be greatly appreciated! (Assuming everything looks ready - otherwise PTAL again)

dreamofabear · 2018-08-08T15:48:59Z

test/functional/test-amp-img.js

+      el = impl.element;
+      img = el.firstChild;
+      expect(impl.togglePlaceholder).to.have.been.calledWith(false);
+      impl.unlayoutCallback();


Missed comment from earlier: Why is unlayoutCallback called here?

Oh sorry! I just saw it in the aftereach of other tests, so I included it in my aftereach method, and then I took out the afterEach because I only had one test method after deleting the other two, so I included the code here. I'll take it out though!

…pproject#17132) * Redid blur detection changes with updated amp-img file * Fixed stylistic issues * Lint errors * Got the animation of the fading in of the rendered image * Fixed wrong variable name * Fix small changes * fixed lint errors * Added tests * Fixed lint errors * Re-add accidentally deleted line * Fixed some of the requested changes. Animation changes are a WIP * Consolidated changes but need to reconfigure tests * Took out comments and debugging code * Rewrote tests to reflect implementation change * Consolidated changes further and redid tests * Fixed small style changes * Fixed tests and small fixes * Fixed javadoc * Fixed style things * Changed bundle size * Took out unnecessary unlayoutCallback() call

Redid blur detection changes with updated amp-img file

fa1c72c

googlebot added the cla: yes label Jul 26, 2018

dreamofabear requested changes Jul 26, 2018

View reviewed changes

rsimha mentioned this pull request Jul 26, 2018

🏗🐛 Make the installation of custom lint rules more robust #17135

Merged

addieachan added 2 commits July 26, 2018 16:35

Fixed stylistic issues

3abd2c6

Lint errors

ebf23f1

dreamofabear mentioned this pull request Jul 26, 2018

Detects if an amp-img has a blurred image placeholder #17111

Closed

addieachan added 2 commits July 31, 2018 11:25

Got the animation of the fading in of the rendered image

3bfe881

Fixed wrong variable name

48232d4

addieachan force-pushed the detectBlur2 branch from cacce62 to 48232d4 Compare July 31, 2018 15:43

addieachan commented Jul 31, 2018

View reviewed changes

Fix small changes

2318049

addieachan force-pushed the detectBlur2 branch from c705de1 to 2318049 Compare July 31, 2018 17:41

fixed lint errors

7be8afe

addieachan force-pushed the detectBlur2 branch from dcfb61d to 7be8afe Compare July 31, 2018 18:36

addieachan added 3 commits July 31, 2018 14:58

Merge remote-tracking branch 'upstream/master' into detectBlur2

a05e7ae

Added tests

656092e

Fixed lint errors

bc115d8

addieachan commented Aug 1, 2018

View reviewed changes

Re-add accidentally deleted line

1ff9439

dreamofabear reviewed Aug 2, 2018

View reviewed changes

addieachan added 4 commits August 2, 2018 16:24

Fixed some of the requested changes. Animation changes are a WIP

0622712

Consolidated changes but need to reconfigure tests

1c7a256

Took out comments and debugging code

0e5727a

Rewrote tests to reflect implementation change

0c4ca3e

dreamofabear reviewed Aug 6, 2018

View reviewed changes

alanorozco reviewed Aug 6, 2018

View reviewed changes

gmajoulet reviewed Aug 6, 2018

View reviewed changes

addieachan added 2 commits August 6, 2018 22:06

Consolidated changes further and redid tests

a1c5bdf

Fixed small style changes

c2adc5a

dreamofabear reviewed Aug 7, 2018

View reviewed changes

addieachan added 2 commits August 7, 2018 13:58

Fixed tests and small fixes

b6f4ea1

Fixed javadoc

81e8724

dreamofabear approved these changes Aug 7, 2018

View reviewed changes

Fixed style things

423db40

addieachan commented Aug 7, 2018

View reviewed changes

addieachan added 2 commits August 7, 2018 22:12

Changed bundle size

0fb088d

Merge branch 'master' into detectBlur2

ae4f688

dreamofabear reviewed Aug 8, 2018

View reviewed changes

addieachan added 3 commits August 8, 2018 13:17

Took out unnecessary unlayoutCallback() call

0f0a2bd

Merge branch 'master' into detectBlur2

01c392a

Merge branch 'master' into detectBlur2

8705fea

dreamofabear merged commit 57058c7 into ampproject:master Aug 13, 2018

This was referenced Sep 5, 2018

Add blurry image placeholder fade out to amp videos #17873

Closed

✨ Adds placeholder transition from blurred to amp-video posters #17881

Merged

Allows amp-img.js to check if it has a blurred placeholder child. #17132

Allows amp-img.js to check if it has a blurred placeholder child. #17132

Conversation

addieachan commented Jul 26, 2018

addieachan commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addieachan Jul 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addieachan commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addieachan commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addieachan commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addieachan commented Aug 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addieachan Jul 31, 2018 •

edited

addieachan commented Aug 8, 2018 •

edited