Fix: amp-ad clean up intersectionObserver timeout #4233
Conversation
| @@ -218,10 +223,18 @@ export class IntersectionObserver extends Observable { | |||
| } | |||
|
|
|||
| /** | |||
| * Provice a function to clear timeout before set this intersection to null. | |||
| * Provide a function to clear timeout before set this intersection to null. | |||
| */ | |||
| destroy() { | |||
| timer.cancel(this.flushTimeout_); | |||
There was a problem hiding this comment.
I just noticed that we do this.baseElement_.getViewport().onScroll above and such. Should we unlisten them as well here?
There was a problem hiding this comment.
I assume there's no need, because in every scenario where we call destory() we would have already set the baseElement viewportCallback(false), and the intersectionObserver onViewportCallback(false) would unlisten them for us.
https://github.com/ampproject/amphtml/blob/master/src/intersection-observer.js#L176
There was a problem hiding this comment.
I'd still go ahead and call onViewportCallback(false) in destroy for double tap (https://www.youtube.com/watch?v=HQe2f8L-Rkc)
|
LGTM |
| }, | ||
| 'scheduleUnlayout': { | ||
| message: bannedTermsHelpString, | ||
| whitelist: [ |
There was a problem hiding this comment.
Instead of whitelisting, let's use /*OK*/ where it's called.
| if (this.unlistenViewportChanges_) { | ||
| this.unlistenViewportChanges_(); | ||
| this.unlistenViewportChanges_ = null; | ||
| } |
There was a problem hiding this comment.
I didn't call onViewportCallback(false) because I don't want to call fire() in destroy() function, but I did add the double tap that check onScroll and onChange listener are unlistened.
Nice Video BTW!
@dvoytenko
|
@jridgewell PTAL |
| */ | ||
| destroy() { | ||
| timer.cancel(this.flushTimeout_); | ||
| this.flushTimeout_ = 0; | ||
| if (this.unlistenViewportChanges_) { |
There was a problem hiding this comment.
Let's move this into it's own method, so we don't have the duplication.
There was a problem hiding this comment.
Done, Add unlistenOnOutViewport_ method
|
I remove the
I put a TODO comment there. It would be a good place to check in the future. |
|
LGTM. |
Add presubmit check for
scheduleUnlayoutfunctionscheduleUnlayoutfor ad when user close sticky-adCheck element inside document before in intersectionObserver
flush_function, fix tests.follow up #4035