Migrate activity to doc-scope and fix instrumentation resolution by mkhatib · Pull Request #6241 · ampproject/amphtml

mkhatib · 2016-11-18T02:34:33Z

More for: #5565

Testing this with pwa.html example looks good. Analytics events triggered and all. But something seems broken for some reason in that example, like I can't really see the document (or scroll it) but I was able to do scroll-element-into-view through the elements panel in devtools. So not sure what's going on there - body for the shadow dom is 0px height.

@dvoytenko as we chatted, instrumentation is now resolved async in amp-analytics due to the race condition between the life cycle of the element and the time we actually install these services.

Let me know if this looks good so I can fix up the tests.

avimehta · 2016-11-18T06:24:00Z

extensions/amp-analytics/0.1/amp-analytics.js

-            this.instrumentation_.addListener(
-                trigger, this.handleEvent_.bind(this, trigger), this.element);
-
+            this.instrumentationPromise_.then(instrumentation => {


would it be simpler to set the value of instrumentation service at the end of layoutcallback. So something like this:

return instrumentationServiceForDoc(this.getAmpDoc())) .then(instrumentation => {this.instrumentation_ = instrumentation;}) .then(() => this.consentPromise_) .then(this.fetchRemoteConfig_.bind(this)) .then(this.onFetchRemoteConfigSuccess_.bind(this));

Yes this would delay the layout for amp-analytics but it might make sense in this case since I guess analytics functionality is dependent on instrumentation being ready.

Yes. @avimehta's suggestion is better. There won't be any real layout delays. But what we want is to resolve instrumentation after we start fetch.

avimehta · 2016-11-18T06:26:02Z

examples/analytics.amp.html

  <script async custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script>
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  <script async src="https://cdn.ampproject.org/v0.js"></script>
+  <script async src="./viewer-integr.js" data-amp-report-test="viewer-integr.js"></script>


what does this do?

I thought this might be needed for the PWA stuff, but not sure. This is usually included for examples to be able to be included in the viewer.html demo.

Not really needed.

dvoytenko · 2016-11-18T18:41:31Z

extensions/amp-analytics/0.1/amp-analytics.js

 import {installCidService} from './cid-impl';
 import {installCryptoService} from './crypto-impl';
-import {installActivityService} from './activity-impl';
+import {Activity} from './activity-impl';


Done... I hate sorting...

It's actually quite pleasant once you get used to it.

dvoytenko · 2016-11-18T18:43:38Z

extensions/amp-analytics/0.1/amp-analytics.js

-            this.instrumentation_.addListener(
-                trigger, this.handleEvent_.bind(this, trigger), this.element);
-
+            this.instrumentationPromise_.then(instrumentation => {


Yes. @avimehta's suggestion is better. There won't be any real layout delays. But what we want is to resolve instrumentation after we start fetch.

dvoytenko · 2016-11-18T18:45:56Z

extensions/amp-analytics/0.1/instrumentation.js

-  return /** @type {!InstrumentationService} */ (getExistingServiceForDoc(
-      nodeOrDoc, 'amp-analytics-instrumentation'));
+  return /** @type {!Promise<InstrumentationService>} */ (
+      getElementServiceForDoc(


getServicePromiseForDoc makes more sense here.

dvoytenko · 2016-11-18T18:47:57Z

src/activity.js

+export function activityForDoc(nodeOrDoc) {
  return /** @type {!Promise<!Activity>} */ (
-      getElementService(win, 'activity', 'amp-analytics'));
+      getElementServiceForDoc(nodeOrDoc, 'activity', 'amp-analytics'));


FYI: this is the correct use of getElementServiceForDoc. However, getServicePromiseForDoc is better in the instrumentation.js. The reason is that this is a public declaration of the service.

Thanks for explaining. Ack.

dvoytenko · 2016-11-18T18:48:20Z

src/service/url-replacements-impl.js


    this.set('BACKGROUND_STATE', () => {
-      return viewerForDoc(this.ampdoc.win.document).isVisible() ? '0' : '1';
+      return viewerForDoc(this.ampdoc).isVisible() ? '0' : '1';


Is this not accurate? Or the ugh on the old change? 😄

The old code is "ugh". I'm thinking maybe we should consider prohibiting doc-service lookup via Document argument to avoid this kind of silly bugs.

dvoytenko · 2016-11-18T22:21:46Z

extensions/amp-analytics/0.1/amp-analytics.js


-    return this.consentPromise_
+    return instrumentationServiceForDoc(this.getAmpDoc())
+        .then(instrumentation => {


As mentioned before, can you move this then after fetchRemoteConfig_? No reason to delay fetching remote data.

mkhatib · 2016-11-22T21:15:18Z

PTAL 👀

avimehta · 2016-11-22T21:54:38Z

extensions/amp-analytics/0.1/amp-analytics.js


    // For consistency with amp-pixel we also expand any url replacements.
-    return urlReplacementsForDoc(this.win.document).expandAsync(request)
+    return urlReplacementsForDoc(this.getAmpDoc()).expandAsync(request)


I am not completely sure if this is correct. I think this will return the service for the main window. What we want is the service a) for the embed if inside an embed and b) for the top level doc if the element is not in embed.

Same above and below.

Yeah, we might need to revert this to use this.win.document then. However, this throws an error AFAIR but I can experiment some more.

cc/ @dvoytenko

@dvoytenko using this.win.document I get errors like:
Error: No ampdoc found for [object HTMLDocument]

Is this supposed to work?

What seems to be working is to pass this.element as we discussed.

This has to be the nearest context node you can find. Here, it could be either:

this.element

event.target

I think the first option is better, but you need to check where events come from. This is something you should also document in comments very well.

Used this.element. I am not entirely sure but event object in this context doesn't seem to include a target.

yes, it is not the js/dom event object but a struct defined by instrumentation.js.

avimehta · 2016-11-22T22:00:13Z

extensions/amp-analytics/0.1/visibility-impl.js

    }
  }

+  let foundEl;


Would moving foundEl above the preceeding if block help? i think the contained chack can be abllied to that as well.

You mean the closestBySelector(parentEl, '.-amp-element') result be checked if is contained in doc?

Not sure if that's the right response to the original question. But checking that the result is in the ampdoc is a good idea.

avimehta · 2016-11-22T22:00:55Z

src/service/url-replacements-impl.js


    this.set('BACKGROUND_STATE', () => {
-      return viewerForDoc(this.ampdoc.win.document).isVisible() ? '0' : '1';
+      return viewerForDoc(this.ampdoc).isVisible() ? '0' : '1';


mkhatib · 2016-11-23T02:56:47Z

extensions/amp-analytics/0.1/visibility-impl.js

+    foundEl = el.parentElement.querySelector(selector);
  } else if (selector[0] == '#') {
-    return ampdoc.getElementById(selector.slice(1));
+    foundEl = el.ownerDocument.getElementById(selector.slice(1));


This is actually more tricky than anticipated.

In embedded this will work.
In PWA this will fail since getElementById won't pierce shadow dom.

So this has to change to become something like:

let doc = ampdoc; if (elWin !== ampdoc.win) { doc = el.ownerDocument; } foundEl = doc.getElementById(selector.slice(1));

@dvoytenko am I getting something wrong?

Yes, this is not entirely correct. The situations you have to consider here are:

The el is in the ampdoc directly. In that case the answer is simply ampdoc.getElementById().

If the el is in the friendly iframe, then it should be ~~ampdoc.getElementById()~~ el.ownerDocument.getElementById().

Whether it's option 1 or 2 you can tell by the presence of parentEl above. You probably want to refactor it a bit to use throughout.

Eventually we'll need to build out the math for all this logic, but let's first see what it is here.

Finally: this logic is not so much hard as it just has to deal with multiple environments. It's important to present them all in tests. Right now, from what I can tell, the tests are insufficient. Let's add more.

Not sure I understand your reply, can you double check I think you might have missed something. Are you saying in both cases we need to use ampdoc.getElementById?

If the el is in the friendly iframe, then it should be ampdoc.getElementById().

This is inaccurate, ampdoc.getElementById will always return null in this case because it won't pierce the iframe window, did you mis-type this or am I missing something?

I think overall I need a better understanding for what getElement should and shouldn't return. I'll draft a small document later and see if I can chat with @avimehta to figure that out.

here is the summary. We can discuss it more over chat or gvc:

Analytics can be done on a page by using amp-analytics tag. There can be more than one of these tags. el in this function is one such amp-analytics tag.

el contains some config that specifies that certain behavior for another element needs to be tracked. The element is identified using selector and selectionMethod. Lets call this element trackedElement.

Generally, trackedElement should always be in the same document as the el. So if el is in shadow dom or friendly iframe, trackedElement should be in the shadow dom or friendly iframe as well.

One exception to this is when ':root' selector is specified. In this case, we travel through the ancestor chain and return the first .-amp-element. This allows for tracking of shadow-dom or iframe as a whole unit.

How we search for trackedElement is determined by selectionMethod.

for selectionMethod == closest, we only look in ancestor chain and find the element that matches.

for selectionMethod == scope, we only look for elements in parent element of el

for no selectionMethod, we just search the DOM for the element.

hope this helps...

@mkhatib Apologies for confusion. I meant to say:

If the el is in the friendly iframe, then it should be ~~ampdoc.getElementById()~~ el.ownerDocument.getElementById().

Thanks @avimehta for the explaination that helped a lot. I updated the code to reflect this.

I've also added some more expectations for the embedded case.

dvoytenko · 2016-11-23T17:31:07Z

extensions/amp-analytics/0.1/amp-analytics.js

    if (threshold >= 0 && threshold <= 100) {
      const key = this.expandTemplate_(spec['sampleOn'], trigger);
-      const keyPromise = urlReplacementsForDoc(this.win.document)
+      const keyPromise = urlReplacementsForDoc(this.getAmpDoc())


Ditto: we need to figure out what's the right argument here. My general reaction is that it should be this.element most of the time, but please check.

Done. this.element is the right one.

dvoytenko · 2016-11-23T17:33:00Z

extensions/amp-analytics/0.1/visibility-impl.js

    }
  }

+  let foundEl;


Not sure if that's the right response to the original question. But checking that the result is in the ampdoc is a good idea.

dvoytenko · 2016-11-23T17:35:20Z

extensions/amp-analytics/0.1/visibility-impl.js

    return null;
  }

+  const elWin = el.ownerDocument.defaultView;


ampdoc is missing above in the @param

dvoytenko · 2016-11-23T17:41:02Z

extensions/amp-analytics/0.1/visibility-impl.js

  // Special case for root selector.
  if (selector == ':host' || selector == ':root') {
-    const elWin = ampdoc.win;
    const parentEl = elWin.frameElement && elWin.frameElement.parentElement;


Please switch to use getParentWindowFrameElement from service.js for frameElement calculation. Looking at this, I actually no idea why we need .parentElement here. I don't think it's needed. The .frameElement (returned by getParentWindowFrameElement) is in itself the parentEl.

cc/ @avimehta here, I think he's the original author here, just to make sure we're not missing anything.

yes, @dvoytenko is correct. .parentElement is an artifact of initial implementation. Please remove it and use the helper method instead.

dvoytenko · 2016-11-23T17:41:25Z

extensions/amp-analytics/0.1/visibility-impl.js

+    foundEl = el.parentElement.querySelector(selector);
  } else if (selector[0] == '#') {
-    return ampdoc.getElementById(selector.slice(1));
+    foundEl = el.ownerDocument.getElementById(selector.slice(1));


Yes, this is not entirely correct. The situations you have to consider here are:

The el is in the ampdoc directly. In that case the answer is simply ampdoc.getElementById().

If the el is in the friendly iframe, then it should be ~~ampdoc.getElementById()~~ el.ownerDocument.getElementById().

Whether it's option 1 or 2 you can tell by the presence of parentEl above. You probably want to refactor it a bit to use throughout.

Eventually we'll need to build out the math for all this logic, but let's first see what it is here.

Finally: this logic is not so much hard as it just has to deal with multiple environments. It's important to present them all in tests. Right now, from what I can tell, the tests are insufficient. Let's add more.

dvoytenko · 2016-11-23T17:42:08Z

extensions/amp-analytics/0.1/visibility-impl.js

+
+  if (foundEl) {
+    // Restrict result to be contained by ampdoc.
+    const isContainedInDoc = ampdoc.contains(


This this should become ampdoc.contains(parentEl || foundEl)

mkhatib · 2016-11-29T01:48:04Z

PTAL 👀

avimehta · 2016-11-29T18:07:25Z

extensions/amp-analytics/0.1/visibility-impl.js

    // Restrict result to be contained by ampdoc.
-    if (closestEl && ampdoc.contains(closestEl)) {
-      return closestEl;
+    const isContainedInDoc = ampdoc.contains(friendlyFrame || foundEl);


don't need to create the const i think.

yeah I just thought it's easier to read this way.

avimehta

lgtm from my side.

dvoytenko · 2016-11-30T00:25:30Z

extensions/amp-analytics/0.1/visibility-impl.js

-      return closestBySelector(parentEl, '.-amp-element');
-    }
+  if (friendlyFrame && (selector == ':host' || selector == ':root')) {
+    foundEl = closestBySelector(friendlyFrame, '.-amp-element');


I believe we should do this:

if (selector == :host || selector == :root) { foundEl = friendlyFrame ? closestBySelector(...) : null; }

Otherwise the code in else below does not actually produce the right result. We have a follow up issue for @avimehta to address this, but to start with it should not produce a wrong result.

…project#6241)

Migrate activity to doc-scope and fix instrumentation resolution.

3c25d3b

mkhatib added the NEEDS REVIEW label Nov 18, 2016

mkhatib assigned avimehta, dvoytenko and dreamofabear Nov 18, 2016

avimehta reviewed Nov 18, 2016

View reviewed changes

dvoytenko suggested changes Nov 18, 2016

View reviewed changes

fix comments

832b6ed

dvoytenko reviewed Nov 18, 2016

View reviewed changes

Mohammad Khatib added 3 commits November 19, 2016 17:12

Fix tests

9780f5c

fix lints

47c1a3b

fix test

94fe462

avimehta reviewed Nov 22, 2016

View reviewed changes

mkhatib commented Nov 23, 2016

View reviewed changes

dvoytenko reviewed Nov 23, 2016

View reviewed changes

dvoytenko suggested changes Nov 23, 2016

View reviewed changes

Updates

e9c3e03

avimehta reviewed Nov 29, 2016

View reviewed changes

avimehta approved these changes Nov 29, 2016

View reviewed changes

dvoytenko approved these changes Nov 30, 2016

View reviewed changes

fix

70a9ffa

mkhatib merged commit 7ff0f3e into ampproject:master Nov 30, 2016

mkhatib deleted the analytics-docs branch November 30, 2016 01:24

jridgewell mentioned this pull request Dec 7, 2016

Friendly Frames Embed Error #6543

Closed

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Migrate activity to doc-scope and fix instrumentation resolution (amp…

c7563a5

…project#6241)

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Migrate activity to doc-scope and fix instrumentation resolution (amp…

924a3a8

…project#6241)

Conversation

mkhatib commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhatib commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhatib Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

mkhatib Nov 23, 2016 •

edited

Loading

dvoytenko Nov 23, 2016 •

edited

Loading

avimehta Nov 24, 2016 •

edited

Loading