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.
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
Allows amp-img.js to check if it has a blurred placeholder child. #17132
Changes from 19 commits
fa1c72c
3abd2c6
ebf23f1
3bfe881
48232d4
2318049
7be8afe
a05e7ae
656092e
bc115d8
1ff9439
0622712
1c7a256
0e5727a
0c4ca3e
a1c5bdf
c2adc5a
b6f4ea1
81e8724
423db40
0fb088d
ae4f688
0f0a2bd
01c392a
8705fea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 1s cubic-bezier(0.0, 0.0, 0.2, 1) !important; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually asked for it in the previous review, that's the material deceleration curve. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
|
@@ -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 */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
|
@@ -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(); | ||
|
@@ -374,4 +377,75 @@ describe('amp-img', () => { | |
impl.unlayoutCallback(); | ||
}); | ||
|
||
describe('blurred image placeholder', () => { | ||
|
||
/** | ||
* 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 = () => {return img;}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: You can use closure shorthand here. |
||
} 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); | ||
impl.layoutWidth_ = 200; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stub
|
||
|
||
impl.togglePlaceholder = sandbox.stub(); | ||
toggleExperiment(impl.win, 'blurry-placeholder', true, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You only need to call this once in toggleExperiment(window, 'blurry-placeholder', true, true); |
||
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'); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an expectation that togglePlaceholder was not called here. |
||
|
||
impl = getImgWithBlur(true, false); | ||
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, 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dispatching the event is no longer necessary. You can probably just remove this test. |
||
el = impl.element; | ||
img = el.firstChild; | ||
expect(impl.togglePlaceholder).to.have.been.calledWith(false); | ||
impl.unlayoutCallback(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed comment from earlier: Why is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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!