Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 25 commits into from Aug 13, 2018
Merged

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

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
fa1c72c
Redid blur detection changes with updated amp-img file
addieachan Jul 26, 2018
3abd2c6
Fixed stylistic issues
addieachan Jul 26, 2018
ebf23f1
Lint errors
addieachan Jul 26, 2018
3bfe881
Got the animation of the fading in of the rendered image
addieachan Jul 31, 2018
48232d4
Fixed wrong variable name
addieachan Jul 31, 2018
2318049
Fix small changes
addieachan Jul 31, 2018
7be8afe
fixed lint errors
addieachan Jul 31, 2018
a05e7ae
Merge remote-tracking branch 'upstream/master' into detectBlur2
addieachan Jul 31, 2018
656092e
Added tests
addieachan Aug 1, 2018
bc115d8
Fixed lint errors
addieachan Aug 1, 2018
1ff9439
Re-add accidentally deleted line
addieachan Aug 1, 2018
0622712
Fixed some of the requested changes. Animation changes are a WIP
addieachan Aug 2, 2018
1c7a256
Consolidated changes but need to reconfigure tests
addieachan Aug 4, 2018
0e5727a
Took out comments and debugging code
addieachan Aug 4, 2018
0c4ca3e
Rewrote tests to reflect implementation change
addieachan Aug 6, 2018
a1c5bdf
Consolidated changes further and redid tests
addieachan Aug 7, 2018
c2adc5a
Fixed small style changes
addieachan Aug 7, 2018
b6f4ea1
Fixed tests and small fixes
addieachan Aug 7, 2018
81e8724
Fixed javadoc
addieachan Aug 7, 2018
423db40
Fixed style things
addieachan Aug 7, 2018
0fb088d
Changed bundle size
addieachan Aug 8, 2018
ae4f688
Merge branch 'master' into detectBlur2
addieachan Aug 8, 2018
0f0a2bd
Took out unnecessary unlayoutCallback() call
addieachan Aug 8, 2018
01c392a
Merge branch 'master' into detectBlur2
addieachan Aug 10, 2018
8705fea
Merge branch 'master' into detectBlur2
addieachan Aug 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions builtins/amp-img.js
View file
Open in desktop
Expand Up @@ -17,9 +17,11 @@
import {BaseElement} from '../src/base-element';
import {dev} from '../src/log';
import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../src/utils/img';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

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';

/**
* Attributes to propagate to internal image when changed externally.
Expand Down Expand Up @@ -167,6 +169,17 @@ export class AmpImg extends BaseElement {
return true;
}

/** @override **/
firstLayoutCompleted() {
const placeholder = this.getPlaceholder();
if (placeholder && placeholder.classList.contains('i-amphtml-blur') &&
isExperimentOn(this.win, 'blurry-placeholder')) {
setImportantStyles(placeholder, {'opacity': 0});
} else {
this.togglePlaceholder(false);
}
}

/**
* @private
*/
Expand Down
10 changes: 9 additions & 1 deletion css/amp.css
View file
Open in desktop
Expand Up @@ -167,6 +167,13 @@ html.i-amphtml-singledoc.i-amphtml-ios-embed-sd > body {
display: inline-block;
}


.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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
Copy link

Choose a reason for hiding this comment

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

Do we need to animate both the placeholder and the image instead of just the latter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know I talked to you in person about this, but the reason I think it used the animation on both before was because since the blur is a child, it was rendered on top of the parent so the blur would appear even on top of the loaded image. So the animation I could see was actually the fade out of the blur. I did adjust this and was able to remove the placeholder animation by setting the z-index of .i-amphtml-blur to -1. Is this an ok solution or could introducing the z-index value cause other problems?


.i-amphtml-layout-nodisplay,
[layout=nodisplay]:not(.i-amphtml-layout-nodisplay)
{
Expand Down Expand Up @@ -252,7 +259,8 @@ i-amphtml-sizer {
display: block !important;
}

.i-amphtml-fill-content {
.i-amphtml-fill-content,
.i-amphtml-blur {
display: block;
/* These lines are a work around to this issue in iOS: */
/* https://bugs.webkit.org/show_bug.cgi?id=155198 */
Expand Down
75 changes: 75 additions & 0 deletions test/functional/test-amp-img.js
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ import {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';

describe('amp-img', () => {
let sandbox;
Expand Down Expand Up @@ -207,6 +208,7 @@ describe('amp-img', () => {
el.setAttribute('width', 100);
el.setAttribute('height', 100);
el.getResources = () => Services.resourcesForDoc(document);
el.getPlaceholder = sandbox.stub();
impl = new AmpImg(el);
impl.createdCallback();
sandbox.stub(impl, 'getLayoutWidth').returns(100);
Expand Down Expand Up @@ -364,6 +366,7 @@ describe('amp-img', () => {
el.setAttribute('aria-labelledby', 'id2');
el.setAttribute('aria-describedby', 'id3');

el.getPlaceholder = sandbox.stub();
const impl = new AmpImg(el);
impl.buildCallback();
impl.layoutCallback();
Expand All @@ -374,4 +377,76 @@ describe('amp-img', () => {
impl.unlayoutCallback();
});

describe('blurred image placeholder', () => {
beforeEach(() => {
toggleExperiment(window, 'blurry-placeholder', true, true);
});

/**
* Creates an amp-img with an image child that could potentially be a
* blurry placeholder.
* @param {boolean} addPlaceholder Whether the child should have a
* placeholder attribute.
* @param {boolean} addBlurClass Whether the child should have the
* class that allows it to be a blurred placeholder.
* @return {AmpImg} An amp-img object potentially with a blurry placeholder
*/
function getImgWithBlur(addPlaceholder, addBlurClass) {
Copy link

Choose a reason for hiding this comment

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

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;
});

const el = document.createElement('amp-img');
const img = document.createElement('img');
if (addPlaceholder) {
img.setAttribute('placeholder', '');
el.getPlaceholder = () => img;
} else {
el.getPlaceholder = sandbox.stub();
}
if (addBlurClass) {
img.classList.add('i-amphtml-blur');
}
el.appendChild(img);
el.getResources = () => Services.resourcesForDoc(document);
const impl = new AmpImg(el);
sandbox.stub(impl, 'getLayoutWidth').returns(200);
impl.togglePlaceholder = sandbox.stub();
return impl;
}

it('should set placeholder opacity to 0 on image load', () => {
let impl = getImgWithBlur(true, true);
impl.buildCallback();
impl.layoutCallback();
impl.firstLayoutCompleted();
let el = impl.element;
let img = el.firstChild;
expect(img.style.opacity).to.equal('0');
expect(impl.togglePlaceholder).to.not.be.called;

impl = getImgWithBlur(true, false);
impl.buildCallback();
impl.layoutCallback();
impl.firstLayoutCompleted();
el = impl.element;
img = el.firstChild;
expect(img.style.opacity).to.be.equal('');
Copy link

Choose a reason for hiding this comment

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

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

expect(impl.togglePlaceholder).to.have.been.calledWith(false);

impl = getImgWithBlur(false, true);
impl.buildCallback();
impl.layoutCallback();
impl.firstLayoutCompleted();
el = impl.element;
img = el.firstChild;
expect(img.style.opacity).to.be.equal('');
expect(impl.togglePlaceholder).to.have.been.calledWith(false);

impl = getImgWithBlur(false, false);
impl.buildCallback();
impl.layoutCallback();
impl.firstLayoutCompleted();
el = impl.element;
img = el.firstChild;
expect(impl.togglePlaceholder).to.have.been.calledWith(false);
impl.unlayoutCallback();
Copy link

Choose a reason for hiding this comment

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

Why call unlayoutCallback here?

Copy link

Choose a reason for hiding this comment

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

Missed comment from earlier: Why is unlayoutCallback called here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

});
});
});