Suggested changes

ampproject · Apr 30, 2020 · 1dc361a · 1dc361a
1 parent 9d15c94
commit 1dc361a
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 19 deletions.
diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js
@@ -293,7 +293,7 @@ export class AnalyticsRoot {
       let elements = [];
       for (let i = 0; i < selectors.length; i++) {
         let nodeList;
-        const elementArray = [];
+        let elementArray = [];
         const selector = selectors[i];
         try {
           nodeList = this.getRoot().querySelectorAll(selector);
@@ -302,9 +302,10 @@ export class AnalyticsRoot {
         }
         for (let j = 0; j < nodeList.length; j++) {
           if (this.contains(nodeList[j])) {
-            this.checkElementDataVars(nodeList[j], elementArray, selector);
+            elementArray.push(nodeList[j]);
           }
         }
+        elementArray = this.getDataVarsElements_(elementArray, selector);
         userAssert(elementArray.length, `Element "${selector}" not found`);
         this.verifyAmpElements_(elementArray, selector);
         elements = elements.concat(elementArray);
@@ -317,30 +318,38 @@ export class AnalyticsRoot {
   }
 
   /**
-   * Check if the given element has a data-vars attribute.
-   * Warn if not the case.
-   * @param {!Element} element
+   * Return all elements that have a data-vars attribute.
    * @param {!Array<!Element>} elementArray
    * @param {string} selector
+   * @return {!Array<!Element>}
    */
-  checkElementDataVars(element, elementArray, selector) {
-    const dataVarKeys = Object.keys(
-      getDataParamsFromAttributes(
-        element,
-        /* computeParamNameFunc */ undefined,
-        VARIABLE_DATA_ATTRIBUTE_KEY
-      )
-    );
-    if (dataVarKeys.length) {
-      elementArray.push(element);
-    } else {
+  getDataVarsElements_(elementArray, selector) {
+    let removedCount = 0;
+    const dataVarsArray = [];
+    for (let i = 0; i < elementArray.length; i++) {
+      const dataVarKeys = Object.keys(
+        getDataParamsFromAttributes(
+          elementArray[i],
+          /* computeParamNameFunc */ undefined,
+          VARIABLE_DATA_ATTRIBUTE_KEY
+        )
+      );
+      if (dataVarKeys.length) {
+        dataVarsArray.push(elementArray[i]);
+      } else {
+        removedCount++;
+      }
+    }
+    if (removedCount) {
       user().warn(
         TAG,
-        'An element was ommited from selector "%s"' +
+        '%s element(s) ommited from selector "%s"' +
           ' because no data-vars-* attribute was found.',
+        removedCount,
         selector
       );
     }
+    return dataVarsArray;
   }
 
   /**

diff --git a/extensions/amp-analytics/0.1/test/test-analytics-root.js b/extensions/amp-analytics/0.1/test/test-analytics-root.js
@@ -403,7 +403,8 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => {
         expect(spy).callCount(1);
         expect(spy).to.have.been.calledWith(
           'amp-analytics/analytics-root',
-          'An element was ommited from selector "%s" because no data-vars-* attribute was found.',
+          '"%s" element(s) ommited from selector "%s" because no data-vars-* attribute was found.',
+          1,
           '.myClass'
         );
         expect(children).to.deep.equal([child, child2]);
@@ -765,6 +766,7 @@ describes.realWin(
         const spy = env.sandbox.spy(user(), 'warn');
         const child2 = win.document.createElement('child');
         const child3 = win.document.createElement('child');
+        // Child4 has no data-vars-* attribute
         const child4 = win.document.createElement('child');
         // Parent child attached to parent doc should not be captured
         const parentChild = env.parentWin.document.createElement('child');
@@ -789,7 +791,8 @@ describes.realWin(
         expect(spy).callCount(1);
         expect(spy).to.have.been.calledWith(
           'amp-analytics/analytics-root',
-          'An element was ommited from selector "%s" because no data-vars-* attribute was found.',
+          '"%s" element(s) ommited from selector "%s" because no data-vars-* attribute was found.',
+          1,
           '.myClass'
         );
         expect(elements).to.deep.equals([child, child2]);

diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js
@@ -1965,6 +1965,9 @@ describes.realWin('Events', {amp: 1}, (env) => {
           targetSignals2 = new Signals();
           target2.signals = () => targetSignals2;
 
+          target.setAttribute('data-vars-id', '123');
+          target2.setAttribute('data-vars-id', '123');
+
           saveCallback2 = env.sandbox.match((arg) => {
             if (typeof arg == 'function') {
               saveCallback2.callback = arg;
@@ -2049,12 +2052,14 @@ describes.realWin('Events', {amp: 1}, (env) => {
           expect(eventsSpy.getCall(0).args[2]).to.equal(target);
           expect(eventsSpy.getCall(0).args[3]).to.deep.equal({
             totalVisibleTime: 10,
+            id: '123',
           });
           expect(eventsSpy.getCall(1).args[0]).to.equal('visible');
           expect(eventsSpy.getCall(1).args[1]).to.equal(eventResolver);
           expect(eventsSpy.getCall(1).args[2]).to.equal(target2);
           expect(eventsSpy.getCall(1).args[3]).to.deep.equal({
             totalVisibleTime: 15,
+            id: '123',
           });
         });