From e532fe672ff09b655a401a8285be3e2f6c230694 Mon Sep 17 00:00:00 2001 From: William Chou Date: Tue, 4 Jun 2019 19:02:10 -0400 Subject: [PATCH 1/6] Include bottom margin in getContentHeight. --- .../viewport-binding-ios-embed-wrapper.js | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/src/service/viewport/viewport-binding-ios-embed-wrapper.js b/src/service/viewport/viewport-binding-ios-embed-wrapper.js index 29b3005ee7e8..cdecbbdf0fc3 100644 --- a/src/service/viewport/viewport-binding-ios-embed-wrapper.js +++ b/src/service/viewport/viewport-binding-ios-embed-wrapper.js @@ -246,19 +246,41 @@ 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 {body} = this.win.document; + const {height} = body./*OK*/ getBoundingClientRect(); + + // Unlike viewport-binding-natural.js, there's no need to calculate the + // "top gap" since the wrapped body accounts for the top margin of children. + // However, the parent's paddingTop and "bottom gap" must be added... + + // Bottom gap: + // As of Safari 12.1.1, the wrapped body's rect height does not include the + // bottom margin of children and there's no other API that does. + // 1. Find the last child that has a non-zero height. + // 2. Add its marginBottom to the height calculation. + let bottomGap = 0; + if (isExperimentOn(this.win, 'bottom-margin-in-content-height')) { + let n = body.lastElementChild; + while (n) { + const r = n./*OK*/ getBoundingClientRect(); + if (r.height > 0) { + break; + } else { + n = n.previousElementSibling; + } + } + if (n) { + bottomGap = parseInt(computedStyle(this.win, n).marginBottom, 10); + } + } + + const style = computedStyle(this.win, body); return ( - rect.height + + height + this.paddingTop_ + parseInt(style.marginTop, 10) + + bottomGap + parseInt(style.marginBottom, 10) ); } From 2f20ac7a0e7e981dd213e2f2bc50050b993d68cc Mon Sep 17 00:00:00 2001 From: William Chou Date: Tue, 4 Jun 2019 19:28:45 -0400 Subject: [PATCH 2/6] Support other viewport bindings. --- .../viewport/viewport-binding-ios-embed-sd.js | 28 ++++++++++++++---- .../viewport-binding-ios-embed-wrapper.js | 8 ++--- .../viewport/viewport-binding-natural.js | 29 +++++++++++++++++-- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/src/service/viewport/viewport-binding-ios-embed-sd.js b/src/service/viewport/viewport-binding-ios-embed-sd.js index a07e9499cf54..9cbcb4463616 100644 --- a/src/service/viewport/viewport-binding-ios-embed-sd.js +++ b/src/service/viewport/viewport-binding-ios-embed-sd.js @@ -400,20 +400,38 @@ 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); + const content = this.wrapper_; + const rect = content./*OK*/ getBoundingClientRect(); + // The Y-position of any element 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. + // on ancestor elements and the scroller's border-top. const topGapPlusPaddingAndBorder = rect.top + this.getScrollTop(); + + let bottomGap = 0; + if (isExperimentOn(this.win, 'bottom-margin-in-content-height')) { + let n = this.win.document.body.lastElementChild; + while (n) { + const r = n./*OK*/ getBoundingClientRect(); + if (r.height > 0) { + break; + } else { + n = n.previousElementSibling; + } + } + if (n) { + bottomGap = parseInt(computedStyle(this.win, n).marginBottom, 10); + } + } + + const style = computedStyle(this.win, content); return ( rect.height + topGapPlusPaddingAndBorder + parseInt(style.marginTop, 10) + + bottomGap + 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 cdecbbdf0fc3..7487a0200669 100644 --- a/src/service/viewport/viewport-binding-ios-embed-wrapper.js +++ b/src/service/viewport/viewport-binding-ios-embed-wrapper.js @@ -247,8 +247,8 @@ export class ViewportBindingIosEmbedWrapper_ { /** @override */ getContentHeight() { // The wrapped body, not this.wrapper_ itself, will have the correct height. - const {body} = this.win.document; - const {height} = body./*OK*/ getBoundingClientRect(); + const content = this.win.document.body; + const {height} = content./*OK*/ getBoundingClientRect(); // Unlike viewport-binding-natural.js, there's no need to calculate the // "top gap" since the wrapped body accounts for the top margin of children. @@ -261,7 +261,7 @@ export class ViewportBindingIosEmbedWrapper_ { // 2. Add its marginBottom to the height calculation. let bottomGap = 0; if (isExperimentOn(this.win, 'bottom-margin-in-content-height')) { - let n = body.lastElementChild; + let n = content.lastElementChild; while (n) { const r = n./*OK*/ getBoundingClientRect(); if (r.height > 0) { @@ -275,7 +275,7 @@ export class ViewportBindingIosEmbedWrapper_ { } } - const style = computedStyle(this.win, body); + const style = computedStyle(this.win, content); return ( height + this.paddingTop_ + diff --git a/src/service/viewport/viewport-binding-natural.js b/src/service/viewport/viewport-binding-natural.js index a630ef6cfdbd..4d94d5676b8e 100644 --- a/src/service/viewport/viewport-binding-natural.js +++ b/src/service/viewport/viewport-binding-natural.js @@ -205,19 +205,42 @@ 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); + const content = this.getScrollingElement(); + const rect = content./*OK*/ getBoundingClientRect(); + const style = computedStyle(this.win, content); // The Y-position of any element 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(); + + // Bottom gap: + // As of Safari 12.1.1, the wrapped body's rect height does not include the + // bottom margin of children and there's no other API that does. + // 1. Find the last child that has a non-zero height. + // 2. Add its marginBottom to the height calculation. + let bottomGap = 0; + if (isExperimentOn(this.win, 'bottom-margin-in-content-height')) { + let n = content.lastElementChild; + while (n) { + const r = n./*OK*/ getBoundingClientRect(); + if (r.height > 0) { + break; + } else { + n = n.previousElementSibling; + } + } + if (n) { + bottomGap = parseInt(computedStyle(this.win, n).marginBottom, 10); + } + } + return ( rect.height + topGapPlusPadding + parseInt(style.marginTop, 10) + + bottomGap + parseInt(style.marginBottom, 10) ); } From d7aa84f34d062d01b66d2fb71ee713d93e3688f3 Mon Sep 17 00:00:00 2001 From: William Chou Date: Wed, 5 Jun 2019 20:52:32 -0400 Subject: [PATCH 3/6] Dedupe code with helper function. --- src/service/viewport/viewport-binding-def.js | 30 +++++++++++++++++++ .../viewport/viewport-binding-ios-embed-sd.js | 22 ++++---------- .../viewport-binding-ios-embed-wrapper.js | 27 ++++------------- .../viewport/viewport-binding-natural.js | 28 ++++------------- 4 files changed, 45 insertions(+), 62 deletions(-) diff --git a/src/service/viewport/viewport-binding-def.js b/src/service/viewport/viewport-binding-def.js index c7e72b6bf92f..2e9da6ec1eeb 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,30 @@ export class ViewportBindingDef { */ getScrollingElementScrollsLikeViewport() {} } + +/** + * Returns the margin-bottom of the last child of `element` with non-zero + * height, if any. Otherwise, returns 0. + * + * 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. + * + * @param {!Window} win + * @param {!Element} element + * @return {number} + */ +export function marginBottomOfLastChild(win, element) { + if (!isExperimentOn(win, 'bottom-margin-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 9cbcb4463616..fdd405b1c57c 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, @@ -409,22 +412,7 @@ export class ViewportBindingIosEmbedShadowRoot_ { // and add it manually. Note that the "top gap" includes any padding-top // on ancestor elements and the scroller's border-top. const topGapPlusPaddingAndBorder = rect.top + this.getScrollTop(); - - let bottomGap = 0; - if (isExperimentOn(this.win, 'bottom-margin-in-content-height')) { - let n = this.win.document.body.lastElementChild; - while (n) { - const r = n./*OK*/ getBoundingClientRect(); - if (r.height > 0) { - break; - } else { - n = n.previousElementSibling; - } - } - if (n) { - bottomGap = parseInt(computedStyle(this.win, n).marginBottom, 10); - } - } + const bottomGap = marginBottomOfLastChild(this.win, this.win.document.body); const style = computedStyle(this.win, content); return ( diff --git a/src/service/viewport/viewport-binding-ios-embed-wrapper.js b/src/service/viewport/viewport-binding-ios-embed-wrapper.js index 7487a0200669..f98ae6a4844c 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'; @@ -253,27 +256,7 @@ export class ViewportBindingIosEmbedWrapper_ { // Unlike viewport-binding-natural.js, there's no need to calculate the // "top gap" since the wrapped body accounts for the top margin of children. // However, the parent's paddingTop and "bottom gap" must be added... - - // Bottom gap: - // As of Safari 12.1.1, the wrapped body's rect height does not include the - // bottom margin of children and there's no other API that does. - // 1. Find the last child that has a non-zero height. - // 2. Add its marginBottom to the height calculation. - let bottomGap = 0; - if (isExperimentOn(this.win, 'bottom-margin-in-content-height')) { - let n = content.lastElementChild; - while (n) { - const r = n./*OK*/ getBoundingClientRect(); - if (r.height > 0) { - break; - } else { - n = n.previousElementSibling; - } - } - if (n) { - bottomGap = parseInt(computedStyle(this.win, n).marginBottom, 10); - } - } + const bottomGap = marginBottomOfLastChild(this.win, content); const style = computedStyle(this.win, content); return ( diff --git a/src/service/viewport/viewport-binding-natural.js b/src/service/viewport/viewport-binding-natural.js index 4d94d5676b8e..507c9dc43f06 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'; @@ -214,28 +217,7 @@ export class ViewportBindingNatural_ { // 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(); - - // Bottom gap: - // As of Safari 12.1.1, the wrapped body's rect height does not include the - // bottom margin of children and there's no other API that does. - // 1. Find the last child that has a non-zero height. - // 2. Add its marginBottom to the height calculation. - let bottomGap = 0; - if (isExperimentOn(this.win, 'bottom-margin-in-content-height')) { - let n = content.lastElementChild; - while (n) { - const r = n./*OK*/ getBoundingClientRect(); - if (r.height > 0) { - break; - } else { - n = n.previousElementSibling; - } - } - if (n) { - bottomGap = parseInt(computedStyle(this.win, n).marginBottom, 10); - } - } - + const bottomGap = marginBottomOfLastChild(this.win, content); return ( rect.height + topGapPlusPadding + From 19f307006bc83b630db8bc66e7f50f8d1ebe7980 Mon Sep 17 00:00:00 2001 From: William Chou Date: Wed, 5 Jun 2019 21:01:22 -0400 Subject: [PATCH 4/6] Restrict to Safari. --- src/service/viewport/viewport-binding-def.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/service/viewport/viewport-binding-def.js b/src/service/viewport/viewport-binding-def.js index 2e9da6ec1eeb..298db7640310 100644 --- a/src/service/viewport/viewport-binding-def.js +++ b/src/service/viewport/viewport-binding-def.js @@ -1,3 +1,4 @@ +import {Services} from '../../services'; import {computedStyle} from '../../style'; import {isExperimentOn} from '../../experiments'; @@ -216,7 +217,10 @@ export class ViewportBindingDef { * @return {number} */ export function marginBottomOfLastChild(win, element) { - if (!isExperimentOn(win, 'bottom-margin-in-content-height')) { + if ( + !isExperimentOn(win, 'bottom-margin-in-content-height') || + !Services.platformFor(win).isSafari() + ) { return 0; } let n = element.lastElementChild; From fb0dbe498725e1f3d9fc78d1b652452d70c9b685 Mon Sep 17 00:00:00 2001 From: William Chou Date: Thu, 6 Jun 2019 15:27:03 -0400 Subject: [PATCH 5/6] Cleaner code. --- src/service/viewport/viewport-binding-def.js | 9 +------ .../viewport/viewport-binding-ios-embed-sd.js | 24 ++++++++++------- .../viewport-binding-ios-embed-wrapper.js | 17 +++++++----- .../viewport/viewport-binding-natural.js | 27 ++++++++++++------- 4 files changed, 43 insertions(+), 34 deletions(-) diff --git a/src/service/viewport/viewport-binding-def.js b/src/service/viewport/viewport-binding-def.js index 298db7640310..d506436dde84 100644 --- a/src/service/viewport/viewport-binding-def.js +++ b/src/service/viewport/viewport-binding-def.js @@ -1,4 +1,3 @@ -import {Services} from '../../services'; import {computedStyle} from '../../style'; import {isExperimentOn} from '../../experiments'; @@ -209,18 +208,12 @@ export class ViewportBindingDef { * Returns the margin-bottom of the last child of `element` with non-zero * height, if any. Otherwise, returns 0. * - * 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. - * * @param {!Window} win * @param {!Element} element * @return {number} */ export function marginBottomOfLastChild(win, element) { - if ( - !isExperimentOn(win, 'bottom-margin-in-content-height') || - !Services.platformFor(win).isSafari() - ) { + if (!isExperimentOn(win, 'margin-bottom-in-content-height')) { return 0; } let n = element.lastElementChild; diff --git a/src/service/viewport/viewport-binding-ios-embed-sd.js b/src/service/viewport/viewport-binding-ios-embed-sd.js index fdd405b1c57c..5ea4a8f2dace 100644 --- a/src/service/viewport/viewport-binding-ios-embed-sd.js +++ b/src/service/viewport/viewport-binding-ios-embed-sd.js @@ -406,20 +406,26 @@ export class ViewportBindingIosEmbedShadowRoot_ { const content = this.wrapper_; const rect = content./*OK*/ getBoundingClientRect(); - // The Y-position of any element can be offset by the vertical margin + // 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. - const topGapPlusPaddingAndBorder = rect.top + this.getScrollTop(); - const bottomGap = marginBottomOfLastChild(this.win, this.win.document.body); + // 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) + - bottomGap + + 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 f98ae6a4844c..5e32afa2fd92 100644 --- a/src/service/viewport/viewport-binding-ios-embed-wrapper.js +++ b/src/service/viewport/viewport-binding-ios-embed-wrapper.js @@ -253,17 +253,20 @@ export class ViewportBindingIosEmbedWrapper_ { const content = this.win.document.body; const {height} = content./*OK*/ getBoundingClientRect(); - // Unlike viewport-binding-natural.js, there's no need to calculate the - // "top gap" since the wrapped body accounts for the top margin of children. - // However, the parent's paddingTop and "bottom gap" must be added... - const bottomGap = marginBottomOfLastChild(this.win, content); + // 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 ( - height + - this.paddingTop_ + parseInt(style.marginTop, 10) + - bottomGap + + 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 507c9dc43f06..6939dc063b96 100644 --- a/src/service/viewport/viewport-binding-natural.js +++ b/src/service/viewport/viewport-binding-natural.js @@ -210,19 +210,26 @@ export class ViewportBindingNatural_ { // there's no workable analog for either ios-embed-* modes. const content = this.getScrollingElement(); const rect = content./*OK*/ getBoundingClientRect(); - const style = computedStyle(this.win, content); - // The Y-position of any element can be offset by the vertical margin + + // 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(); - const bottomGap = marginBottomOfLastChild(this.win, content); + // 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) + - bottomGap + + rect.height + + childMarginBottom + parseInt(style.marginBottom, 10) ); } From 662fa44ed98dd2f88f9d774f094da615581529a2 Mon Sep 17 00:00:00 2001 From: William Chou Date: Fri, 7 Jun 2019 14:41:28 -0400 Subject: [PATCH 6/6] Add experiment description. --- tools/experiments/experiments.js | 6 ++++++ 1 file changed, 6 insertions(+) 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) {