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.

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

First pass at IntersectionObserver in Resources #26236

Merged
merged 50 commits into from
Mar 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
8a46ce0
Rebase IO prototype with examples.
Nov 15, 2019
d0c1840
Support unload, fix build/measure/layout order.
Dec 27, 2019
1e8c67a
Uncomment and gate discoverWork_().
Dec 27, 2019
c5e5d1a
Uncomment and gate relayoutTop and remeasurePass.
Dec 27, 2019
cfbb1f7
Uncomment and guard remaining code.
Jan 2, 2020
9cc7a92
Uncomment .
Jan 2, 2020
e495dce
Use isIframed(), simplify mutate switch.
Jan 2, 2020
b0cfb46
Update TODOs.
Jan 2, 2020
1786728
Only enable on Chrome 81+.
Jan 2, 2020
8cedc2a
Move observation to upgrade, only force-build intersecting elements.
Jan 3, 2020
0e52356
Fix unload timing, always wait for build.
Jan 3, 2020
5f6b846
Fix collapsing, delegate layout box calc to viewport binding.
Jan 3, 2020
17789b1
Remove a couple TODOs.
Jan 3, 2020
26d86dc
Correctly defer ready-scan signal, fix unobserve due to reparenting.
Jan 4, 2020
7143746
Cleanup some TODOs.
Jan 6, 2020
b80beca
Skip passes after resize (vsync) and expand.
Jan 6, 2020
27700f4
Apply sizes/media queries on viewport size change.
Jan 6, 2020
4d86b4f
Remeasure all resources on viewport size change, and support requestM…
Jan 6, 2020
bbb5736
Reenable remeasuring children after size change.
Jan 6, 2020
90262ff
Fix wrong early return.
Jan 6, 2020
27ff69e
Partial revert: don't request measure on every mutate.
Jan 6, 2020
08eea13
Add experiment and guard.
Jan 7, 2020
6c9293b
Add measure and unload helpers for DRY.
Jan 7, 2020
a31aba5
Fix lint.
Jan 7, 2020
fbb0c9a
Fix types.
Jan 7, 2020
29bccff
Merge branch 'master' into resources-io
Jan 7, 2020
b16c8f1
Various clean up.
Jan 7, 2020
5865324
Tweak Resource.measure_, fix presubmit.
Jan 7, 2020
9e7df17
Fix types again.
Jan 7, 2020
b17098c
Fix bugs, don't run pass on scroll.
Jan 8, 2020
cd371ec
Add comment about fast scrolling issue.
Jan 8, 2020
ca847a5
Fix amp-accordion collapse detection.
Jan 9, 2020
96ee39a
Accordion fix is just an animation tradeoff; adjust comments.
Jan 9, 2020
7dee29e
Add comment about unlayout.
Jan 10, 2020
3f5e69c
Revert idempotency of Resource.measure_.
Jan 11, 2020
95ef085
Fix method signature.
Jan 11, 2020
e938211
Merge branch 'master' into resources-io
Jan 13, 2020
3e9f93c
Remove 'stress' example pages.
Jan 13, 2020
0521289
Fix test-viewport.js.
Jan 13, 2020
dbee300
Merge branch 'master' into resources-io
Feb 14, 2020
949e23e
Tweak feature detection.
Feb 17, 2020
5f3e9ae
Invert measureResource_, better comments.
Feb 17, 2020
9bfe421
Fix JSDoc.
Feb 17, 2020
9489416
Add a couple TODOs.
Feb 17, 2020
7e4f4e5
Coerce type for root.
Feb 18, 2020
0c52272
Fix typo.
Feb 18, 2020
82990f8
Need downcast->upcast to avoid JSC error.
Feb 21, 2020
1430cc3
Justin's comments.
Feb 28, 2020
98b7b29
Fix test-layout-rect.js, minor comment.
Feb 28, 2020
f23479f
s/isExperimentOn/isIntersectionExperimentOn
Mar 4, 2020
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
5 changes: 5 additions & 0 deletions src/inabox/inabox-resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ export class InaboxResources {
return this.firstPassDone_.promise;
}

/** @override */
isIntersectionExperimentOn() {
return false;
}

/**
* @private
*/
Expand Down
2 changes: 0 additions & 2 deletions src/ini-load.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ export function getMeasuredResources(ampdoc, hostWin, filterFn) {
// First, wait for the `ready-scan` signal. Waiting for each element
// individually is too expensive and `ready-scan` will cover most of
// the initially parsed elements.
// TODO(jridgewell): this path should be switched to use a future
// "layer has been measured" signal.
return ampdoc
.signals()
.whenSignal(READY_SCAN_SIGNAL)
Expand Down
8 changes: 4 additions & 4 deletions src/layout-rect.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,11 @@ export function layoutRectFromDomRect(rect) {

/**
* Returns true if the specified two rects overlap by a single pixel.
* @param {!LayoutRectDef} r1
* @param {!LayoutRectDef} r2
* @param {!LayoutRectDef|!ClientRect} r1
* @param {!LayoutRectDef|!ClientRect} r2
* @return {boolean}
*/
export function layoutRectsOverlap(r1, r2) {
export function rectsOverlap(r1, r2) {
return (
r1.top <= r2.bottom &&
r2.top <= r1.bottom &&
Expand Down Expand Up @@ -188,7 +188,7 @@ export function layoutPositionRelativeToScrolledViewport(
right: viewport.getWidth(),
})
);
if (layoutRectsOverlap(layoutBox, scrollLayoutBox)) {
if (rectsOverlap(layoutBox, scrollLayoutBox)) {
return RelativePositions.INSIDE;
} else {
return layoutRectsRelativePos(layoutBox, scrollLayoutBox);
Expand Down
56 changes: 45 additions & 11 deletions src/service/mutator-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {Services} from '../services';
import {areMarginsChanged} from '../layout-rect';
import {closest} from '../dom';
import {computedStyle} from '../style';
import {dev} from '../log';
import {dev, devAssert} from '../log';
import {isExperimentOn} from '../experiments';
import {registerServiceBuilderForDoc} from '../service';

Expand Down Expand Up @@ -58,6 +58,9 @@ export class MutatorImpl {
this.activeHistory_.onFocus(element => {
this.checkPendingChangeSize_(element);
});

/** @private @const {boolean} */
this.intersect_ = this.resources_.isIntersectionExperimentOn();
}

/** @override */
Expand Down Expand Up @@ -132,17 +135,27 @@ export class MutatorImpl {

/** @override */
collapseElement(element) {
const box = this.viewport_.getLayoutRect(element);
const resource = Resource.forElement(element);
if (box.width != 0 && box.height != 0) {
if (isExperimentOn(this.win, 'dirty-collapse-element')) {
this.dirtyElement(element);
} else {
this.resources_.setRelayoutTop(box.top);
// With IntersectionObserver, no need to relayout or remeasure
// due to a single element collapse (similar to "relayout top").
if (!this.intersect_) {
const box = this.viewport_.getLayoutRect(element);
if (box.width != 0 && box.height != 0) {
if (isExperimentOn(this.win, 'dirty-collapse-element')) {
this.dirtyElement(element);
} else {
this.resources_.setRelayoutTop(box.top);
}
}
}

const resource = Resource.forElement(element);
resource.completeCollapse();
this.resources_.schedulePass(FOUR_FRAME_DELAY_);

// Unlike completeExpand(), there's no requestMeasure() call here that
// requires another pass (with IntersectionObserver).
if (!this.intersect_) {
this.resources_.schedulePass(FOUR_FRAME_DELAY_);
}
}

/** @override */
Expand Down Expand Up @@ -199,11 +212,28 @@ export class MutatorImpl {
if (measurer) {
measurer();
}
relayoutTop = calcRelayoutTop();
// With IntersectionObserver, "relayout top" is no longer needed since
// relative positional changes won't affect correctness.
if (!this.intersect_) {
relayoutTop = calcRelayoutTop();
}
},
mutate: () => {
mutator();

// TODO(willchou): IntersectionObserver won't catch size changes,
// which means layout boxes may be stale. However, always requesting
// measure after any mutation is overkill and probably expensive.
// Instead, survey measureMutateElement() callers to determine which
// should explicitly call requestMeasure() to fix this.

// With IntersectionObserver, no need to remeasure and set relayout
// on element size changes since enter/exit viewport will be detected.
if (this.intersect_) {
this.resources_.maybeHeightChanged();
dreamofabear marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (element.classList.contains('i-amphtml-element')) {
const r = Resource.forElement(element);
r.requestMeasure();
Expand Down Expand Up @@ -245,6 +275,7 @@ export class MutatorImpl {
* @param {!Element} element
*/
dirtyElement(element) {
devAssert(!this.intersect_);
let relayoutAll = false;
const isAmpElement = element.classList.contains('i-amphtml-element');
if (isAmpElement) {
Expand Down Expand Up @@ -402,7 +433,10 @@ export class MutatorImpl {
callback: opt_callback,
}
);
this.resources_.schedulePassVsync();
// With IntersectionObserver, remeasuring after size changes are no longer needed.
if (!this.intersect_) {
this.resources_.schedulePassVsync();
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/service/position-observer/position-observer-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import {devAssert} from '../../log';
import {
layoutRectEquals,
layoutRectLtwh,
layoutRectsOverlap,
layoutRectsRelativePos,
rectsOverlap,
} from '../../layout-rect';

/** @enum {number} */
Expand Down Expand Up @@ -103,7 +103,7 @@ export class PositionObserverWorker {
position.viewportRect
);

if (layoutRectsOverlap(positionRect, position.viewportRect)) {
if (rectsOverlap(positionRect, position.viewportRect)) {
// Update position
this.prevPosition_ = position;
// Only call handler if entry element overlap with viewport.
Expand Down
40 changes: 23 additions & 17 deletions src/service/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import {isBlockedByConsent} from '../error';
import {
layoutRectLtwh,
layoutRectSizeEquals,
layoutRectsOverlap,
moveLayoutRect,
rectsOverlap,
} from '../layout-rect';
import {startsWith} from '../string';
import {toWin} from '../types';
Expand Down Expand Up @@ -416,8 +416,10 @@ export class Resource {
/**
* Measures the resource's boundaries. An upgraded element will be
* transitioned to the "ready for layout" state.
* @param {!ClientRect=} opt_premeasuredRect If provided, use this
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ClientRect a thing? I know of DOMRect returned by the getBoundingClientRect()...

Copy link
Author

Choose a reason for hiding this comment

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

It is! :) We already use it in a few places. The type of boundingClientRect is surprisingly just:

{
  bottom: number,
  height: number,
  left: number,
  right: number,
  top: number,
  width: number
}

* premeasured ClientRect instead of calling getBoundingClientRect.
*/
measure() {
measure(opt_premeasuredRect) {
// Check if the element is ready to be measured.
// Placeholders are special. They are technically "owned" by parent AMP
// elements, sized by parents, but laid out independently. This means
Expand Down Expand Up @@ -448,14 +450,14 @@ export class Resource {
this.isMeasureRequested_ = false;

const oldBox = this.layoutBox_;
this.measureViaResources_();
const box = this.layoutBox_;
this.computeMeasurements_(opt_premeasuredRect);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a real cost for asking for BCR twice? Also, I'm not sure this will always work correctly. InOb is not the same thing as ResizeObserver. InOb would legitimately skip most of resize events if they do not change intersection. I think this issue comes up here because we've been using measure(), requestMeasure(), mutateElement() and the rest of our infra to do all three: resize, position, and in-viewport monitoring. Separating them at this point would be difficult.

Alternative approach could be do something like this:

  1. Separate layoutBox into position and size.
  2. Remove calls to getLayoutBox() API and instead use an asynchronous API of some kind to either provide measurements or intersection.
  3. Remove measureCallback() APIs and replace them with async versions, that are cleaner map to resize observer concept.
  4. Remove inViewport() callback and replace with async API better mapped to InOb.

Copy link
Author

Choose a reason for hiding this comment

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

Great points. Totally agree on the "stale layout box" issue due to resizing and that updating our APIs is important for dev ergonomics as we migrate.

Separate layoutBox into position and size.

I like it. E.g. we only need position for resource scheduling and size for mutation (changeSize). I wonder if we can do away with caching measurements in general, long-term -- "if you need position/size, just use vsync".

Added your suggestions to the tracking issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd definitely support doing way with caching. The only reason it truly existed is to avoid measurements during scroll.

const newBox = this.layoutBox_;

// Note that "left" doesn't affect readiness for the layout.
const sizeChanges = !layoutRectSizeEquals(oldBox, box);
const sizeChanges = !layoutRectSizeEquals(oldBox, newBox);
if (
this.state_ == ResourceState.NOT_LAID_OUT ||
oldBox.top != box.top ||
oldBox.top != newBox.top ||
sizeChanges
) {
if (
Expand All @@ -469,17 +471,20 @@ export class Resource {
}

if (!this.hasBeenMeasured()) {
this.initialLayoutBox_ = box;
this.initialLayoutBox_ = newBox;
}

this.element.updateLayoutBox(box, sizeChanges);
this.element.updateLayoutBox(newBox, sizeChanges);
}

/** Use resources for measurement */
measureViaResources_() {
/**
* Computes the current layout box and position-fixed state of the element.
* @param {!ClientRect=} opt_premeasuredRect
* @private
*/
computeMeasurements_(opt_premeasuredRect) {
const viewport = Services.viewportForDoc(this.element);
const box = viewport.getLayoutRect(this.element);
this.layoutBox_ = box;
this.layoutBox_ = viewport.getLayoutRect(this.element, opt_premeasuredRect);

// Calculate whether the element is currently is or in `position:fixed`.
let isFixed = false;
Expand Down Expand Up @@ -507,7 +512,7 @@ export class Resource {
// viewport. When accessing the layoutBox through #getLayoutBox, we'll
// return the new absolute position.
this.layoutBox_ = moveLayoutRect(
box,
this.layoutBox_,
-viewport.getScrollLeft(),
-viewport.getScrollTop()
);
Expand Down Expand Up @@ -621,12 +626,13 @@ export class Resource {
/**
* Whether the resource is displayed, i.e. if it has non-zero width and
* height.
* @param {!ClientRect=} opt_premeasuredRect If provided, use this
* premeasured ClientRect instead of using the cached layout box.
* @return {boolean}
*/
isDisplayed() {
isDisplayed(opt_premeasuredRect) {
const isFluid = this.element.getLayout() == Layout.FLUID;
// TODO(jridgewell): #getSize
const box = this.getLayoutBox();
const box = opt_premeasuredRect || this.getLayoutBox();
const hasNonZeroSize = box.height > 0 && box.width > 0;
return (
(isFluid || hasNonZeroSize) &&
Expand All @@ -649,7 +655,7 @@ export class Resource {
* @return {boolean}
*/
overlaps(rect) {
return layoutRectsOverlap(this.getLayoutBox(), rect);
return rectsOverlap(this.getLayoutBox(), rect);
}

/**
Expand Down
Loading