[amp-inabox] Only request position when visibility-v2 is disabled or …#6787
[amp-inabox] Only request position when visibility-v2 is disabled or …#6787lannka merged 2 commits intoampproject:masterfrom
Conversation
…native IntersectionObserver is not available.
|
lgtm |
|
@bradfrizzell @keithwrightbos just FYI, this means ads tag will not receive a request from inabox (hence no host script needs to be loaded) if we're in a browser that supports IntersectionObserver. |
| /** @override */ | ||
| connect() { | ||
| if (this.visibilityV2Enabled_) { | ||
| // Visibility V2 uses native IntersectionObserver, no position data needed |
There was a problem hiding this comment.
I wonder does the native IntersectionObserver callback function handle remeasure correctly? Could you link me the code where the callback function is?
There was a problem hiding this comment.
The intersection is not calculated by AMP runtime, so no re-measure needs to happen:
https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/0.1/visibility-impl.js#L353
There was a problem hiding this comment.
But we will update this.viewportRect in the polyfill case, and then runtime will take care of the scheduling layout of the element. I can't find where we update this in our native IO callback function. Without it how would runtime know when to layout the element of inabox? This also happen to the viewport onScroll and viewport onResize event. Am I missing something?
There was a problem hiding this comment.
you're right. but inabox V1 does not take care of resource scheduling. we will only make the amp-analytics visibility tracking useful at the moment.
There was a problem hiding this comment.
Then I think we can't just return when this.visiblityV2Enabled_ is true, connect() function is not only about amp-analytics visibility tracking, it will also take care of remeasuring viewportBox for runtime use. We can't simply skip that part of logic with visiblityV2Enabled_ is true?
There was a problem hiding this comment.
As talked offline. This is good with inabox V1. LGTM
|
LGTM |
ampproject#6787) * [amp-inabox] Only request position when visibility-v2 is disabled or native IntersectionObserver is not available. * Fix lint
ampproject#6787) * [amp-inabox] Only request position when visibility-v2 is disabled or native IntersectionObserver is not available. * Fix lint
…native IntersectionObserver is not available.
for #5700