✨auto-lightbox: Discard tap actions with invalid references

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');
   }
 }
 
@@ -386,6 +386,10 @@ function generateLightboxUid() {
  */
 export function apply(ampdoc, element) {
   return Mutation.mutate(element, () => {
+    // amp-lightbox-gallery will reject elements with a set `on` attribute.
+    // Since the attribute is resolved to be valid before applying, this is ok
+    // to do.
+    element.removeAttribute('on');
     element.setAttribute(LIGHTBOXABLE_ATTR, generateLightboxUid());
   }).then(() => {
     Services.extensionsFor(ampdoc.win)

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',
@@ -668,6 +676,15 @@ describes.realWin(TAG, {
       expect(element).to.have.attribute(LIGHTBOXABLE_ATTR);
     });
 
+    it('removes `on` action', async() => {
+      const element = html`<amp-img src="chabuddy.g" on="tap:foo"></amp-img>`;
+
+      await apply(env.ampdoc, element);
+
+      expect(element).to.have.attribute(LIGHTBOXABLE_ATTR);
+      expect(element).not.to.have.attribute('on');
+    });
+
     it('sets unique group for each element', async() => {
       const candidates = [1, 2, 3].map(() => html`<amp-img></amp-img>`);
 

src/service/action-impl.js

@@ -446,6 +446,35 @@ export class ActionService {
     return !!this.findAction_(target, 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} target
+   * @param {string} actionEventType
+   * @param {!Element=} opt_stopAt
+   * @return {boolean}
+   */
+  hasResolvableAction(target, actionEventType, opt_stopAt) {
+    const action = this.findAction_(target, 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);
+  }
+
   /**
    * Sets the action whitelist. Can be used to clear it.
    * @param {!Array<{tagOrTarget: string, method: string}>} whitelist
@@ -485,25 +514,19 @@ 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;
+    action.actionInfos.forEach(({target, args, method, str}) => {
       // Replace any variables in args with data in `event`.
-      const args = dereferenceExprsInArgs(actionInfo.args, event);
+      const dereferencedArgs = dereferenceExprsInArgs(args, event);
       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)

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';
 
 
@@ -642,6 +643,55 @@ describe('Action hasAction', () => {
 
 });
 
+describes.fakeWin('Action hasResolvableAction', {}, env => {
+  let action;
+  let html;
+
+  beforeEach(() => {
+    html = htmlFor(env.win.document);
+    action = new ActionService(new AmpDocSingle(env.win), 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;