✨auto-lightbox: Discard tap actions with invalid references (ampproje…

…ct#20789) Exposes `ActionService.hasResolvableAction` to: - Discard tap action in auto-lightbox criteria when it does not resolve to a valid target. - Fix `amp-lightbox-gallery` so it will properly traverse up the tree to discard targets, and not to discard those dynamically set.
estherkim · Feb 13, 2019 · 12d441c · 12d441c
1 parent dd4037d
commit 12d441c
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 25 deletions.
diff --git a/extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js b/extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js
@@ -152,7 +152,7 @@ export class Criteria {
       return false;
     }
     const actions = Services.actionServiceForDoc(element);
-    return !actions.hasAction(element, 'tap');
+    return !actions.hasResolvableAction(element, 'tap');
   }
 }
 

diff --git a/extensions/amp-auto-lightbox/0.1/test/test-amp-auto-lightbox.js b/extensions/amp-auto-lightbox/0.1/test/test-amp-auto-lightbox.js
@@ -176,13 +176,22 @@ describes.realWin(TAG, {
       criteriaIsMetThereforeAcceptsOrRejects(false, [{rejects: 'any'}]);
     });
 
+    beforeEach(() => {
+      // Insert element for valid tap actions to be resolved.
+      env.win.document.body.appendChild(html`<div id="valid"></div>`);
+    });
+
     itAcceptsOrRejects([
       {
         accepts: 'elements by default',
       },
       {
         accepts: 'elements with a non-tap action',
-        mutate: el => el.setAttribute('on', 'nottap:doSomething'),
+        mutate: el => el.setAttribute('on', 'nottap:valid'),
+      },
+      {
+        accepts: 'elements with a tap action that does not resolve to a node',
+        mutate: el => el.setAttribute('on', 'tap:i-do-not-exist'),
       },
       {
         accepts: 'elements inside non-clickable anchor',
@@ -198,12 +207,11 @@ describes.realWin(TAG, {
       },
       {
         rejects: 'items actionable by tap with a single action',
-        mutate: el => el.setAttribute('on', 'tap:doSomething'),
+        mutate: el => el.setAttribute('on', 'tap:valid'),
       },
       {
         rejects: 'items actionable by tap with multiple actions',
-        mutate: el =>
-          el.setAttribute('on', 'whatever:doSomething;tap:doSomethingElse'),
+        mutate: el => el.setAttribute('on', 'whatever:something;tap:valid'),
       },
       {
         rejects: 'items inside an amp-selector',

diff --git a/extensions/amp-lightbox-gallery/0.1/service/lightbox-manager-impl.js b/extensions/amp-lightbox-gallery/0.1/service/lightbox-manager-impl.js
@@ -149,7 +149,8 @@ export class LightboxManager {
     if (!ELIGIBLE_TAP_TAGS[element.tagName]) {
       return false;
     }
-    if (element.hasAttribute('on')) {
+    const actions = Services.actionServiceForDoc(element);
+    if (actions.hasResolvableAction(element, 'tap')) {
       return false;
     }
     return true;

diff --git a/src/service/action-impl.js b/src/service/action-impl.js
@@ -437,13 +437,42 @@ export class ActionService {
 
   /**
    * Checks if the given element has registered a particular action type.
-   * @param {!Element} target
+   * @param {!Element} element
+   * @param {string} actionEventType
+   * @param {!Element=} opt_stopAt
+   * @return {boolean}
+   */
+  hasAction(element, actionEventType, opt_stopAt) {
+    return !!this.findAction_(element, actionEventType, opt_stopAt);
+  }
+
+  /**
+   * Checks if the given element's registered action resolves to at least one
+   * existing element by id or a global target (e.g. "AMP").
+   * @param {!Element} element
    * @param {string} actionEventType
    * @param {!Element=} opt_stopAt
    * @return {boolean}
    */
-  hasAction(target, actionEventType, opt_stopAt) {
-    return !!this.findAction_(target, actionEventType, opt_stopAt);
+  hasResolvableAction(element, actionEventType, opt_stopAt) {
+    const action = this.findAction_(element, actionEventType, opt_stopAt);
+    if (!action) {
+      return false;
+    }
+    return action.actionInfos.some(({target}) => !!this.getActionNode_(target));
+  }
+
+  /**
+   * For global targets e.g. "AMP", returns the document root. Otherwise,
+   * `target` is an element id and the corresponding element is returned.
+   * @param {string} target
+   * @return {?Document|?Element|?ShadowRoot}
+   * @private
+   */
+  getActionNode_(target) {
+    return this.globalTargets_[target] ?
+      this.root_ :
+      this.root_.getElementById(target);
   }
 
   /**
@@ -487,24 +516,18 @@ export class ActionService {
     // Invoke actions serially, where each action waits for its predecessor
     // to complete. `currentPromise` is the i'th promise in the chain.
     let currentPromise = null;
-    action.actionInfos.forEach(actionInfo => {
-      const {target} = actionInfo;
-      const args = dereferenceArgsVariables(actionInfo.args, event, opt_args);
+    action.actionInfos.forEach(({target, args, method, str}) => {
+      const dereferencedArgs = dereferenceArgsVariables(args, event, opt_args);
       const invokeAction = () => {
-        // For global targets e.g. "AMP, `node` is the document root. Otherwise,
-        // `target` is an element id and `node` is the corresponding element.
-        const node = (this.globalTargets_[target])
-          ? this.root_
-          : this.root_.getElementById(target);
-        if (node) {
-          const invocation = new ActionInvocation(node, actionInfo.method,
-              args, source, action.node, event, trust,
-              actionEventType, node.tagName || target, sequenceId);
-          return this.invoke_(invocation);
-        } else {
-          this.error_(`Target "${target}" not found for action ` +
-              `[${actionInfo.str}].`);
+        const node = this.getActionNode_(target);
+        if (!node) {
+          this.error_(`Target "${target}" not found for action [${str}].`);
+          return;
         }
+        const invocation = new ActionInvocation(node, method,
+            dereferencedArgs, source, action.node, event, trust,
+            actionEventType, node.tagName || target, sequenceId);
+        return this.invoke_(invocation);
       };
       // Wait for the previous action, if any.
       currentPromise = (currentPromise)

diff --git a/test/unit/test-action.js b/test/unit/test-action.js
@@ -30,6 +30,7 @@ import {AmpDocSingle} from '../../src/service/ampdoc-impl';
 import {Keys} from '../../src/utils/key-codes';
 import {createCustomEvent} from '../../src/event-helper';
 import {createElementWithAttributes} from '../../src/dom';
+import {htmlFor} from '../../src/static-template';
 import {setParentWindow} from '../../src/service';
 import {whenCalled} from '../../testing/test-helper.js';
 
@@ -653,6 +654,55 @@ describe('Action hasAction', () => {
 
 });
 
+describes.fakeWin('Action hasResolvableAction', {amp: true}, env => {
+  let action;
+  let html;
+
+  beforeEach(() => {
+    html = htmlFor(env.win.document);
+    action = new ActionService(env.ampdoc, env.win.document);
+
+    // Insert element for valid actions to be resolved.
+    env.win.document.body.appendChild(html`<div id="valid-target"></div>`);
+  });
+
+  it('returns true if the target element exists (single)', () => {
+    const element = html`<div on="event1: valid-target"></div>`;
+    expect(action.hasResolvableAction(element, 'event1')).to.equal(true);
+  });
+
+  it('returns true if the target element exists (action up the tree)', () => {
+    const wrapper = html`<div on="event1: valid-target"></div>`;
+    const child = html`<div></div>`;
+    wrapper.appendChild(child);
+    expect(action.hasResolvableAction(child, 'event1')).to.equal(true);
+  });
+
+  it('returns true if the target element exists (one amongst many)', () => {
+    const element =
+      html`<div on="event1: i-dont-exist, valid-target, i-dont-exist-either">
+      </div>`;
+    expect(action.hasResolvableAction(element, 'event1')).to.equal(true);
+  });
+
+  it('returns false if the target element does not exist (one)', () => {
+    const element = html`<div on="event1: i-do-not-exist"></div>`;
+    expect(action.hasResolvableAction(element, 'event1')).to.equal(false);
+  });
+
+  it('returns false if the target element does not exist (multiple)', () => {
+    const element =
+      html`<div on="event1: i-do-not-exist, i-dont-exist-either"></div>`;
+    expect(action.hasResolvableAction(element, 'event1')).to.equal(false);
+  });
+
+  it('returns false if target element does not have the target action', () => {
+    const element = html`<div on="event1: valid-target"></div>`;
+    expect(action.hasResolvableAction(element, 'event2')).to.equal(false);
+  });
+
+});
+
 
 describe('Action method', () => {
   let sandbox;