`documentHeight` calculation not considering bottom margin/padding and overflow content #13343

zhangsu · 2018-02-07T18:53:24Z

We used to calculate the documentHeight value using scrollHeight on scrollingElement, but we recently changed the formula in #12229 to use the height of the bounding client rect of <body> plus any margin/padding on any element between the top of the viewport and the the top of <body>:

amphtml/src/service/viewport/viewport-binding-natural.js

Line 205 in 7b211a1

return rect.height + rect.top + this.getScrollTop();

This fixes the issue when the actual document content height is smaller than the viewport height, in which case scrollHeight is always set to the height of the viewport, resulting in incorrect document height.

However, with the following AMP HTML:

<!doctype html>
<html ⚡>
<head>
<meta charset="utf-8">
<link rel="canonical" href="./regular-html-version.html">
<meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
<script async src="https://cdn.ampproject.org/v0.js"></script>
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<style amp-custom>
  html {
    margin: 10px 0;
    padding: 20px 0;
    border: 2px solid blue;
  }

  body {
    height: 100px;
    margin: 10px 0;
    padding: 20px 0;
    border: 2px solid red;
  }

  .container {
    height: 100px;
  }
  .foo {
    margin: 800px 0;
  }
</style>
</head>
<body id="body">
  <div class="container">
    <div class="foo">test</div>
  </div>
</body>
</html>

The new formula doesn't result in the same number as the original formula:

> rect = document.body.getBoundingClientRect(); rect.height + rect.top + document.scrollingElement.scrollTop
956
> document.scrollingElement.scrollHeight
1652

There are two problems:

The new formula is not taking the margin-bottom and padding-bottom of the <html> element into consideration. 956 = 944 (bounding client rect height of <body>) + 12 (top margin and border of <html>), but with the padding, border and margin at the bottom of <html>, the number should really be 956 + 20 + 2 + 10 = 988.
The new formula is not taking overflow content into consideration. .container is set to have a fixed height of 100px, while its only child .foo has a top and bottom margin of 800px. As a result, the bottom margin of .foo overflows its parent .container. In the old formula, scrollHeight of <html> actually includes the overflow margin, but the new formula doesn't, as the bounding client rect of <html> (or <body>) doesn't include the overflow margin in descendent nodes.

For problem 1, I managed to fix it with the following formula:

> document.scrollingElement.getBoundingClientRect().height +
    parseInt(getComputedStyle(document.scrollingElement).marginTop, 10) +
    parseInt(getComputedStyle(document.scrollingElement).marginBottom, 10)
988

The bounding client rect of <html> includes padding and border of <html>, but not margin, so including the top and bottom margin should give the correct result. However, this doesn't fix problem 2. Not sure how we should approach problem 2 without re-implementing too much of the layout engine.

@peterjosling @jridgewell @raywainman @choumx

The text was updated successfully, but these errors were encountered:

ampprojectbot · 2018-06-05T08:05:10Z

This issue hasn't been updated in awhile. @jridgewell Do you have any updates?

ampprojectbot · 2018-09-11T10:43:20Z

This issue hasn't been updated in awhile. @jridgewell Do you have any updates?

ampprojectbot · 2018-12-12T09:33:44Z

This issue hasn't been updated in awhile. @jridgewell Do you have any updates?

dreamofabear · 2018-12-13T23:32:38Z

Is this still an issue? I wonder if this was related to quirks mode.

zhangsu · 2018-12-14T05:21:11Z

I just tried again and can still reproduce. If I simply set the height of the iframe to whatever the documentHeight AMP runtime sends to the parent, then the everything starting at the blue bottom border of the html element (which includes the blue bottom border and the margin-bottom of the html element) gets cut off and overflows the iframe container.

dreamofabear · 2018-12-14T16:38:29Z

Just read the original issue, makes sense. #12229 assumed it "should be a no-op for docs which
are already taller than the viewport." This is not true as Su describes and we should fix getContentHeight().

@jridgewell Let's get this in the fix-it.

jridgewell · 2018-12-17T21:45:57Z

Why aren't we just using scrollHeight?

dreamofabear · 2018-12-17T21:55:01Z

We should use scrollHeight when scrollHeight > document.documentElement.clientHeight, and Peter's "content height" otherwise.

jridgewell · 2018-12-17T21:57:30Z

clientHeight can't be trusted to give an accurate height. How about screen height or scroll height?

jridgewell · 2018-12-18T21:44:23Z

Doing some checks:

Chrome (Galaxy S5)	short	long	margin+padding
`window.innerHeight`	640px	640px	640px
`viewport.getScrollingHeight()`	640px	3056px	1706px
`viewport.getContentHeight()`	112.4375px	3034.3125px	1010px
`document.documentElement.offsetHeight`	134px	3056px	1022px
Safari (iPhone 5S)
`window.innerHeight`	454px	454px	454px
`viewport.getScrollHeight()`	454px	3108px	1017px
`viewport.getContentHeight()`	114.4375px	3086.3125px	999px
`document.body.offsetHeight`	60px	3032px	943px

jridgewell · 2018-12-18T22:39:07Z

So what is it that we want here? This entire thing confuses the hell out of me.

zhangsu · 2018-12-19T22:24:37Z

My understanding is that we are not simply using scrollHeight because scrollHeight is capped at the viewport height, so if the viewport is taller than the actual "content", then scrollHeight would just be equal to the viewport height.

Maybe a better way to define the desired documentHeight would be:

documentHeight should always be equal to the document.scrollingElement.scrollHeight obtained from a viewport that has vertical scrollbar displayed by the browser due to viewport height being < the actual content height. If the viewport height is larger than the content height, then we should get the correct documentHeight by getting document.scrollingElement.scrollHeight after we shrink the viewport until a vertical scrollbar appears.

Tried my best to put these into words... But I don't know how we can do this in a nice way programmatically. As I mentioned in the issue description at the top, Peter's "content height" algorithm doesn't really include bottom padding and margin on the <html> element, nor does it correctly calculate the content height when there's an element that overflows <body>.

dreamofabear · 2018-12-20T23:10:35Z

I'm not aware of a good way to do that either.

We could use scrollHeight when the document doesn't fit in viewport. But after some thought, it's probably better to be "consistently wrong" than "sometimes wrong depending on browser size".

I think AMP developers should be able to restructure overflowing child elements such that they're recognized by our "content height" calculation. Is this possible for your use case, Su?

Also, note that document.scrollingElement.scrollHeight actually doesn't add margin of the <html> element. However, it's probably more intuitive to include it in our definition of "content height".

The height is measured in the same way as clientHeight: it includes the element's padding, but not its border, margin or horizontal scrollbar (if present)

WDYT about using the formula above in OP (and leaving problem (2) unsolved)?

getContentHeight() {
  const scrollingElement = this.getScrollingElement();
  const rect = scrollingElement.getBoundingClientRect();
  const style = getComputedStyle(scrollingElement);
  return rect.height + parseInt(style.marginTop, 10) + parseInt(style.marginBottom, 10);
}

zhangsu · 2018-12-21T03:57:28Z

Sounds good. We can document the overflow caveat in a best practices doc somewhere for developers.

dreamofabear added Type: Bug WG: runtime labels Feb 7, 2018

dreamofabear assigned jridgewell Feb 7, 2018

ampprojectbot added this to the Backlog Bugs milestone Feb 12, 2018

ampprojectbot added the P2: Soon label Feb 19, 2018

dreamofabear modified the milestones: Backlog Bugs, Fixit End of 2018 Dec 14, 2018

This was referenced Dec 21, 2018

attemptChangeSize not denied for above-the-fold iframe #19941

Closed

🐛 Update contentHeight algorithms #20035

Merged

jridgewell closed this as completed in #20035 Jan 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`documentHeight` calculation not considering bottom margin/padding and overflow content #13343

`documentHeight` calculation not considering bottom margin/padding and overflow content #13343

zhangsu commented Feb 7, 2018 •

edited

Loading

ampprojectbot commented Jun 5, 2018

ampprojectbot commented Sep 11, 2018

ampprojectbot commented Dec 12, 2018

dreamofabear commented Dec 13, 2018

zhangsu commented Dec 14, 2018

dreamofabear commented Dec 14, 2018

jridgewell commented Dec 17, 2018

dreamofabear commented Dec 17, 2018 •

edited

Loading

jridgewell commented Dec 17, 2018

jridgewell commented Dec 18, 2018 •

edited

Loading

jridgewell commented Dec 18, 2018

zhangsu commented Dec 19, 2018

dreamofabear commented Dec 20, 2018

zhangsu commented Dec 21, 2018

documentHeight calculation not considering bottom margin/padding and overflow content #13343

documentHeight calculation not considering bottom margin/padding and overflow content #13343

Comments

zhangsu commented Feb 7, 2018 • edited Loading

ampprojectbot commented Jun 5, 2018

ampprojectbot commented Sep 11, 2018

ampprojectbot commented Dec 12, 2018

dreamofabear commented Dec 13, 2018

zhangsu commented Dec 14, 2018

dreamofabear commented Dec 14, 2018

jridgewell commented Dec 17, 2018

dreamofabear commented Dec 17, 2018 • edited Loading

jridgewell commented Dec 17, 2018

jridgewell commented Dec 18, 2018 • edited Loading

jridgewell commented Dec 18, 2018

zhangsu commented Dec 19, 2018

dreamofabear commented Dec 20, 2018

zhangsu commented Dec 21, 2018

`documentHeight` calculation not considering bottom margin/padding and overflow content #13343

`documentHeight` calculation not considering bottom margin/padding and overflow content #13343

zhangsu commented Feb 7, 2018 •

edited

Loading

dreamofabear commented Dec 17, 2018 •

edited

Loading

jridgewell commented Dec 18, 2018 •

edited

Loading