First pass at IntersectionObserver in Resources #26236

dreamofabear · 2020-01-07T18:29:30Z

Partial for #25428.

Guarded by "intersect-resources" experiment flag. IntersectionObserver spec behavior change slated to ship in Chrome 81 in March.

Still a fair amount of TODOs to resolve e.g. dynamic loading rectangle during prerender mode, skipping intersections when scroll velocity is very high.

However, I'd like to start scaling the testing beyond myself (see below). Cost is a minor bundle size increase:

dist/amp4ads-v0.js: Δ +0.03KB
dist/shadow-v0.js: Δ +0.59KB
dist/v0.js: Δ +0.46KB

Summary

IntersectionObserver callback intersects_() replaces most of discoverWork_(), with some exceptions (e.g. idleRenderOutsideViewport is unimplemented).
Use IntersectionObserverEntry.boundingClientRect instead of Element.getBoundingClientRect() where possible.
Disable layout hacks needed for positional offsets in elements like remeasurePass() and setRelayoutTop().

Testing

Did some manual testing across several components and behaviors. Simple performance tests look roughly comparable.

Next, will write a test plan for @diantekyrie for broader extension coverage.

…easure.

jridgewell

The possibility of stale size boxes worries me.

jridgewell · 2020-02-27T21:50:13Z

src/layout-rect.js

- * @param {!LayoutRectDef} r1
- * @param {!LayoutRectDef} r2
+ * @param {{top: number, bottom: number, left: number, right: number}} r1
+ * @param {{top: number, bottom: number, left: number, right: number}} r2


Yah, I agree that there's a risk of obfuscation. layoutRectFromDomRect was made for this.

src/service/mutator-impl.js

src/service/resources-impl.js

jridgewell · 2020-02-27T22:31:18Z

src/service/resources-impl.js

+
+      // Force all intersecting, non-zero-sized, non-owned elements to be built.
+      // E.g. ensures that all in-viewport elements are built in prerender mode.
+      if (


Is this from?

amphtml/src/service/resources-impl.js

Lines 1134 to 1147 in 4847c7f

if (

!r.isBuilt() &&

!r.hasOwner() &&

r.hasBeenMeasured() &&

r.isDisplayed() &&

r.overlaps(loadRect)

) {

this.buildOrScheduleBuildForResource_(

r,

/* checkForDupes */ true,

/* scheduleWhenBuilt */ undefined,

/* force */ true

);

}

jridgewell · 2020-02-27T22:32:21Z

src/service/resources-impl.js

+        !r.isBuilt() &&
+        !r.isBuilding() &&
+        isIntersecting &&
+        r.isDisplayed(clientRect) &&


Is this necessary with isIntersecting?

Yea, it's kinda confusing. isDisplayed refers to size of the element. You can have an intersecting element with zero size.

jridgewell · 2020-02-27T22:33:55Z

src/service/resources-impl.js

+
+    const toUnload = [];
+
+    const promises = entries.map(entry => {


Note that there may be later entries with more up-to-date info on the same element. We may need to work from right to left, and skip the leftmost entires that are old.

Can this actually happen in practice? I'd be surprised.

jridgewell · 2020-02-27T22:34:58Z

src/service/resources-impl.js

+      const {boundingClientRect, isIntersecting, target, rootBounds} = entry;
+      // Closure Compiler doesn't recognize boundingClientRect as a ClientRect.
+      const clientRect = /** @type {!ClientRect} */ (boundingClientRect);
+      devAssert(target.isUpgraded());


Is this a safe assertion?

Yea, we only start observing on upgrade.

jridgewell · 2020-02-27T22:36:50Z

src/service/resources-impl.js

+        const wasIntersecting = r.isInViewport();
+        let isDisplayed = this.measureResource_(r, clientRect);
+
+        if (wasIntersecting && !isIntersecting) {


Should we first refactor this so that the Resource is responsible? Eg, we just call setInViewport(isIntersecting) and the Resource can go "oh, shit, I need to build".

I like where your head's at, but I definitely disagree with "refactor first". :)

src/service/resources-impl.js

dvoytenko

LGTM on my side. But please take a look at the experiment override question.

src/inabox/inabox-resources.js

jridgewell

To give file-size approval. I'll do a full review soon.

William Chou added 26 commits November 15, 2019 12:27

Rebase IO prototype with examples.

8a46ce0

Support unload, fix build/measure/layout order.

d0c1840

Uncomment and gate discoverWork_().

1e8c67a

Uncomment and gate relayoutTop and remeasurePass.

c5e5d1a

Uncomment and guard remaining code.

cfbb1f7

Uncomment .

9cc7a92

Use isIframed(), simplify mutate switch.

e495dce

Update TODOs.

b0cfb46

Only enable on Chrome 81+.

1786728

Move observation to upgrade, only force-build intersecting elements.

8cedc2a

Fix unload timing, always wait for build.

0e52356

Fix collapsing, delegate layout box calc to viewport binding.

5f6b846

Remove a couple TODOs.

17789b1

Correctly defer ready-scan signal, fix unobserve due to reparenting.

26d86dc

Cleanup some TODOs.

7143746

Skip passes after resize (vsync) and expand.

b80beca

Apply sizes/media queries on viewport size change.

27700f4

Remeasure all resources on viewport size change, and support requestM…

4d86b4f

…easure.

Reenable remeasuring children after size change.

bbb5736

Fix wrong early return.

90262ff

Partial revert: don't request measure on every mutate.

27ff69e

Add experiment and guard.

08eea13

Add measure and unload helpers for DRY.

6c9293b

Fix lint.

a31aba5

Fix types.

fbb0c9a

Merge branch 'master' into resources-io

29bccff

googlebot added the cla: yes label Jan 7, 2020

Various clean up.

b16c8f1

amp-bundle-size bot requested a review from jridgewell January 7, 2020 19:03

Tweak Resource.measure_, fix presubmit.

5865324

William Chou added 8 commits February 14, 2020 17:20

Merge branch 'master' into resources-io

dbee300

Tweak feature detection.

949e23e

Invert measureResource_, better comments.

5f3e9ae

Fix JSDoc.

9bfe421

Add a couple TODOs.

9489416

Coerce type for root.

7e4f4e5

Fix typo.

0c52272

Need downcast->upcast to avoid JSC error.

82990f8

jridgewell reviewed Feb 27, 2020

View reviewed changes

William Chou added 2 commits February 28, 2020 16:31

Justin's comments.

1430cc3

Fix test-layout-rect.js, minor comment.

98b7b29

dvoytenko approved these changes Mar 4, 2020

View reviewed changes

src/inabox/inabox-resources.js Outdated Show resolved Hide resolved

s/isExperimentOn/isIntersectionExperimentOn

f23479f

jridgewell approved these changes Mar 9, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

dreamofabear merged commit 54c8873 into ampproject:master Mar 9, 2020

dreamofabear deleted the resources-io branch March 9, 2020 15:34

ampprojectbot added the PR use: In Beta / Experimental label Mar 13, 2020

ampprojectbot added the PR use: In Stable label Mar 17, 2020

ampprojectbot mentioned this pull request Aug 6, 2020

🚨 Error: undefined is not a constructor (evaluating 'new a.win.IntersectionObserver(b=>{this.pa=b[b.length-1].isIntersecting;Mc(this,this.pa&&a.isVisible())})') #29721

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at IntersectionObserver in Resources #26236

First pass at IntersectionObserver in Resources #26236

dreamofabear commented Jan 7, 2020 •

edited

Loading

jridgewell left a comment

jridgewell Feb 27, 2020

jridgewell Feb 27, 2020

jridgewell Feb 27, 2020

dreamofabear Feb 28, 2020

jridgewell Feb 27, 2020

dreamofabear Feb 28, 2020

jridgewell Feb 27, 2020

dreamofabear Feb 28, 2020

jridgewell Feb 27, 2020

dreamofabear Feb 28, 2020

dvoytenko left a comment

jridgewell left a comment

This comment has been minimized.

	if (
	!r.isBuilt() &&
	!r.hasOwner() &&
	r.hasBeenMeasured() &&
	r.isDisplayed() &&
	r.overlaps(loadRect)
	) {
	this.buildOrScheduleBuildForResource_(
	r,
	/* checkForDupes */ true,
	/* scheduleWhenBuilt */ undefined,
	/* force */ true
	);
	}

First pass at IntersectionObserver in Resources #26236

First pass at IntersectionObserver in Resources #26236

Conversation

dreamofabear commented Jan 7, 2020 • edited Loading

Summary

Testing

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

This comment has been minimized.

dreamofabear commented Jan 7, 2020 •

edited

Loading