From 1d4c80a1b7cc01b8d00feabd6c77f7b4670191d4 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 28 Dec 2020 11:09:29 -0800 Subject: [PATCH 1/2] Remove getPageLayoutBox, getLayoutBox, and getIntersectionChangeEntry APIs from docking --- .../0.1/amp-video-docking.js | 129 +++++++++++------- extensions/amp-video-docking/0.1/math.js | 3 +- .../0.1/test/test-amp-video-docking.js | 62 +++++---- 3 files changed, 119 insertions(+), 75 deletions(-) diff --git a/extensions/amp-video-docking/0.1/amp-video-docking.js b/extensions/amp-video-docking/0.1/amp-video-docking.js index bf266e97b9f3..84258b290ea8 100644 --- a/extensions/amp-video-docking/0.1/amp-video-docking.js +++ b/extensions/amp-video-docking/0.1/amp-video-docking.js @@ -21,7 +21,8 @@ import {HtmlLiteralTagDef} from './html'; import { LayoutRectDef, layoutRectEquals, - moveLayoutRect, + layoutRectFromDomRect, + rectIntersection, } from '../../../src/layout-rect'; import { PlayingStates, @@ -140,6 +141,7 @@ let DockedDef; * @struct @typedef {{ * type: DockTargetType, * rect: !LayoutRectDef, + * slot: (!Element|undefined), * }} */ let DockTargetDef; @@ -450,7 +452,7 @@ export class VideoDocking { const slot = this.getSlot_(); if (slot) { // Match slot's top edge to tie transition to element. - return slot.getPageLayoutBox().top; + return slot.getBoundingClientRect().top; } return 0; } @@ -461,8 +463,8 @@ export class VideoDocking { const {element} = video; const fidelity = PositionObserverFidelity.HIGH; - this.getPositionObserver_().observe(element, fidelity, () => - this.updateOnPositionChange_(video) + this.getPositionObserver_().observe(element, fidelity, ({positionRect}) => + this.updateOnPositionChange_(video, positionRect) ); this.observed_.push(video); } @@ -594,8 +596,9 @@ export class VideoDocking { } const {element} = video; - const {intersectionRect} = element.getIntersectionChangeEntry(); - if (!isSizedRect(intersectionRect)) { + const clientRect = element.getBoundingClientRect(); + const intersectionRect = rectIntersection(clientRect, this.viewportRect_); + if (!intersectionRect || !isSizedRect(intersectionRect)) { return null; } if (intersectionRect.top > this.getTopBoundary_()) { @@ -610,8 +613,8 @@ export class VideoDocking { * @private */ getScrollAdjustedRect_(element) { - const dy = -this.viewport_.getScrollTop(); - return moveLayoutRect(element.getLayoutBox(), /* dx */ 0, dy); + element = element.element || element; + return layoutRectFromDomRect(element.getBoundingClientRect()); } /** @@ -643,16 +646,17 @@ export class VideoDocking { /** * @param {!VideoOrBaseElementDef} video + * @param {!LayoutRectDef=} opt_clientRect * @private */ - updateOnPositionChange_(video) { + updateOnPositionChange_(video, opt_clientRect) { if (this.isTransitioning_) { return; } if (this.scrollDirection_ == DirectionY.TOP) { const target = this.getTargetFor_(video); if (target) { - this.dockOnPositionChange_(video, target); + this.dockOnPositionChange_(video, target, opt_clientRect); } } else if (this.scrollDirection_ == DirectionY.BOTTOM) { if (!this.currentlyDocked_) { @@ -687,7 +691,7 @@ export class VideoDocking { * @private */ isValidSize_(video) { - const {width, height} = video.getLayoutBox(); + const {width, height} = video.element.getBoundingClientRect(); if (width / height < 1 - FLOAT_TOLERANCE) { complainAboutPortrait(video.element); return false; @@ -714,9 +718,10 @@ export class VideoDocking { /** * @param {!VideoOrBaseElementDef} video * @param {!DockTargetDef} target + * @param {!LayoutRectDef=} opt_clientRect * @private */ - dockOnPositionChange_(video, target) { + dockOnPositionChange_(video, target, opt_clientRect) { if (this.ignoreDueToDismissal_(video)) { return; } @@ -725,43 +730,46 @@ export class VideoDocking { return; } - const currentlyDocked = this.currentlyDocked_; - - if ( - currentlyDocked && - currentlyDocked.step >= 0 && - layoutRectEquals(currentlyDocked.target.rect, target.rect) - ) { - return; - } - - this.dockInTransferLayerStep_(video, target); + this.dockInTransferLayerStep_( + video, + target, + /* step */ 0.1, + opt_clientRect + ); } /** * @param {!VideoOrBaseElementDef} video * @param {!DockTargetDef} target - * @param {number=} opt_step + * @param {number} step + * @param {!LayoutRectDef=} opt_clientRect * @return {!Promise} * @private */ - dockInTransferLayerStep_(video, target, opt_step) { + dockInTransferLayerStep_(video, target, step, opt_clientRect) { // Do this in a multi-step process due to a browser quirk in transferring // layers to GPU. - const step = opt_step || 0.1; // This cutoff is arbitrary and may be dependant on performance. - // TODO(alanorozco): Investigate. if (step > 0.3) { - return this.dock_(video, target, /* step */ 1); + return this.dock_(video, target, /* step */ 1, opt_clientRect); } const isTransferLayerStep = true; - return this.dock_(video, target, step, isTransferLayerStep).then( + return this.dock_( + video, + target, + step, + opt_clientRect, + isTransferLayerStep + ).then( () => new Promise((resolve) => { this.raf_(() => { - this.dockInTransferLayerStep_(video, target, step + 0.1).then( - resolve - ); + this.dockInTransferLayerStep_( + video, + target, + step + 0.1, + opt_clientRect + ).then(resolve); }); }) ); @@ -771,11 +779,12 @@ export class VideoDocking { * @param {!VideoOrBaseElementDef} video * @param {!DockTargetDef} target * @param {number} step + * @param {!LayoutRectDef=} opt_clientRect * @param {boolean=} opt_isTransferLayerStep * @return {!Promise} * @private */ - dock_(video, target, step, opt_isTransferLayerStep) { + dock_(video, target, step, opt_clientRect, opt_isTransferLayerStep) { const {element} = video; // Component background is now visible, so hide the poster for the Android @@ -784,7 +793,12 @@ export class VideoDocking { // TODO(alanorozco): Make docking agnostic to this workaround. this.removePosterForAndroidBug_(element); - const {x, y, scale, relativeX} = this.getDims_(video, target, step); + const {x, y, scale, relativeX} = this.getDims_( + video, + target, + step, + opt_clientRect + ); video.hideControls(); @@ -799,7 +813,8 @@ export class VideoDocking { scale, step, transitionDurationMs, - relativeX + relativeX, + opt_clientRect ).then(() => { if (opt_isTransferLayerStep) { // Do not enable controls during transfer layer steps, which would @@ -812,11 +827,14 @@ export class VideoDocking { /** * @param {!Actions} action + * @param {!DockTargetDef=} opt_target * @private */ - trigger_(action) { + trigger_(action, opt_target) { const element = dev().assertElement( - this.getSlot_() || this.getDockedVideo_().element + (opt_target && opt_target.slot) || + this.getSlot_() || + this.getDockedVideo_().element ); const trust = ActionTrust.LOW; @@ -908,24 +926,34 @@ export class VideoDocking { * @param {number} step in [0..1] * @param {number} transitionDurationMs * @param {DirectionX=} opt_relativeX + * @param {!LayoutRectDef=} opt_clientRect * @return {!Promise} * @private */ - placeAt_(video, x, y, scale, step, transitionDurationMs, opt_relativeX) { + placeAt_( + video, + x, + y, + scale, + step, + transitionDurationMs, + opt_relativeX, + opt_clientRect + ) { if (this.alreadyPlacedAt_(x, y, scale)) { return Promise.resolve(); } this.isTransitioning_ = true; - const {width, height} = video.getLayoutBox(); + const {element} = video; + const {width, height} = + opt_clientRect || video.element.getBoundingClientRect(); this.placedAt_ = {x, y, scale}; const transitionTiming = step > 0 ? 'ease-out' : 'ease-in'; - const {element} = video; - const internalElement = getInternalVideoElementFor(element); const shadowLayer = this.getShadowLayer_(); const {overlay} = this.getControls_(); @@ -1171,7 +1199,7 @@ export class VideoDocking { return; } this.getControls_().setVideo(video, target.rect); - this.trigger_(Actions.DOCK); + this.trigger_(Actions.DOCK, target); } /** @@ -1472,7 +1500,7 @@ export class VideoDocking { getCenterX_(offsetX) { const {target, step} = this.currentlyDocked_; const video = this.getDockedVideo_(); - const {width} = video.getLayoutBox(); + const {width} = video.element.getBoundingClientRect(); const {x, scale} = this.getDims_(video, target, step); return x + offsetX + (width * scale) / 2; } @@ -1520,11 +1548,12 @@ export class VideoDocking { * @param {!VideoOrBaseElementDef} video * @param {!DockTargetDef} target * @param {number} step in [0..1] + * @param {!LayoutRectDef} opt_clientRect * @return {{x: number, y: number, scale: number, relativeX: !DirectionX}} */ - getDims_(video, target, step) { + getDims_(video, target, step, opt_clientRect) { return interpolatedBoxesTransform( - this.getScrollAdjustedRect_(video), + opt_clientRect || this.getScrollAdjustedRect_(video), target.rect, step ); @@ -1672,12 +1701,18 @@ export class VideoDocking { */ getUsableTarget_(video) { const slot = this.getSlot_(); - const inlineRect = video.getLayoutBox(); + const inlineRect = layoutRectFromDomRect( + video.element.getBoundingClientRect() + ); if (slot) { return { type: DockTargetType.SLOT, - rect: letterboxRect(inlineRect, slot.getPageLayoutBox()), + rect: letterboxRect( + inlineRect, + layoutRectFromDomRect(slot.getBoundingClientRect()) + ), + slot, }; } diff --git a/extensions/amp-video-docking/0.1/math.js b/extensions/amp-video-docking/0.1/math.js index df2b44585d4b..178075f1955e 100644 --- a/extensions/amp-video-docking/0.1/math.js +++ b/extensions/amp-video-docking/0.1/math.js @@ -162,7 +162,8 @@ export function topCornerRect( * @param {!AmpElement} element * @return {boolean} */ -export const isVisibleBySize = (element) => isSizedRect(element.getLayoutBox()); +export const isVisibleBySize = (element) => + isSizedRect(element.getBoundingClientRect()); /** * @param {!LayoutRectDef|!ClientRect|!DOMRect} rect diff --git a/extensions/amp-video-docking/0.1/test/test-amp-video-docking.js b/extensions/amp-video-docking/0.1/test/test-amp-video-docking.js index 632f07eaf3f0..efeea96dd976 100644 --- a/extensions/amp-video-docking/0.1/test/test-amp-video-docking.js +++ b/extensions/amp-video-docking/0.1/test/test-amp-video-docking.js @@ -38,8 +38,6 @@ import {createElementWithAttributes} from '../../../../src/dom'; import {htmlFor} from '../../../../src/static-template'; import {layoutRectLtwh} from '../../../../src/layout-rect'; -const noop = () => {}; - const slotId = 'my-slot-element'; describes.realWin('video docking', {amp: true}, (env) => { @@ -56,16 +54,13 @@ describes.realWin('video docking', {amp: true}, (env) => { function createAmpElementMock(tag = 'div', attrs = {}) { const element = createElementWithAttributes(env.win.document, tag, attrs); const defaultLayoutRect = layoutRectLtwh(0, 0, 0, 0); - Object.assign(element, { - getIntersectionChangeEntry: noop, - getLayoutBox: () => defaultLayoutRect, - }); - return { + const impl = { element, - getLayoutBox: () => defaultLayoutRect, mutateElement: (cb) => tryResolve(cb), applyFillContent: env.sandbox.spy(), }; + stubLayoutBox(impl, defaultLayoutRect); + return impl; } function createVideo() { @@ -93,13 +88,14 @@ describes.realWin('video docking', {amp: true}, (env) => { } function stubLayoutBox(impl, rect) { - impl.getLayoutBox = () => rect; - impl.getPageLayoutBox = impl.getLayoutBox; - impl.element.getLayoutBox = impl.getLayoutBox; - impl.element.getPageLayoutBox = impl.getLayoutBox; - impl.element.getIntersectionChangeEntry = () => ({ - intersectionRect: rect, - }); + let stub = impl.element.__stubBoundingClientRect; + if (!stub) { + stub = impl.element.__stubBoundingClientRect = env.sandbox.stub( + impl.element, + 'getBoundingClientRect' + ); + } + stub.returns(rect); } function setValidAreaWidth() { @@ -295,8 +291,8 @@ describes.realWin('video docking', {amp: true}, (env) => { enableComputedStyle(video.element); enableComputedStyle(overlay); - const width = 400; - const height = 300; + const expectedWidth = 400; + const expectedHeight = 300; const x = 30; const y = 60; @@ -304,7 +300,7 @@ describes.realWin('video docking', {amp: true}, (env) => { const step = 1; const transitionDurationMs = 200; - placeElementLtwh(video, 0, 0, width, height); + placeElementLtwh(video, 0, 0, expectedWidth, expectedHeight); await docking.placeAt_(video, x, y, scale, step, transitionDurationMs); @@ -313,13 +309,18 @@ describes.realWin('video docking', {amp: true}, (env) => { const expectedTransform = transformMatrix(x, y, scale); [internalElement, overlay, shadow].forEach((el) => { - expect(getComputedStyle(el)).to.include({ - 'transform': expectedTransform, - 'width': width + 'px', - 'min-width': width + 'px', - 'height': height + 'px', - 'min-height': height + 'px', - }); + const { + transform, + width, + height, + minWidth, + minHeight, + } = getComputedStyle(el); + expect(transform).to.equal(expectedTransform); + expect(width).to.equal(expectedWidth + 'px'); + expect(minWidth).to.equal(expectedWidth + 'px'); + expect(height).to.equal(expectedHeight + 'px'); + expect(minHeight).to.equal(expectedHeight + 'px'); }); }); @@ -900,7 +901,7 @@ describes.realWin('video docking', {amp: true}, (env) => { const {x, y, scale} = docking.getDims_(video, target, step); expect(x, 'x').to.equal(videoX); - expect(y, 'y').to.equal(videoY - scrollTop); + expect(y, 'y').to.equal(videoY); expect(scale, 'scale').to.equal(1); }); @@ -1018,7 +1019,13 @@ describes.realWin('video docking', {amp: true}, (env) => { it('does not enable docked controls if transferring layer', async () => { const {enable} = stubControls(); - await docking.dock_(video, target, step, /* isTransferLayerStep */ true); + await docking.dock_( + video, + target, + step, + /* clientRect */ undefined, + /* isTransferLayerStep */ true + ); expect(enable).to.not.have.been.called; }); @@ -1524,6 +1531,7 @@ describes.realWin('video docking', {amp: true}, (env) => { video, target: { type: useSlot ? DockTargetType.SLOT : DockTargetType.CORNER, + slot: useSlot ? slot : undefined, }, }; From ed091b5000f2f549302971caad9e306dc6ca7935 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 28 Dec 2020 13:25:55 -0800 Subject: [PATCH 2/2] lints --- build-system/tasks/presubmit-checks.js | 1 - .../amp-video-docking/0.1/amp-video-docking.js | 16 ++++++++-------- extensions/amp-video-docking/0.1/math.js | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 82430f13c604..f3226fb5802c 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -1205,7 +1205,6 @@ const forbiddenTermsSrcInclusive = { 'src/base-element.js', 'src/custom-element.js', 'src/service/resource.js', - 'extensions/amp-video-docking/0.1/amp-video-docking.js', ], }, '\\.getIntersectionElementLayoutBox': { diff --git a/extensions/amp-video-docking/0.1/amp-video-docking.js b/extensions/amp-video-docking/0.1/amp-video-docking.js index 84258b290ea8..9e09f0150f2a 100644 --- a/extensions/amp-video-docking/0.1/amp-video-docking.js +++ b/extensions/amp-video-docking/0.1/amp-video-docking.js @@ -452,7 +452,7 @@ export class VideoDocking { const slot = this.getSlot_(); if (slot) { // Match slot's top edge to tie transition to element. - return slot.getBoundingClientRect().top; + return slot./*OK*/ getBoundingClientRect().top; } return 0; } @@ -596,7 +596,7 @@ export class VideoDocking { } const {element} = video; - const clientRect = element.getBoundingClientRect(); + const clientRect = element./*OK*/ getBoundingClientRect(); const intersectionRect = rectIntersection(clientRect, this.viewportRect_); if (!intersectionRect || !isSizedRect(intersectionRect)) { return null; @@ -614,7 +614,7 @@ export class VideoDocking { */ getScrollAdjustedRect_(element) { element = element.element || element; - return layoutRectFromDomRect(element.getBoundingClientRect()); + return layoutRectFromDomRect(element./*OK*/ getBoundingClientRect()); } /** @@ -691,7 +691,7 @@ export class VideoDocking { * @private */ isValidSize_(video) { - const {width, height} = video.element.getBoundingClientRect(); + const {width, height} = video.element./*OK*/ getBoundingClientRect(); if (width / height < 1 - FLOAT_TOLERANCE) { complainAboutPortrait(video.element); return false; @@ -948,7 +948,7 @@ export class VideoDocking { const {element} = video; const {width, height} = - opt_clientRect || video.element.getBoundingClientRect(); + opt_clientRect || video.element./*OK*/ getBoundingClientRect(); this.placedAt_ = {x, y, scale}; @@ -1500,7 +1500,7 @@ export class VideoDocking { getCenterX_(offsetX) { const {target, step} = this.currentlyDocked_; const video = this.getDockedVideo_(); - const {width} = video.element.getBoundingClientRect(); + const {width} = video.element./*OK*/ getBoundingClientRect(); const {x, scale} = this.getDims_(video, target, step); return x + offsetX + (width * scale) / 2; } @@ -1702,7 +1702,7 @@ export class VideoDocking { getUsableTarget_(video) { const slot = this.getSlot_(); const inlineRect = layoutRectFromDomRect( - video.element.getBoundingClientRect() + video.element./*OK*/ getBoundingClientRect() ); if (slot) { @@ -1710,7 +1710,7 @@ export class VideoDocking { type: DockTargetType.SLOT, rect: letterboxRect( inlineRect, - layoutRectFromDomRect(slot.getBoundingClientRect()) + layoutRectFromDomRect(slot./*OK*/ getBoundingClientRect()) ), slot, }; diff --git a/extensions/amp-video-docking/0.1/math.js b/extensions/amp-video-docking/0.1/math.js index 178075f1955e..e38f6775cde4 100644 --- a/extensions/amp-video-docking/0.1/math.js +++ b/extensions/amp-video-docking/0.1/math.js @@ -163,7 +163,7 @@ export function topCornerRect( * @return {boolean} */ export const isVisibleBySize = (element) => - isSizedRect(element.getBoundingClientRect()); + isSizedRect(element./*OK*/ getBoundingClientRect()); /** * @param {!LayoutRectDef|!ClientRect|!DOMRect} rect