diff --git a/src/service/viewport/viewport-binding-ios-embed-sd.js b/src/service/viewport/viewport-binding-ios-embed-sd.js index 184703c6ac56..9e2cf0e72b2c 100644 --- a/src/service/viewport/viewport-binding-ios-embed-sd.js +++ b/src/service/viewport/viewport-binding-ios-embed-sd.js @@ -174,9 +174,6 @@ export class ViewportBindingIosEmbedShadowRoot_ { /** @private @const {boolean} */ this.useLayers_ = isExperimentOn(this.win, 'layers'); - /** @private {number} */ - this.paddingTop_ = 0; - /** @private {boolean} */ this.bodySyncScheduled_ = false; @@ -320,7 +317,6 @@ export class ViewportBindingIosEmbedShadowRoot_ { /** @override */ updatePaddingTop(paddingTop) { - this.paddingTop_ = paddingTop; setImportantStyles(this.scroller_, { 'padding-top': px(paddingTop), }); @@ -397,14 +393,20 @@ export class ViewportBindingIosEmbedShadowRoot_ { /** @override */ getContentHeight() { // Don't use scrollHeight, since it returns `MAX(viewport_height, - // document_height)`, even though we only want the latter. Also, it doesn't - // account for margins - const scrollingElement = this.wrapper_; - const rect = scrollingElement./*OK*/getBoundingClientRect(); - const style = computedStyle(this.win, scrollingElement); + // document_height)` (we only want the latter), and it doesn't account + // for margins. + const bodyWrapper = this.wrapper_; + const rect = bodyWrapper./*OK*/getBoundingClientRect(); + const style = computedStyle(this.win, bodyWrapper); + // The Y-position of any element can be offset by the vertical margin + // of its first child, and this is _not_ accounted for in `rect.height`. + // This "top gap" causes smaller than expected contentHeight, so calculate + // and add it manually. Note that the "top gap" includes any padding-top + // on ancestor elements and the scroller's border-top. The "bottom gap" + // remains unaddressed. + const topGapPlusPaddingAndBorder = rect.top + this.getScrollTop(); return rect.height - + this.paddingTop_ - + this.getBorderTop() + + topGapPlusPaddingAndBorder + parseInt(style.marginTop, 10) + parseInt(style.marginBottom, 10); } diff --git a/src/service/viewport/viewport-binding-ios-embed-wrapper.js b/src/service/viewport/viewport-binding-ios-embed-wrapper.js index 676e048a7769..d92e14ae1303 100644 --- a/src/service/viewport/viewport-binding-ios-embed-wrapper.js +++ b/src/service/viewport/viewport-binding-ios-embed-wrapper.js @@ -239,11 +239,14 @@ export class ViewportBindingIosEmbedWrapper_ { /** @override */ getContentHeight() { // Don't use scrollHeight, since it returns `MAX(viewport_height, - // document_height)`, even though we only want the latter. Also, it doesn't - // account for margins + // document_height)` (we only want the latter), and it doesn't account + // for margins. const scrollingElement = this.win.document.body; const rect = scrollingElement./*OK*/getBoundingClientRect(); const style = computedStyle(this.win, scrollingElement); + // Note: unlike viewport-binding-natural.js, there's no need to calculate + // the "top gap" since the wrapped body _does_ account for child margins. + // However, the parent's paddingTop still needs to be added. return rect.height + this.paddingTop_ + parseInt(style.marginTop, 10) diff --git a/src/service/viewport/viewport-binding-natural.js b/src/service/viewport/viewport-binding-natural.js index 17674a614f47..db39ae703a86 100644 --- a/src/service/viewport/viewport-binding-natural.js +++ b/src/service/viewport/viewport-binding-natural.js @@ -67,9 +67,6 @@ export class ViewportBindingNatural_ { /** @private @const {boolean} */ this.useLayers_ = isExperimentOn(this.win, 'layers'); - /** @private {number} */ - this.paddingTop_ = 0; - dev().fine(TAG_, 'initialized natural viewport'); } @@ -117,7 +114,6 @@ export class ViewportBindingNatural_ { /** @override */ updatePaddingTop(paddingTop) { - this.paddingTop_ = paddingTop; setImportantStyles(this.win.document.documentElement, { 'padding-top': px(paddingTop), }); @@ -200,17 +196,20 @@ export class ViewportBindingNatural_ { /** @override */ getContentHeight() { // Don't use scrollHeight, since it returns `MAX(viewport_height, - // document_height)`, even though we only want the latter. Also, it doesn't - // account for margins + // document_height)` (we only want the latter), and it doesn't account + // for margins. Also, don't use documentElement's rect height because + // there's no workable analog for either ios-embed-* modes. const scrollingElement = this.getScrollingElement(); const rect = scrollingElement./*OK*/getBoundingClientRect(); const style = computedStyle(this.win, scrollingElement); - let paddingTop = 0; - if (scrollingElement !== this.win.document.documentElement) { - paddingTop = this.paddingTop_; - } + // The Y-position of any element can be offset by the vertical margin + // of its first child, and this is _not_ accounted for in `rect.height`. + // This "top gap" causes smaller than expected contentHeight, so calculate + // and add it manually. Note that the "top gap" includes any padding-top + // on ancestor elements, and the "bottom gap" remains unaddressed. + const topGapPlusPadding = rect.top + this.getScrollTop(); return rect.height - + paddingTop + + topGapPlusPadding + parseInt(style.marginTop, 10) + parseInt(style.marginBottom, 10); } diff --git a/test/unit/test-viewport-binding.js b/test/unit/test-viewport-binding.js index 73c9af37bb21..e2a2803f2926 100644 --- a/test/unit/test-viewport-binding.js +++ b/test/unit/test-viewport-binding.js @@ -37,8 +37,11 @@ describes.realWin('ViewportBindingNatural', {ampCss: true}, env => { let ampdoc; let viewer; let child; + let sandbox; beforeEach(() => { + sandbox = env.sandbox; + env.iframe.style.width = '100px'; env.iframe.style.height = '200px'; win = env.win; @@ -138,6 +141,19 @@ describes.realWin('ViewportBindingNatural', {ampCss: true}, env => { expect(binding.getContentHeight()).to.equal(310); }); + it('should account for child margin-top', () => { + child.style.marginTop = '15px'; + expect(binding.getContentHeight()).to.equal(315); + }); + + it('should account for child margin-top (WebKit)', () => { + sandbox.stub(win.document, 'scrollingElement').value(null); + sandbox.stub(binding.platform_, 'isWebKit').returns(true); + + child.style.marginTop = '15px'; + expect(binding.getContentHeight()).to.equal(315); + }); + it('should update scrollTop on scrollElement', () => { win.pageYOffset = 11; win.document.scrollingElement.scrollTop = 17; @@ -183,7 +199,6 @@ describes.realWin('ViewportBindingNatural', {ampCss: true}, env => { expect(rect.height).to.equal(15); // round(14.5) }); - it('should disable scroll temporarily and reset scroll', () => { let htmlCss = win.getComputedStyle(win.document.documentElement); expect(htmlCss.overflowX).to.equal('hidden'); @@ -386,6 +401,11 @@ describes.realWin('ViewportBindingIosEmbedWrapper', {ampCss: true}, env => { expect(binding.getContentHeight()).to.equal(311); // +1px for border-top. }); + it('should account for child margin-top', () => { + child.style.marginTop = '15px'; + expect(binding.getContentHeight()).to.equal(316); // +1px for border-top. + }); + it('should update scrollTop on wrapper', () => { binding.setScrollTop(21); expect(binding.wrapper_.scrollTop).to.equal(21); @@ -731,6 +751,11 @@ describes.realWin('ViewportBindingIosEmbedShadowRoot_', {ampCss: true}, env => { expect(binding.getContentHeight()).to.equal(311); // +1px for border-top. }); + it('should account for child margin-top', () => { + child.style.marginTop = '15px'; + expect(binding.getContentHeight()).to.equal(316); // +1px for border-top. + }); + it('should update scrollTop on scroller', () => { binding.setScrollTop(21); expect(binding.scroller_.scrollTop).to.equal(21);