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
🐛Intersection Observer Polyfill Mutation Observer - Fixed Race Condition #19168
Changes from 17 commits
7ef9930
ad491aa
63caf4b
2be1f3b
59f561e
c17cb5c
87ec925
f96fd1b
0eba342
d5051b9
9892c8e
42a9964
c7282f8
241a4fe
05f8046
e75f2b5
14fc692
095f94f
9bf91c1
5861c3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,11 +242,14 @@ export class IntersectionObserverPolyfill { | |
*/ | ||
this.mutationObserver_ = null; | ||
|
||
/** @private {Pass} */ | ||
this.mutationPass_ = null; | ||
|
||
/** @private {?./service/viewport/viewport-impl.Viewport} */ | ||
this.viewport_ = null; | ||
|
||
/** @private {?Pass} */ | ||
this.mutationPass_ = null; | ||
/** @private {?./service/resources-impl.Resources} */ | ||
this.resources_ = null; | ||
} | ||
|
||
/** | ||
|
@@ -294,12 +297,19 @@ export class IntersectionObserverPolyfill { | |
// from multiple documents. | ||
const ampdoc = Services.ampdoc(element); | ||
if (ampdoc.win.MutationObserver && !this.mutationObserver_) { | ||
this.viewport_ = Services.viewportForDoc(element); | ||
this.mutationPass_ = new Pass(ampdoc.win, () => { | ||
if (this.viewport_) { | ||
this.tick(this.viewport_.getRect()); | ||
} | ||
}); | ||
|
||
if (!this.viewport_) { | ||
this.viewport_ = Services.viewportForDoc(element); | ||
} | ||
|
||
if (!this.resources_) { | ||
this.resources_ = Services.resourcesForDoc(ampdoc); | ||
} | ||
|
||
this.mutationPass_ = new Pass( | ||
ampdoc.win, | ||
this.handleMutationObserverPass_.bind(this) | ||
); | ||
this.mutationObserver_ = new ampdoc.win.MutationObserver( | ||
this.handleMutationObserverNotification_.bind(this) | ||
); | ||
|
@@ -356,7 +366,10 @@ 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 | ||
); | ||
if (change) { | ||
changes.push(change); | ||
} | ||
|
@@ -376,22 +389,18 @@ 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. | ||
* @return {?IntersectionObserverEntry} A valid change entry or null if ratio | ||
* @private | ||
*/ | ||
getValidIntersectionChangeEntry_(state, hostViewport, opt_iframe) { | ||
const {element} = state; | ||
|
||
// Normalize container LayoutRect to be relative to page | ||
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(); | ||
const ownerRect = owner && owner.getLayoutBox(); | ||
|
||
// calculate intersectionRect. that the element intersects with hostViewport | ||
// and intersects with owner element and container iframe if exists. | ||
|
@@ -426,7 +435,20 @@ export class IntersectionObserverPolyfill { | |
|
||
// Wait one animation frame so that other mutations may arrive. | ||
this.mutationPass_.schedule(16); | ||
return; | ||
} | ||
|
||
/** | ||
* Handle Mutation Observer Pass | ||
* This performas the tick, and is wrapped in a paas | ||
* To handle throttling of the observer | ||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @private | ||
*/ | ||
handleMutationObserverPass_() { | ||
if (this.viewport_ && this.resources_) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to check this. |
||
this.resources_.onNextPass(() => { | ||
this.tick(this.viewport_.getRect()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tick is being throttled by the pass isn't it? 😮 Or do you want the pass inside the tick itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I totally forgot we were using a Pass. This is fine, then. |
||
}); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -438,7 +460,6 @@ export class IntersectionObserverPolyfill { | |
this.mutationObserver_.disconnect(); | ||
} | ||
this.mutationObserver_ = null; | ||
this.viewport_ = null; | ||
if (this.mutationPass_) { | ||
this.mutationPass_.cancel(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -211,6 +211,9 @@ export class Resources { | |
/** @private {boolean} */ | ||
this.maybeChangeHeight_ = false; | ||
|
||
/** @const @private {!Array<function()>} */ | ||
this.passCallbacks_ = []; | ||
|
||
/** @private @const {!FiniteStateMachine<!VisibilityState>} */ | ||
this.visibilityStateMachine_ = new FiniteStateMachine( | ||
this.viewer_.getVisibilityState() | ||
|
@@ -1140,6 +1143,14 @@ export class Resources { | |
this.schedulePass(); | ||
} | ||
|
||
/** | ||
* Registers a callback to be called when the next pass happens. | ||
* @param {function()} callback | ||
*/ | ||
onNextPass(callback) { | ||
this.passCallbacks_.push(callback); | ||
} | ||
|
||
/** | ||
* Runs a pass immediately. | ||
*/ | ||
|
@@ -1169,7 +1180,7 @@ export class Resources { | |
this.contentHeight_ = this.viewport_.getContentHeight(); | ||
this.viewer_.sendMessage('documentHeight', | ||
dict({'height': this.contentHeight_}), /* cancelUnsent */true); | ||
dev().fine(TAG_, 'document height on load: ' + this.contentHeight_); | ||
dev().fine(TAG_, 'document height on load: %s', this.contentHeight_); | ||
} | ||
|
||
const viewportSize = this.viewport_.getSize(); | ||
|
@@ -1201,11 +1212,17 @@ export class Resources { | |
this.viewer_.sendMessage('documentHeight', | ||
dict({'height': measuredContentHeight}), /* cancelUnsent */true); | ||
this.contentHeight_ = measuredContentHeight; | ||
dev().fine(TAG_, 'document height changed: ' + this.contentHeight_); | ||
dev().fine(TAG_, 'document height changed: %s', this.contentHeight_); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
this.viewport_.contentHeightChanged(); | ||
} | ||
}); | ||
} | ||
|
||
for (let i = 0; i < this.passCallbacks_.length; i++) { | ||
const fn = this.passCallbacks_[i]; | ||
fn(); | ||
} | ||
this.passCallbacks_.length = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha! Didn't know we can set length to 0 to clean an array : ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotta give the credit to @jridgewell on that one, I didn't know either 😂 |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1634,6 +1634,22 @@ describe('Resources discoverWork', () => { | |
}); | ||
}); | ||
}); | ||
|
||
describe('onNextPass', () => { | ||
|
||
it('should only run callbacks once.', () => { | ||
resources.isRuntimeOn_ = true; | ||
resources.documentReady_ = true; | ||
resources.firstPassAfterDocumentReady_ = true; | ||
|
||
const passCallback = sandbox.spy(); | ||
resources.onNextPass(passCallback); | ||
resources.doPass(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: if I understand it correctly, the callback will be called the first |
||
resources.doPass(); | ||
|
||
expect(passCallback).to.be.calledOnce; | ||
}); | ||
}); | ||
}); | ||
|
||
describes.realWin('Resources contentHeight', { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
this.viewport_
andthis.resources_
used elsewhere? And canelement
andampdoc
be passed to thehandleMutationObserverPass
? Asking because would prefer use callingServices.viewportForDoc(element)
andServices.resourcesForDoc(ampdoc)
directly instead. The cost difference should be minimum.