From d09a31c80dea16bec8973e97f384e5b17eccc7a6 Mon Sep 17 00:00:00 2001 From: Peter Josling Date: Mon, 4 Dec 2017 21:21:46 +0000 Subject: [PATCH] Calculate correct documentHeight for short AMP docs (#12229) * Use contentHeight for documentHeight messages Handles the edge case of AMP documents which are shorter than their containing viewport, which causes documentHeight to be scrollHeight, which is the height of the viewport. Should be a no-op for docs which are already taller than the viewport. Re-implements changes made in https://github.com/ampproject/amphtml/commit/ac741f6d448305570659d0eae3c11c9049ae0fef, which were reverted before merging. * Use HTML5 doctype for RealWinFixture Non-HTML5 doctype has different behaviour for html/body element heights in Safari. --- src/inabox/inabox-viewport.js | 1 + src/service/resources-impl.js | 18 ++++++------- src/service/viewport/viewport-binding-def.js | 9 +++++++ .../viewport-binding-ios-embed-wrapper.js | 12 +++++++++ .../viewport/viewport-binding-natural.js | 12 +++++++++ src/service/viewport/viewport-impl.js | 13 +++++++++ test/functional/test-resources.js | 26 +++++++++--------- test/functional/test-viewport-binding.js | 27 ++++++++++++++++++- test/functional/test-viewport.js | 6 +++++ testing/describes.js | 2 +- 10 files changed, 102 insertions(+), 24 deletions(-) diff --git a/src/inabox/inabox-viewport.js b/src/inabox/inabox-viewport.js index c62560bf50b2..937aa5273213 100644 --- a/src/inabox/inabox-viewport.js +++ b/src/inabox/inabox-viewport.js @@ -359,6 +359,7 @@ export class ViewportBindingInabox { /** @override */ setScrollTop() {/* no-op */} /** @override */ getScrollWidth() {return 0;} /** @override */ getScrollHeight() {return 0;} + /** @override */ getContentHeight() {return 0;} /** @override */ getBorderTop() {return 0;} /** @override */ requiresFixedLayerTransfer() {return false;} } diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index f61225f79364..bc0d4c67b5e4 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -197,7 +197,7 @@ export class Resources { this.vsyncScheduled_ = false; /** @private {number} */ - this.scrollHeight_ = 0; + this.contentHeight_ = 0; /** @private {boolean} */ this.maybeChangeHeight_ = false; @@ -1035,10 +1035,10 @@ export class Resources { 'linkRels': Services.documentInfoForDoc(this.ampdoc).linkRels, }), /* cancelUnsent */true); - this.scrollHeight_ = this.viewport_.getScrollHeight(); + this.contentHeight_ = this.viewport_.getContentHeight(); this.viewer_.sendMessage('documentHeight', - dict({'height': this.scrollHeight_}), /* cancelUnsent */true); - dev().fine(TAG_, 'document height on load: ' + this.scrollHeight_); + dict({'height': this.contentHeight_}), /* cancelUnsent */true); + dev().fine(TAG_, 'document height on load: ' + this.contentHeight_); } const viewportSize = this.viewport_.getSize(); @@ -1065,12 +1065,12 @@ export class Resources { if (this.maybeChangeHeight_) { this.maybeChangeHeight_ = false; this.vsync_.measure(() => { - const measuredScrollHeight = this.viewport_.getScrollHeight(); - if (measuredScrollHeight != this.scrollHeight_) { + const measuredContentHeight = this.viewport_.getContentHeight(); + if (measuredContentHeight != this.contentHeight_) { this.viewer_.sendMessage('documentHeight', - dict({'height': measuredScrollHeight}), /* cancelUnsent */true); - this.scrollHeight_ = measuredScrollHeight; - dev().fine(TAG_, 'document height changed: ' + this.scrollHeight_); + dict({'height': measuredContentHeight}), /* cancelUnsent */true); + this.contentHeight_ = measuredContentHeight; + dev().fine(TAG_, 'document height changed: ' + this.contentHeight_); } }); } diff --git a/src/service/viewport/viewport-binding-def.js b/src/service/viewport/viewport-binding-def.js index 305f4f4ed10e..150f1724f49d 100644 --- a/src/service/viewport/viewport-binding-def.js +++ b/src/service/viewport/viewport-binding-def.js @@ -141,6 +141,15 @@ export class ViewportBindingDef { */ getScrollHeight() {} + /** + * Returns the height of the content of the document, including the + * padding top for the viewer header. + * contentHeight will match scrollHeight in all cases unless the viewport is + * taller than the content. + * @return {number} + */ + getContentHeight() {} + /** * Returns the rect of the element within the document. * @param {!Element} unusedEl diff --git a/src/service/viewport/viewport-binding-ios-embed-wrapper.js b/src/service/viewport/viewport-binding-ios-embed-wrapper.js index 205122f1c249..a7e637871e6d 100644 --- a/src/service/viewport/viewport-binding-ios-embed-wrapper.js +++ b/src/service/viewport/viewport-binding-ios-embed-wrapper.js @@ -216,6 +216,18 @@ export class ViewportBindingIosEmbedWrapper_ { return this.wrapper_./*OK*/scrollHeight; } + /** @override */ + getContentHeight() { + // The reparented body inside wrapper will have the correct content height. + // Body is overflow: hidden so that the scrollHeight include the margins of + // body's first and last child. + // Body height doesn't include paddingTop on the parent, so we add on the + // position of the body from the top of the viewport and subtract the + // scrollTop (as position relative to the viewport changes as you scroll). + const rect = this.win.document.body./*OK*/getBoundingClientRect(); + return rect.height + rect.top + this.getScrollTop(); + } + /** @override */ getLayoutRect(el, opt_scrollLeft, opt_scrollTop) { const scrollTop = opt_scrollTop != undefined diff --git a/src/service/viewport/viewport-binding-natural.js b/src/service/viewport/viewport-binding-natural.js index 8f82a1c20bcd..29ac67ee6ef0 100644 --- a/src/service/viewport/viewport-binding-natural.js +++ b/src/service/viewport/viewport-binding-natural.js @@ -187,6 +187,18 @@ export class ViewportBindingNatural_ { return this.getScrollingElement_()./*OK*/scrollHeight; } + /** @override */ + getContentHeight() { + // The reparented body inside wrapper will have the correct content height. + // Body is overflow: hidden so that the scrollHeight include the margins of + // body's first and last child. + // Body height doesn't include paddingTop on the parent, so we add on the + // position of the body from the top of the viewport and subtract the + // scrollTop (as position relative to the viewport changes as you scroll). + const rect = this.win.document.body./*OK*/getBoundingClientRect(); + return rect.height + rect.top + this.getScrollTop(); + } + /** @override */ getLayoutRect(el, opt_scrollLeft, opt_scrollTop) { const scrollTop = opt_scrollTop != undefined diff --git a/src/service/viewport/viewport-impl.js b/src/service/viewport/viewport-impl.js index b085c66bc7b0..bbe85edaffdd 100644 --- a/src/service/viewport/viewport-impl.js +++ b/src/service/viewport/viewport-impl.js @@ -357,6 +357,19 @@ export class Viewport { return this.binding_.getScrollHeight(); } + /** + * Returns the height of the content of the document, including the + * padding top for the viewer header. + * contentHeight will match scrollHeight in all cases unless the viewport is + * taller than the content. + * Note that this method is not cached since we there's no indication when + * it might change. + * @return {number} + */ + getContentHeight() { + return this.binding_.getContentHeight(); + } + /** * Returns the rect of the viewport which includes scroll positions and size. * @return {!../../layout-rect.LayoutRectDef}} diff --git a/test/functional/test-resources.js b/test/functional/test-resources.js index f29a09f108b4..44b5a2c4f986 100644 --- a/test/functional/test-resources.js +++ b/test/functional/test-resources.js @@ -1603,7 +1603,7 @@ describe('Resources discoverWork', () => { }); }); -describes.realWin('Resources scrollHeight', { +describes.realWin('Resources contentHeight', { amp: { runtimeOn: true, }, @@ -1621,15 +1621,15 @@ describes.realWin('Resources scrollHeight', { }); }); - it('should measure initial scrollHeight', () => { - const scrollHeight = resources.viewport_.getScrollHeight(); + it('should measure initial contentHeight', () => { + const contentHeight = resources.viewport_.getContentHeight(); expect(resources.maybeChangeHeight_).to.equal(false); expect(resources.documentReady_).to.equal(true); - expect(resources.scrollHeight_).to.equal(scrollHeight); + expect(resources.contentHeight_).to.equal(contentHeight); }); - it('should send scrollHeight to viewer if height was changed', () => { - sandbox.stub(resources.viewport_, 'getScrollHeight', () => { + it('should send contentHeight to viewer if height was changed', () => { + sandbox.stub(resources.viewport_, 'getContentHeight', () => { return 200; }); resources.maybeChangeHeight_ = true; @@ -1637,33 +1637,33 @@ describes.realWin('Resources scrollHeight', { resources.doPass(); expect(resources.maybeChangeHeight_).to.equal(false); - expect(resources.scrollHeight_).to.equal(200); + expect(resources.contentHeight_).to.equal(200); expect(viewerSendMessageStub).to.be.calledOnce; expect(viewerSendMessageStub.lastCall.args[0]).to.equal('documentHeight'); expect(viewerSendMessageStub.lastCall.args[1].height).to.equal(200); expect(viewerSendMessageStub.lastCall.args[2]).to.equal(true); }); - it('should not send scrollHeight to viewer if height is not changed', () => { - const scrollHeight = resources.viewport_.getScrollHeight(); + it('should not send contentHeight to viewer if height is not changed', () => { + const contentHeight = resources.viewport_.getContentHeight(); resources.maybeChangeHeight_ = true; resources.doPass(); expect(resources.maybeChangeHeight_).to.equal(false); - expect(resources.scrollHeight_).to.equal(scrollHeight); + expect(resources.contentHeight_).to.equal(contentHeight); expect(viewerSendMessageStub).to.not.be.called; }); - it('should send scrollHeight to viewer if viewport resizes', () => { - sandbox.stub(resources.viewport_, 'getScrollHeight', () => { + it('should send contentHeight to viewer if viewport resizes', () => { + sandbox.stub(resources.viewport_, 'getContentHeight', () => { return 200; }); resources.viewport_.changed_(/* relayoutAll */ true, /* velocity */ 0); resources.doPass(); expect(resources.maybeChangeHeight_).to.equal(false); - expect(resources.scrollHeight_).to.equal(200); + expect(resources.contentHeight_).to.equal(200); expect(viewerSendMessageStub).to.be.calledOnce; expect(viewerSendMessageStub.lastCall.args[0]).to.equal('documentHeight'); expect(viewerSendMessageStub.lastCall.args[1].height).to.equal(200); diff --git a/test/functional/test-viewport-binding.js b/test/functional/test-viewport-binding.js index 7fb9026b39f5..9743f993cce9 100644 --- a/test/functional/test-viewport-binding.js +++ b/test/functional/test-viewport-binding.js @@ -32,6 +32,7 @@ describes.realWin('ViewportBindingNatural', {ampCss: true}, env => { let win; let ampdoc; let viewer; + let child; beforeEach(() => { env.iframe.style.width = '100px'; @@ -39,7 +40,7 @@ describes.realWin('ViewportBindingNatural', {ampCss: true}, env => { win = env.win; win.document.documentElement.classList.add('i-amphtml-singledoc'); - const child = win.document.createElement('div'); + child = win.document.createElement('div'); child.style.width = '200px'; child.style.height = '300px'; win.document.body.appendChild(child); @@ -121,6 +122,18 @@ describes.realWin('ViewportBindingNatural', {ampCss: true}, env => { expect(binding.getScrollHeight()).to.equal(300); }); + it('should calculate contentHeight from body height', () => { + // Set content to be smaller than viewport. + child.style.height = '50px'; + expect(binding.getContentHeight()).to.equal(50); + }); + + it('should include padding top in contentHeight', () => { + binding.updatePaddingTop(10); + binding.setScrollTop(20); // should have no effect on height + expect(binding.getContentHeight()).to.equal(310); + }); + it('should update scrollTop on scrollElement', () => { win.pageYOffset = 11; win.document.scrollingElement.scrollTop = 17; @@ -338,6 +351,18 @@ describes.realWin('ViewportBindingIosEmbedWrapper', {ampCss: true}, env => { expect(binding.getScrollHeight()).to.equal(301); // +1px for border-top. }); + it('should calculate contentHeight from body height', () => { + // Set content to be smaller than viewport. + child.style.height = '50px'; + expect(binding.getContentHeight()).to.equal(51); + }); + + it('should include padding top in contentHeight', () => { + binding.updatePaddingTop(10); + binding.setScrollTop(20); // should have no effect on height + expect(binding.getContentHeight()).to.equal(311); // +1px for border-top. + }); + it('should update scrollTop on wrapper', () => { binding.setScrollTop(21); expect(binding.wrapper_.scrollTop).to.equal(21); diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index 393b1ceb0ca5..0632259f5439 100644 --- a/test/functional/test-viewport.js +++ b/test/functional/test-viewport.js @@ -885,6 +885,12 @@ describes.fakeWin('Viewport', {}, env => { expect(viewport.getScrollHeight()).to.equal(117); }); + it('should delegate contentHeight', () => { + const bindingMock = sandbox.mock(binding); + bindingMock.expects('getContentHeight').withArgs().returns(117).once(); + expect(viewport.getContentHeight()).to.equal(117); + }); + it('should scroll to target position when the viewer sets scrollTop', () => { const bindingMock = sandbox.mock(binding); bindingMock.expects('setScrollTop').withArgs(117).once(); diff --git a/testing/describes.js b/testing/describes.js index c9e9f704c245..d2dbcfea240b 100644 --- a/testing/describes.js +++ b/testing/describes.js @@ -555,7 +555,7 @@ class RealWinFixture { const iframe = document.createElement('iframe'); env.iframe = iframe; iframe.name = 'test_' + iframeCount++; - iframe.srcdoc = '' + + iframe.srcdoc = '' + '' + '
'; iframe.onload = function() {