diff --git a/src/service/viewport/viewport-binding-def.js b/src/service/viewport/viewport-binding-def.js index c7e72b6bf92f..d506436dde84 100644 --- a/src/service/viewport/viewport-binding-def.js +++ b/src/service/viewport/viewport-binding-def.js @@ -1,3 +1,6 @@ +import {computedStyle} from '../../style'; +import {isExperimentOn} from '../../experiments'; + /** * Copyright 2017 The AMP HTML Authors. All Rights Reserved. * @@ -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) { + 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; +} diff --git a/src/service/viewport/viewport-binding-ios-embed-sd.js b/src/service/viewport/viewport-binding-ios-embed-sd.js index a07e9499cf54..5ea4a8f2dace 100644 --- a/src/service/viewport/viewport-binding-ios-embed-sd.js +++ b/src/service/viewport/viewport-binding-ios-embed-sd.js @@ -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, @@ -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) ); } diff --git a/src/service/viewport/viewport-binding-ios-embed-wrapper.js b/src/service/viewport/viewport-binding-ios-embed-wrapper.js index 29b3005ee7e8..5e32afa2fd92 100644 --- a/src/service/viewport/viewport-binding-ios-embed-wrapper.js +++ b/src/service/viewport/viewport-binding-ios-embed-wrapper.js @@ -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'; @@ -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) ); } diff --git a/src/service/viewport/viewport-binding-natural.js b/src/service/viewport/viewport-binding-natural.js index a630ef6cfdbd..6939dc063b96 100644 --- a/src/service/viewport/viewport-binding-natural.js +++ b/src/service/viewport/viewport-binding-natural.js @@ -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'; @@ -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) ); } diff --git a/tools/experiments/experiments.js b/tools/experiments/experiments.js index 2544515f9e18..cf87f7db7a5a 100644 --- a/tools/experiments/experiments.js +++ b/tools/experiments/experiments.js @@ -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) {