Skip to content

Commit

Permalink
🐛Do not perform static layout for cloned nodes with static layout. (a…
Browse files Browse the repository at this point in the history
  • Loading branch information
Sepand Parhami authored Feb 14, 2019
1 parent a751cc7 commit 80f9fa9
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 10 deletions.
4 changes: 2 additions & 2 deletions extensions/amp-a4a/0.1/test/test-template-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ describes.realWin('TemplateRenderer', realWinConfig, env => {
'\n <amp-analytics class="i-amphtml-element i-amphtml' +
'-notbuilt amp-notbuilt i-amphtml-layout-fixed i-amphtml' +
'-layout-size-defined amp-unresolved i-amphtml-' +
'unresolved" style="width: 1px; height: 1px;">' +
'</amp-analytics></div>');
'unresolved" i-amphtml-layout="fixed" style="width: 1px;' +
' height: 1px;"></amp-analytics></div>');
});
});
});
Expand Down
10 changes: 7 additions & 3 deletions src/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,10 @@ export function isLoadingAllowed(element) {
* @return {!Layout}
*/
export function applyStaticLayout(element) {
// Check if the layout has already been done by server-side rendering. The
// document may be visible to the user if the boilerplate was removed so
// please take care in making changes here.
// Check if the layout has already been done by server-side rendering or
// client-side rendering and the element was cloned. The document may be
// visible to the user if the boilerplate was removed so please take care in
// making changes here.
const completedLayoutAttr = element.getAttribute('i-amphtml-layout');
if (completedLayoutAttr) {
const layout = /** @type {!Layout} */ (devAssert(
Expand Down Expand Up @@ -490,5 +491,8 @@ export function applyStaticLayout(element) {
}
setStyle(element, 'height', 0);
}
// Mark the element as having completed static layout, in case it is cloned
// in the future.
element.setAttribute('i-amphtml-layout', layout);
return layout;
}
29 changes: 24 additions & 5 deletions test/unit/test-layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -462,31 +462,39 @@ describe('Layout', () => {
expect(pixel.style.width).to.equal('');
});

it('should fail invalid width and height', () => {
it('should layout with pixel values', () => {
const pixel = document.createElement('amp-pixel');

// Everything is good.
pixel.setAttribute('width', '1px');
pixel.setAttribute('height', '1px');
expect(() => {
applyStaticLayout(pixel);
}).to.not.throw();
});

// Width=auto is also correct.
it('should layout with valid with auto width value', () => {
const pixel = document.createElement('amp-pixel');
pixel.setAttribute('width', 'auto');
pixel.setAttribute('height', '1px');
expect(() => {
applyStaticLayout(pixel);
}).to.not.throw();
});

it('should fail invalid width', () => {
const pixel = document.createElement('amp-pixel');
// Width=X is invalid.
pixel.setAttribute('width', 'X');
pixel.setAttribute('height', '1px');
allowConsoleError(() => { expect(() => {
applyStaticLayout(pixel);
}).to.throw(/Invalid width value/); });
});

it('should fail invalid height', () => {
const pixel = document.createElement('amp-pixel');
// Height=X is invalid.
pixel.setAttribute('height', 'X');
pixel.setAttribute('width', '1px');
pixel.setAttribute('height', 'X');
allowConsoleError(() => { expect(() => {
applyStaticLayout(pixel);
}).to.throw(/Invalid height value/); });
Expand Down Expand Up @@ -537,4 +545,15 @@ describe('Layout', () => {
expect(() => applyStaticLayout(div)).to.throw(/failed/);
});
});

it('should not re-layout cloned content', () => {
div.setAttribute('layout', 'responsive');
div.setAttribute('width', 100);
div.setAttribute('height', 200);
expect(applyStaticLayout(div)).to.equal(Layout.RESPONSIVE);
expect(div.querySelectorAll('i-amphtml-sizer')).to.have.length(1);
const clone = div.cloneNode(true);
expect(applyStaticLayout(clone)).to.equal(Layout.RESPONSIVE);
expect(clone.querySelectorAll('i-amphtml-sizer')).to.have.length(1);
});
});

0 comments on commit 80f9fa9

Please sign in to comment.