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.

Sign up for GitHub

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

Jump to bottom

🐛Intersection Observer Polyfill Mutation Observer - Fixed Race Condition #19168

Merged
merged 20 commits into from Nov 16, 2018
Merged

🐛Intersection Observer Polyfill Mutation Observer - Fixed Race Condition #19168

Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7ef9930
Got a working half-broken page that we can test with
torch2424 Nov 6, 2018
ad491aa
COnfirmed the bug is that the pass isn't long enough
torch2424 Nov 6, 2018
63caf4b
Going to go home. But I learned that it doesn't fire until the child …
torch2424 Nov 6, 2018
2be1f3b
I was doing something
torch2424 Nov 6, 2018
59f561e
Fixed the polyfill completely
torch2424 Nov 6, 2018
c17cb5c
Fixed linting
torch2424 Nov 7, 2018
87ec925
Fixed all precommit checks
torch2424 Nov 7, 2018
f96fd1b
Removed the one-off fandango example
torch2424 Nov 7, 2018
0eba342
Made PR Changes
torch2424 Nov 7, 2018
d5051b9
Created an onNextPass for resources
torch2424 Nov 10, 2018
9892c8e
Fixed tests for ampdoc
torch2424 Nov 10, 2018
42a9964
Removed the original getLayoutRect
torch2424 Nov 10, 2018
c7282f8
Removed a dangling opt_calledFromMutationObserver
torch2424 Nov 10, 2018
241a4fe
Made PR Changes
torch2424 Nov 13, 2018
05f8046
Fixed refresh tests for intersection observer
torch2424 Nov 13, 2018
e75f2b5
Removed returns
torch2424 Nov 13, 2018
14fc692
Fixed some linting errors
torch2424 Nov 13, 2018
095f94f
Made PR Changes
torch2424 Nov 14, 2018
9bf91c1
Merge branch 'master' of https://github.com/torch2424/amphtml into fa…
torch2424 Nov 15, 2018
5861c3b
Fixed linting errors
torch2424 Nov 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 31 additions & 11 deletions src/intersection-observer-polyfill.js
View file
Open in desktop
Expand Up @@ -297,7 +297,7 @@ export class IntersectionObserverPolyfill {
this.viewport_ = Services.viewportForDoc(element);
this.mutationPass_ = new Pass(ampdoc.win, () => {
if (this.viewport_) {
this.tick(this.viewport_.getRect());
this.tick(this.viewport_.getRect(), undefined, true);
}
});
this.mutationObserver_ = new ampdoc.win.MutationObserver(
Expand Down Expand Up @@ -339,8 +339,9 @@ export class IntersectionObserverPolyfill {
* The iframe must be a non-scrollable iframe.
* @param {!./layout-rect.LayoutRectDef} hostViewport
* @param {./layout-rect.LayoutRectDef=} opt_iframe
* @param {boolean=} opt_calledFromMutationObserver
*/
tick(hostViewport, opt_iframe) {
tick(hostViewport, opt_iframe, opt_calledFromMutationObserver) {
if (opt_iframe) {
// If element inside an iframe. Adjust origin to the iframe.left/top.
hostViewport =
Expand All @@ -356,7 +357,11 @@ export class IntersectionObserverPolyfill {

for (let i = 0; i < this.observeEntries_.length; i++) {
const change = this.getValidIntersectionChangeEntry_(
this.observeEntries_[i], hostViewport, opt_iframe);
this.observeEntries_[i],
hostViewport,
opt_iframe,
opt_calledFromMutationObserver
);
if (change) {
changes.push(change);
}
Expand All @@ -376,22 +381,37 @@ export class IntersectionObserverPolyfill {
* @param {!ElementIntersectionStateDef} state
* @param {!./layout-rect.LayoutRectDef} hostViewport hostViewport's rect
* @param {./layout-rect.LayoutRectDef=} opt_iframe iframe container rect
* If opt_iframe is provided, all LayoutRect has position relative to
* the iframe. If opt_iframe is not provided,
* all LayoutRect has position relative to the host document.
* @param {boolean=} opt_calledFromMutationObserver
* Whether this was called from a mutation observer
* This will signal that the layout of the elements
* must use getBoundingClientRect, as Resource Scheduler
* could be called too late, and not clear the layout cache.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could instead trigger tick on the next Resource pass, which will mean everything's been remeasured. Then we don't need booleans floating around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice! Yeah let's try that, I think that would be the most optimal solution. Are the Resource passes usually done fast? As in there is nothing that could make it take longer than a second?

And what is the function/service to wait for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the Resource passes usually done fast? As in there is nothing that could make it take longer than a second?

They can take up to 30s if there's no activity on the page (or it's in the background). But since there was a mutation, it'll happen in the next 100ms.

And what is the function/service to wait for this?

There's not currently one to do this. We can add a method to Resources, say onNextPass. Then, at the end of Resources.p.doPass, we resolve call all the pending callbacks, and clear out the array.

class Resources {
  constructor() {
    // ...

    /** @const @private {!Array<function()>} */
    this.onNextPass_ = [];
  }

  doPass() {
    //...

    for (let i = 0; i < this.onNextPass_.length; i++) {
      const fn = this.onNextPass_[i];
      fn();
    }
    this.onNextPass_.length = 0;
  }

  /**
   * Registers a callback to be called when the next pass happens.
   * @param {function()}
   */
  onNextPass(fn) {
    this.onNextPass_.push(fn);
  }
}

* @return {?IntersectionObserverEntry} A valid change entry or null if ratio
* @private
*/
getValidIntersectionChangeEntry_(state, hostViewport, opt_iframe) {
getValidIntersectionChangeEntry_(state, hostViewport,
opt_iframe, opt_calledFromMutationObserver) {
const {element} = state;

// Normalize container LayoutRect to be relative to page
let elementRect = null;
const owner = element.getOwner();
let ownerRect = null;

// If opt_iframe is provided, all LayoutRect has position relative to
// the iframe.
// If opt_iframe is not provided, all LayoutRect has position relative to
// the host document.
const elementRect = element.getLayoutBox();
const owner = element.getOwner();
ownerRect = owner && owner.getLayoutBox();
// If opt_calledFromMutationObserver is provided,
// we need to get our bounding rect rather
// Than our layout box, in order to avoid race conditions
// with the resource scheduler
if (opt_calledFromMutationObserver) {
elementRect = this.viewport_.getLayoutRect(element);
Copy link
Contributor

Choose a reason for hiding this comment

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

@jridgewell Do we need different method when layer is enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, layers is corrected implemented in getLayoutRect. Note it returns a viewport relative box (not a page relative box), but that's no different than the current code.

ownerRect = owner && this.viewport_.getLayoutRect(owner);
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be inside measure phases.

} else {
elementRect = element.getLayoutBox();
ownerRect = owner && owner.getLayoutBox();
}

// calculate intersectionRect. that the element intersects with hostViewport
// and intersects with owner element and container iframe if exists.
Expand Down