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>:
|
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
We used to calculate the
documentHeightvalue usingscrollHeightonscrollingElement, 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
This fixes the issue when the actual document content height is smaller than the viewport height, in which case
scrollHeightis always set to the height of the viewport, resulting in incorrect document height.However, with the following AMP HTML:
The new formula doesn't result in the same number as the original formula:
There are two problems:
The new formula is not taking the
margin-bottomandpadding-bottomof 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.
.containeris set to have a fixed height of100px, while its only child.foohas a top and bottom margin of800px. As a result, the bottom margin of.foooverflows its parent.container. In the old formula,scrollHeightof<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:
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