Measure contentHeight instead for doc with little content

src/service/resources-impl.js

@@ -173,7 +173,7 @@ export class Resources {
     this.vsyncScheduled_ = false;
 
     /** @private {number} */
-    this.scrollHeight_ = 0;
+    this.contentHeight_ = 0;
 
     /** @private {boolean} */
     this.maybeChangeHeight_ = false;
@@ -851,10 +851,10 @@ export class Resources {
         linkRels: documentInfoForDoc(this.ampdoc).linkRels,
       }, /* cancelUnsent */true);
 
-      this.scrollHeight_ = this.viewport_.getScrollHeight();
+      this.contentHeight_ = this.viewport_.getContentHeight();
       this.viewer_.sendMessage('documentHeight',
-          {height: this.scrollHeight_}, /* cancelUnsent */true);
-      dev().fine(TAG_, 'document height on load: ' + this.scrollHeight_);
+          {height: this.contentHeight_}, /* cancelUnsent */true);
+      dev().fine(TAG_, 'document height on load: ' + this.contentHeight_);
     }
 
     const viewportSize = this.viewport_.getSize();
@@ -872,18 +872,18 @@ export class Resources {
       this.viewer_.getVisibilityState()
     );
 
-    this.vsync_.measure(() => {
-      if (this.maybeChangeHeight_) {
-        const measuredScrollHeight = this.viewport_.getScrollHeight();
-        if (measuredScrollHeight != this.scrollHeight_) {
+    if (this.maybeChangeHeight_) {
+      this.vsync_.measure(() => {
+        const measuredContentHeight = this.viewport_.getContentHeight();
+        if (measuredContentHeight != this.contentHeight_) {
           this.viewer_.sendMessage('documentHeight',
-              {height: measuredScrollHeight}, /* cancelUnsent */true);
-          this.scrollHeight_ = measuredScrollHeight;
-          dev().fine(TAG_, 'document height changed: ' + this.scrollHeight_);
+              {height: measuredContentHeight}, /* cancelUnsent */true);
+          this.contentHeight_ = measuredContentHeight;
+          dev().fine(TAG_, 'document height changed: ' + this.contentHeight_);
         }
         this.maybeChangeHeight_ = false;
-      }
-    });
+      });
+    }
   }
 
   /**

src/service/viewport-impl.js

@@ -308,14 +308,29 @@ export class Viewport {
   }
 
   /**
-   * Returns the scroll height of the content of the document. Note that this
-   * method is not cached since we there's no indication when it might change.
+   * Returns the scroll height of the content of the document.
+   * The scrollHeight will be the viewport height if there's not enough content
+   * to fill up the viewport.
+   * Note that this method is not cached since we there's no indication when
+   * it might change.
    * @return {number}
    */
   getScrollHeight() {
     return this.binding_.getScrollHeight();
   }
 
+  /**
+   * Returns the scroll height of the content of the document.
+   * The scrollHeight will be the exact content height if there's not enough
+   * content to fill up the viewport.
+   * 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}}
@@ -847,10 +862,20 @@ export class ViewportBindingDef {
 
   /**
    * Returns the scroll height of the content of the document.
+   * The scrollHeight will be the viewport height if there's not enough content
+   * to fill up the viewport.
    * @return {number}
    */
   getScrollHeight() {}
 
+  /**
+   * Returns the scroll height of the content of the document.
+   * The scrollHeight will be the exact content height if there's not enough
+   * content to fill up the viewport.
+   * @return {number}
+   */
+  getContentHeight() {}
+
   /**
    * Returns the rect of the element within the document.
    * @param {!Element} unusedEl
@@ -1028,6 +1053,11 @@ export class ViewportBindingNatural_ {
     return this.getScrollingElement_()./*OK*/scrollHeight;
   }
 
+  /** @override */
+  getContentHeight() {
+    return this.win.document.documentElement./*OK*/scrollHeight;
+  }
+
   /** @override */
   getLayoutRect(el, opt_scrollLeft, opt_scrollTop) {
     const scrollTop = opt_scrollTop != undefined
@@ -1312,6 +1342,11 @@ export class ViewportBindingNaturalIosEmbed_ {
 
   /** @override */
   getScrollHeight() {
+    return this.getContentHeight();
+  }
+
+  /** @override */
+  getContentHeight() {
     // We have to use a special "tail" element on iOS due to the issues outlined
     // in the {@link onScrolled_} method. Because we are forced to layout BODY
     // with position:absolute, we can no longer use BODY's scrollHeight to
@@ -1479,6 +1514,12 @@ export class ViewportBindingIosEmbedWrapper_ {
     // - https://bugs.webkit.org/show_bug.cgi?id=149264
     const doc = this.win.document;
     const body = dev().assertElement(doc.body, 'body is not available');
+
+    // Setting overflow: hidden to body gives the correct value for body's
+    // scrollHeight, otherwise the last child's margin-bottom will be discarded
+    // due to margin collapsing
+    setStyle(body, 'overflow', 'hidden');
+
     doc.documentElement.appendChild(this.wrapper_);
     this.wrapper_.appendChild(body);
     // Redefine `document.body`, otherwise it'd be `null`.
@@ -1571,6 +1612,16 @@ export class ViewportBindingIosEmbedWrapper_ {
     return this.wrapper_./*OK*/scrollHeight;
   }
 
+  /** @override */
+  getContentHeight() {
+    const body = this.win.document.body;
+    if (!body) {
+      return 0;
+    }
+    // The reparented body inside wrapper will have the correct content height.
+    return body./*OK*/scrollHeight;
+  }
+
   /** @override */
   getLayoutRect(el, opt_scrollLeft, opt_scrollTop) {
     const scrollTop = opt_scrollTop != undefined

test/functional/test-viewport.js

@@ -1536,8 +1536,8 @@ describes.realWin('ViewportBindingIosEmbedWrapper', {ampCss: true}, env => {
     expect(htmlCss.overflowX).to.equal('hidden');
     expect(wrapperCss.overflowY).to.equal('auto');
     expect(wrapperCss.overflowX).to.equal('hidden');
-    expect(bodyCss.overflowY).to.equal('visible');
-    expect(bodyCss.overflowX).to.equal('visible');
+    expect(bodyCss.overflowY).to.equal('hidden');
+    expect(bodyCss.overflowX).to.equal('hidden');
 
     // Wrapper must be a block and positioned absolute at 0/0/0/0.
     expect(wrapperCss.display).to.equal('block');