Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include child margin-bottom in content height #22718

Merged
merged 6 commits into from
Jun 11, 2019
Merged

Include child margin-bottom in content height #22718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e532fe6
Include bottom margin in getContentHeight.
Jun 4, 2019
2f20ac7
Support other viewport bindings.
Jun 4, 2019
d7aa84f
Dedupe code with helper function.
Jun 6, 2019
19f3070
Restrict to Safari.
Jun 6, 2019
fb0dbe4
Cleaner code.
Jun 6, 2019
662fa44
Add experiment description.
Jun 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/service/viewport/viewport-binding-def.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import {computedStyle} from '../../style';
import {isExperimentOn} from '../../experiments';

/**
* Copyright 2017 The AMP HTML Authors. All Rights Reserved.
*
Expand Down Expand Up @@ -200,3 +203,27 @@ export class ViewportBindingDef {
*/
getScrollingElementScrollsLikeViewport() {}
}

/**
* Returns the margin-bottom of the last child of `element` with non-zero
* height, if any. Otherwise, returns 0.
*
* @param {!Window} win
* @param {!Element} element
* @return {number}
*/
export function marginBottomOfLastChild(win, element) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this algorithm come from?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this how it actually behaves?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, try it out. Note that it only repros on Safari, not Chrome with emulation.

<!doctype html>
<html ⚡4email>
<head>
  <meta charset="utf-8">
  <script async src="https://cdn.ampproject.org/v0.js"></script>
  <style amp4email-boilerplate>body{visibility:hidden}</style>
  <meta name='viewport' content='initial-scale=1'>
  <style amp-custom>
    .red { background-color: red; }
    .blue { background-color: blue; }
    .yellow { background-color: yellow; }
    html { padding-top:50px; }
    #first { margin-top: 100px; }
    #last { margin-bottom: 100px; }
  </style>
</head>
<body>
<p id="first" >
  First element.
</p>

<div id="lipsum">
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec condimentum feugiat nisi, et bibendum est interdum ac. Sed eu pretium lacus. Cras eu laoreet lacus. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Mauris non turpis ipsum. Phasellus sodales dui quam, nec luctus mi euismod nec. Integer vitae nibh leo. Donec leo elit, varius a sapien non, iaculis eleifend odio. In pharetra ultrices magna, at dignissim nisi. Nunc viverra vel nibh ac dictum. Ut iaculis sapien id justo sollicitudin imperdiet. Etiam hendrerit posuere nisi.</p>
<p>Maecenas pretium aliquam neque, eu convallis ipsum lobortis quis. Nunc fermentum vestibulum vulputate. Sed blandit faucibus tellus nec porttitor. Suspendisse faucibus justo sit amet erat convallis pulvinar. Sed mollis, turpis vel faucibus auctor, turpis ipsum efficitur erat, vel tincidunt dolor quam non tortor. In porta porttitor quam ut efficitur. Sed hendrerit condimentum dui in consectetur. Integer a sem egestas, lobortis massa in, consectetur risus. Morbi ut ullamcorper lacus, nec consequat erat. Aenean euismod tristique lorem ac tempor. Curabitur non semper ipsum. Nulla non ante pharetra, viverra urna et, consectetur arcu. Phasellus dapibus consectetur convallis.</p>
<p>Praesent mollis, quam non finibus semper, diam augue elementum lectus, at lobortis justo dolor vel est. Praesent sit amet eleifend lorem. Donec aliquet, dui sit amet rhoncus tempus, dolor ex dignissim ligula, et dictum diam eros nec ex. In ac libero tristique, accumsan mauris et, posuere eros. Cras viverra eros nunc, et egestas odio semper eu. Proin ac vehicula massa. Duis eget ante maximus, condimentum diam in, pretium odio. Proin nec leo ac lorem suscipit faucibus. Nam in est sed magna suscipit congue a id tellus. Fusce ultrices lorem et ullamcorper volutpat. Praesent eu feugiat nunc, convallis finibus ligula. Integer gravida nulla eu viverra ullamcorper.</p>
<p>Mauris condimentum est vitae magna maximus, eu placerat nisi eleifend. Maecenas lobortis, tortor et maximus pellentesque, erat ante pharetra orci, vel efficitur metus elit in urna. Cras et suscipit orci. Praesent sollicitudin vestibulum scelerisque. Nulla justo enim, hendrerit in suscipit sit amet, scelerisque at neque. Nam volutpat ipsum non metus interdum efficitur. Aenean non mauris eget tellus ullamcorper hendrerit et nec nisi. Vivamus ultricies ut purus non ullamcorper. Curabitur eget ornare eros. Nunc fermentum congue venenatis. Nulla id egestas nunc, vel auctor dui. Sed congue nunc vel orci accumsan vehicula. Curabitur ut suscipit neque, vitae gravida diam. Donec congue volutpat massa et sagittis. Duis quis quam gravida, posuere sapien et, rutrum ipsum. Nullam non pretium nisi.</p>
<p>Pellentesque sed auctor massa. Mauris at nisl metus. Duis nec dolor commodo, vestibulum risus a, mollis magna. Fusce sit amet velit et justo imperdiet sollicitudin. Quisque eu facilisis lorem. In massa urna, dapibus id sollicitudin quis, egestas eget tellus. Aliquam blandit ipsum vel maximus vulputate. Sed at accumsan turpis. Sed vel velit odio. Morbi scelerisque mauris quis neque condimentum, non ultrices dui finibus.</p>
<p>Vivamus ultrices urna quis diam vulputate, sit amet condimentum nulla accumsan. Curabitur libero elit, finibus eget neque a, volutpat vestibulum felis. Vestibulum faucibus orci ac turpis sodales venenatis. Nunc erat metus, finibus nec dapibus nec, eleifend eu purus. Nunc euismod sodales sem, non hendrerit dui ullamcorper non. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Morbi id metus velit. In nec nisl ante.</p>
<p>Integer et quam eu magna vestibulum semper sit amet rutrum augue. Vestibulum eget congue quam. Integer placerat congue diam. Etiam rhoncus lorem id ex fringilla, vitae euismod quam posuere. Ut urna turpis, cursus ac dui in, lobortis tristique risus. Cras lacinia ipsum ut erat tincidunt, vel imperdiet ante ornare. Etiam rutrum vitae quam ut vulputate.</p>
<p>Praesent malesuada quis dui sed consequat. Sed suscipit malesuada sem, ac hendrerit neque bibendum a. Curabitur id dapibus justo, eu dapibus ligula. Ut nec bibendum nisl. Donec placerat lorem in tempus sagittis. Quisque dui urna, rutrum vitae dignissim nec, consequat sed ligula. Nam quam mi, condimentum ac rutrum sit amet, posuere quis tellus. Morbi venenatis dictum nibh, at sollicitudin elit finibus vel. Vestibulum a ex malesuada, auctor libero eu, finibus quam. Curabitur maximus nec lacus nec egestas. Interdum et malesuada fames ac ante ipsum primis in faucibus. Vivamus rhoncus magna erat, nec consectetur eros vulputate vel.</p>
<p>Nulla facilisi. Pellentesque ac imperdiet nibh, eu vestibulum neque. Curabitur aliquet dolor ut placerat tristique. Morbi vel pulvinar tortor, pharetra commodo mi. Ut facilisis diam eros, nec suscipit ipsum ultrices ut. Sed sollicitudin fermentum nunc, non varius risus efficitur elementum. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Etiam auctor feugiat malesuada. Pellentesque malesuada diam eros, at tempor nisi feugiat ac. Nulla quis justo eu velit gravida maximus commodo a massa. Maecenas ullamcorper enim ac tempus ultricies. Ut venenatis maximus fermentum.</p>
<p>Nam sed fermentum tortor. Donec felis turpis, auctor a ipsum sed, lacinia varius dolor. Mauris ut ornare sem. Vestibulum iaculis justo ac eros faucibus, in dictum lectus porttitor. Praesent semper, felis ac pulvinar posuere, est turpis cursus arcu, non interdum libero purus sed augue. Nam convallis varius nunc a porta. Etiam arcu dolor, interdum in tellus ut, viverra porta quam. Nulla id nisl nec tortor ornare molestie.</p>
</div>

<p id="last">Last element.</p>
<p></p>

</body>
</html>

if (!isExperimentOn(win, 'margin-bottom-in-content-height')) {
return 0;
}
let n = element.lastElementChild;
while (n) {
const r = n./*OK*/ getBoundingClientRect();
if (r.height > 0) {
break;
} else {
n = n.previousElementSibling;
}
}
return n ? parseInt(computedStyle(win, n).marginBottom, 10) : 0;
}
36 changes: 24 additions & 12 deletions src/service/viewport/viewport-binding-ios-embed-sd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

import {Observable} from '../../observable';
import {Services} from '../../services';
import {ViewportBindingDef} from './viewport-binding-def';
import {
ViewportBindingDef,
marginBottomOfLastChild,
} from './viewport-binding-def';
import {
assertDoesNotContainDisplay,
computedStyle,
Expand Down Expand Up @@ -400,20 +403,29 @@ export class ViewportBindingIosEmbedShadowRoot_ {
// Don't use scrollHeight, since it returns `MAX(viewport_height,
// 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
const content = this.wrapper_;
const rect = content./*OK*/ getBoundingClientRect();

// The Y-position of `content` 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();
// This causes smaller than expected content height, so add it manually.
// Note this "top" value already includes padding-top of ancestor elements
// and getBorderTop().
const top = rect.top + this.getScrollTop();

// As of Safari 12.1.1, the getBoundingClientRect().height does not include
// the bottom margin of children and there's no other API that does.
const childMarginBottom = marginBottomOfLastChild(
this.win,
this.win.document.body
);

const style = computedStyle(this.win, content);
return (
rect.height +
topGapPlusPaddingAndBorder +
top +
parseInt(style.marginTop, 10) +
rect.height +
childMarginBottom +
parseInt(style.marginBottom, 10)
);
}
Expand Down
32 changes: 20 additions & 12 deletions src/service/viewport/viewport-binding-ios-embed-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

import {Observable} from '../../observable';
import {Services} from '../../services';
import {ViewportBindingDef} from './viewport-binding-def';
import {
ViewportBindingDef,
marginBottomOfLastChild,
} from './viewport-binding-def';
import {computedStyle, px, setImportantStyles} from '../../style';
import {dev} from '../../log';
import {isExperimentOn} from '../../experiments';
Expand Down Expand Up @@ -246,19 +249,24 @@ export class ViewportBindingIosEmbedWrapper_ {

/** @override */
getContentHeight() {
// Don't use scrollHeight, since it returns `MAX(viewport_height,
// 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.
// The wrapped body, not this.wrapper_ itself, will have the correct height.
const content = this.win.document.body;
const {height} = content./*OK*/ getBoundingClientRect();

// Unlike other viewport bindings, there's no need to include the
// rect top since the wrapped body accounts for the top margin of children.
// However, the parent's padding-top (this.paddingTop_) must be added.

// As of Safari 12.1.1, the getBoundingClientRect().height does not include
// the bottom margin of children and there's no other API that does.
const childMarginBottom = marginBottomOfLastChild(this.win, content);

const style = computedStyle(this.win, content);
return (
rect.height +
this.paddingTop_ +
parseInt(style.marginTop, 10) +
this.paddingTop_ +
height +
childMarginBottom +
parseInt(style.marginBottom, 10)
);
}
Expand Down
34 changes: 23 additions & 11 deletions src/service/viewport/viewport-binding-natural.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

import {Observable} from '../../observable';
import {Services} from '../../services';
import {ViewportBindingDef} from './viewport-binding-def';
import {
ViewportBindingDef,
marginBottomOfLastChild,
} from './viewport-binding-def';
import {computedStyle, px, setImportantStyles} from '../../style';
import {dev} from '../../log';
import {isExperimentOn} from '../../experiments';
Expand Down Expand Up @@ -205,19 +208,28 @@ export class ViewportBindingNatural_ {
// 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);
// The Y-position of any element can be offset by the vertical margin
const content = this.getScrollingElement();
const rect = content./*OK*/ getBoundingClientRect();

// The Y-position of `content` 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();
// This causes smaller than expected content height, so add it manually.
// Note this "top" value already includes padding-top of ancestor elements
// and getBorderTop().
const top = rect.top + this.getScrollTop();

// As of Safari 12.1.1, the getBoundingClientRect().height does not include
// the bottom margin of children and there's no other API that does.
const childMarginBottom = Services.platformFor(this.win).isSafari()
? marginBottomOfLastChild(this.win, content)
: 0;

const style = computedStyle(this.win, content);
return (
rect.height +
topGapPlusPadding +
top +
parseInt(style.marginTop, 10) +
rect.height +
childMarginBottom +
parseInt(style.marginBottom, 10)
);
}
Expand Down
6 changes: 6 additions & 0 deletions tools/experiments/experiments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,12 @@ const EXPERIMENTS = [
spec: 'https://github.com/ampproject/amphtml/issues/8929',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/22177',
},
{
id: 'margin-bottom-in-content-height',
name: 'Fixes smaller-than-expected "documentHeight" on Safari.',
spec: 'https://github.com/ampproject/amphtml/issues/22718',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/22749',
},
];

if (getMode().localDev) {
Expand Down