Calculate correct documentHeight for short AMP docs (ampproject#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
ampproject@ac741f6,
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

@@ -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;}
 }

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_);
         }
       });
     }

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

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

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

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}}

test/functional/test-resources.js

@@ -1603,7 +1603,7 @@ describe('Resources discoverWork', () => {
   });
 });
 
-describes.realWin('Resources scrollHeight', {
+describes.realWin('Resources contentHeight', {
   amp: {
     runtimeOn: true,
   },
@@ -1621,49 +1621,49 @@ 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;
 
     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);

test/functional/test-viewport-binding.js

@@ -32,14 +32,15 @@ describes.realWin('ViewportBindingNatural', {ampCss: true}, env => {
   let win;
   let ampdoc;
   let viewer;
+  let child;
 
   beforeEach(() => {
     env.iframe.style.width = '100px';
     env.iframe.style.height = '200px';
     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);

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();

testing/describes.js

@@ -555,7 +555,7 @@ class RealWinFixture {
       const iframe = document.createElement('iframe');
       env.iframe = iframe;
       iframe.name = 'test_' + iframeCount++;
-      iframe.srcdoc = '<!doctype><html><head>' +
+      iframe.srcdoc = '<!doctype html><html><head>' +
           '<style>.i-amphtml-element {display: block;}</style>' +
           '<body style="margin:0"><div id=parent></div>';
       iframe.onload = function() {