Skip to content

Commit

Permalink
🚀 Early exit for SSR AMP Image (#33890)
Browse files Browse the repository at this point in the history 
* Early exit for SSR AMP Image to avoid late LCP for attaching the same node again during hydration
* Tests now pass
* Left over debugger statement commented out was removed
* Changes from comments
  • Loading branch information
@kristoferbaxter
kristoferbaxter authored Apr 19, 2021
1 parent ca05d28 commit 7f7ca15
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 125 deletions.
10 changes: 6 additions & 4 deletions builtins/amp-img.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,9 @@ export class AmpImg extends BaseElement {
// fallback to stop from nested fallback abuse.
this.allowImgLoadFallback_ = !this.element.hasAttribute('fallback');

// For inabox SSR, image will have been written directly to DOM so no need
// to recreate. Calling appendChild again will have no effect.
if (this.element.hasAttribute('i-amphtml-ssr')) {
// For SSR, image will have been written directly to DOM so no need to recreate.
const serverRendered = this.element.hasAttribute('i-amphtml-ssr');
if (serverRendered) {
this.img_ = scopedQuerySelector(this.element, '> img:not([placeholder])');
}
this.img_ = this.img_ || new Image();
Expand Down Expand Up @@ -224,7 +224,9 @@ export class AmpImg extends BaseElement {
this.applyFillContent(this.img_, true);
propagateObjectFitStyles(this.element, this.img_);

this.element.appendChild(this.img_);
if (!serverRendered) {
this.element.appendChild(this.img_);
}
return this.img_;
}

Expand Down
185 changes: 76 additions & 109 deletions test/unit/test-amp-img-intrinsic.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,115 +63,89 @@ describes.sandboxed('amp-img layout intrinsic', {}, () => {
browser = new BrowserController(fixture.win);
});
it('should not exceed given width and height even if image\
natural size is larger', () => {
let ampImg;
return getImg({
natural size is larger', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg', // 641 x 481
width: 100,
height: 100,
layout: 'intrinsic',
})
.then((image) => {
ampImg = image;
return browser.waitForElementLayout('amp-img');
})
.then(() => {
expect(ampImg.getBoundingClientRect()).to.include({
width: 100,
height: 100,
});
const img = ampImg.querySelector('img');
expect(img.getBoundingClientRect()).to.include({
width: 100,
height: 100,
});
});
});
await browser.waitForElementLayout('amp-img');
expect(ampImg.getBoundingClientRect()).to.include({
width: 100,
height: 100,
});
const img = ampImg.querySelector('img');
expect(img.getBoundingClientRect()).to.include({
width: 100,
height: 100,
});
});

it('should reach given width and height even if image\
natural size is smaller', () => {
let ampImg;
return getImg({
natural size is smaller', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg', // 641 x 481
width: 800,
height: 600,
layout: 'intrinsic',
})
.then((image) => {
ampImg = image;
return browser.waitForElementLayout('amp-img');
})
.then(() => {
expect(ampImg.getBoundingClientRect()).to.include({
width: 800,
height: 600,
});
const img = ampImg.querySelector('img');
expect(img.getBoundingClientRect()).to.include({
width: 800,
height: 600,
});
});
});
await browser.waitForElementLayout('amp-img');
expect(ampImg.getBoundingClientRect()).to.include({
width: 800,
height: 600,
});
const img = ampImg.querySelector('img');
expect(img.getBoundingClientRect()).to.include({
width: 800,
height: 600,
});
});

it('expands a parent div with no explicit dimensions', () => {
let ampImg;
it('expands a parent div with no explicit dimensions', async () => {
const parentDiv = fixture.doc.getElementById('parent');
// inline-block to force width and height to size of children
// font-size 0 to get rid of the 4px added by inline-block for whitespace
parentDiv.setAttribute('style', 'display: inline-block; font-size: 0;');
return getImg({
const ampImg = await getImg({
src: '/examples/img/sample.jpg', // 641 x 481
width: 600,
height: 400,
layout: 'intrinsic',
})
.then((image) => {
ampImg = image;
return browser.waitForElementLayout('amp-img');
})
.then(() => {
expect(ampImg.getBoundingClientRect()).to.include({
width: 600,
height: 400,
});
const parentDiv = fixture.doc.getElementById('parent');
expect(parentDiv.getBoundingClientRect()).to.include({
width: 600,
height: 400,
});
});
});
await browser.waitForElementLayout('amp-img');
expect(ampImg.getBoundingClientRect()).to.include({
width: 600,
height: 400,
});
expect(parentDiv.getBoundingClientRect()).to.include({
width: 600,
height: 400,
});
});

it('is bounded by explicit dimensions of a parent container', () => {
let ampImg;
it('is bounded by explicit dimensions of a parent container', async () => {
const parentDiv = fixture.doc.getElementById('parent');
parentDiv.setAttribute('style', 'width: 80px; height: 80px');
return getImg({

const ampImg = await getImg({
src: '/examples/img/sample.jpg', // 641 x 481
width: 800,
height: 600,
layout: 'intrinsic',
})
.then((image) => {
ampImg = image;
return browser.waitForElementLayout('amp-img');
})
.then(() => {
expect(ampImg.getBoundingClientRect()).to.include({
width: 80,
height: 60,
});
const parentDiv = fixture.doc.getElementById('parent');
expect(parentDiv.getBoundingClientRect()).to.include({
width: 80,
height: 80,
});
});
});
await browser.waitForElementLayout('amp-img');
expect(ampImg.getBoundingClientRect()).to.include({
width: 80,
height: 60,
});
expect(parentDiv.getBoundingClientRect()).to.include({
width: 80,
height: 80,
});
});

it('SSR sizer does not interfere with img creation', () => {
let ampImg;
it('SSR sizer does not interfere with img creation', async () => {
const parentDiv = fixture.doc.getElementById('parent');
parentDiv.setAttribute('style', 'width: 80px; height: 80px');

Expand All @@ -182,6 +156,13 @@ describes.sandboxed('amp-img layout intrinsic', {}, () => {
height: 600,
layout: 'intrinsic',
});
const serverRenderedImg = createElementWithAttributes(
fixture.doc,
'img',
{
src: '/examples/img/sample.jpg',
}
);
applyStaticLayout(tmp);
const attributes = {
'i-amphtml-ssr': '',
Expand All @@ -190,19 +171,16 @@ describes.sandboxed('amp-img layout intrinsic', {}, () => {
attributes[tmp.attributes[i].name] = tmp.attributes[i].value;
}

return getImg(attributes, toArray(tmp.children))
.then((image) => {
ampImg = image;
return browser.waitForElementLayout('amp-img');
})
.then(() => {
expect(ampImg.querySelector('img[src*="sample.jpg"]')).to.exist;
expect(ampImg.querySelector('img[src*="image/svg+xml"]')).to.exist;
});
const ampImg = await getImg(
attributes,
[serverRenderedImg].concat(toArray(tmp.children))
);
await browser.waitForElementLayout('amp-img');
expect(ampImg.querySelector('img[src*="sample.jpg"]')).to.exist;
expect(ampImg.querySelector('img[src*="image/svg+xml"]')).to.exist;
});

it('SSR sizer does not interfere with SSR img before', () => {
let ampImg;
it('SSR sizer does not interfere with SSR img before', async () => {
const parentDiv = fixture.doc.getElementById('parent');
parentDiv.setAttribute('style', 'width: 80px; height: 80px');

Expand Down Expand Up @@ -230,19 +208,13 @@ describes.sandboxed('amp-img layout intrinsic', {}, () => {
})
);

return getImg(attributes, children)
.then((image) => {
ampImg = image;
return browser.waitForElementLayout('amp-img');
})
.then(() => {
expect(ampImg.querySelector('img[src*="sample.jpg"]')).to.exist;
expect(ampImg.querySelector('img[src*="image/svg+xml"]')).to.exist;
});
const ampImg = await getImg(attributes, children);
await browser.waitForElementLayout('amp-img');
expect(ampImg.querySelector('img[src*="sample.jpg"]')).to.exist;
expect(ampImg.querySelector('img[src*="image/svg+xml"]')).to.exist;
});

it('SSR sizer does not interfere with SSR img after', () => {
let ampImg;
it('SSR sizer does not interfere with SSR img after', async () => {
const parentDiv = fixture.doc.getElementById('parent');
parentDiv.setAttribute('style', 'width: 80px; height: 80px');

Expand Down Expand Up @@ -270,15 +242,10 @@ describes.sandboxed('amp-img layout intrinsic', {}, () => {
})
);

return getImg(attributes, children)
.then((image) => {
ampImg = image;
return browser.waitForElementLayout('amp-img');
})
.then(() => {
expect(ampImg.querySelector('img[src*="sample.jpg"]')).to.exist;
expect(ampImg.querySelector('img[src*="image/svg+xml"]')).to.exist;
});
const ampImg = await getImg(attributes, children);
await browser.waitForElementLayout('amp-img');
expect(ampImg.querySelector('img[src*="sample.jpg"]')).to.exist;
expect(ampImg.querySelector('img[src*="image/svg+xml"]')).to.exist;
});
});
});
14 changes: 12 additions & 2 deletions test/unit/test-amp-img-v1.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -440,9 +440,14 @@ describes.realWin('amp-img V1', {amp: true}, (env) => {
* placeholder attribute.
* @param {boolean} addBlurClass Whether the child should have the
* class that allows it to be a blurred placeholder.
* @param {boolean} serverRendered If the image is server rendered.
* @return {AmpImg} An amp-img object potentially with a blurry placeholder
*/
function getImgWithBlur(addPlaceholder, addBlurClass) {
function getImgWithBlur(
addPlaceholder,
addBlurClass,
serverRendered = false
) {
const el = createImg({
src: '/examples/img/sample.jpg',
id: 'img1',
Expand All @@ -462,6 +467,12 @@ describes.realWin('amp-img V1', {amp: true}, (env) => {
img.classList.add('i-amphtml-blurry-placeholder');
}
el.appendChild(img);
if (serverRendered) {
const serverRenderedImg = doc.createElement('img');
serverRenderedImg.setAttribute('src', '/examples/img/sample.jpg');
el.appendChild(serverRenderedImg);
el.setAttribute('i-amphtml-ssr', '');
}
doc.body.appendChild(el);
return el;
}
Expand Down Expand Up @@ -506,7 +517,6 @@ describes.realWin('amp-img V1', {amp: true}, (env) => {

it('does not interfere with SSR img creation', async () => {
const ampImg = getImgWithBlur(true, true);
ampImg.setAttribute('i-amphtml-ssr', '');
await ampImg.build();

expect(ampImg.querySelector('img[src*="sample.jpg"]')).to.exist;
Expand Down
36 changes: 26 additions & 10 deletions test/unit/test-amp-img.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,9 +510,14 @@ describes.sandboxed('amp-img', {}, (env) => {
* placeholder attribute.
* @param {boolean} addBlurClass Whether the child should have the
* class that allows it to be a blurred placeholder.
* @param {boolean} serverRendered If the image is server rendered.
* @return {AmpImg} An amp-img object potentially with a blurry placeholder
*/
function getImgWithBlur(addPlaceholder, addBlurClass) {
function getImgWithBlur(
addPlaceholder,
addBlurClass,
serverRendered = false
) {
const el = document.createElement('amp-img');
const img = document.createElement('img');
el.setAttribute('src', '/examples/img/sample.jpg');
Expand All @@ -529,6 +534,12 @@ describes.sandboxed('amp-img', {}, (env) => {
el.getLayoutSize = () => ({width: 200, height: 100});
el.appendChild(img);
el.getResources = () => Services.resourcesForDoc(document);
if (serverRendered) {
const serverRenderedImg = document.createElement('img');
serverRenderedImg.setAttribute('src', '/examples/img/sample.jpg');
el.appendChild(serverRenderedImg);
el.setAttribute('i-amphtml-ssr', '');
}
const impl = new AmpImg(el);
impl.togglePlaceholder = sandbox.stub();
return impl;
Expand Down Expand Up @@ -572,9 +583,8 @@ describes.sandboxed('amp-img', {}, (env) => {
});

it('does not interfere with SSR img creation', () => {
const impl = getImgWithBlur(true, true);
const impl = getImgWithBlur(true, true, true);
const ampImg = impl.element;
ampImg.setAttribute('i-amphtml-ssr', '');
impl.buildCallback();
impl.layoutCallback();

Expand Down Expand Up @@ -660,13 +670,19 @@ describes.sandboxed('amp-img', {}, (env) => {
});

it('should not generate sizes for amp-imgs when rendered from the server', async () => {
const ampImg = await getImg({
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
width: 300,
height: 200,
'i-amphtml-ssr': '',
});
const serverRenderedImg = document.createElement('img');
serverRenderedImg.setAttribute('src', '/examples/img/sample.jpg');
serverRenderedImg.setAttribute('srcset', SRCSET_STRING);
const ampImg = await getImg(
{
src: '/examples/img/sample.jpg',
srcset: SRCSET_STRING,
width: 300,
height: 200,
'i-amphtml-ssr': '',
},
[serverRenderedImg]
);
const impl = await ampImg.getImpl(false);
impl.buildCallback();
await impl.layoutCallback();
Expand Down

0 comments on commit 7f7ca15

Please sign in to comment.