From 12d441c4bf18ca5ec2593891f6b9e2afd4431488 Mon Sep 17 00:00:00 2001 From: Alan Orozco Date: Wed, 13 Feb 2019 13:44:50 -0800 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8auto-lightbox:=20Discard=20tap=20actio?= =?UTF-8?q?ns=20with=20invalid=20references=20(#20789)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../0.1/amp-auto-lightbox.js | 2 +- .../0.1/test/test-amp-auto-lightbox.js | 16 +++-- .../0.1/service/lightbox-manager-impl.js | 3 +- src/service/action-impl.js | 61 +++++++++++++------ test/unit/test-action.js | 50 +++++++++++++++ 5 files changed, 107 insertions(+), 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 index 4fdf688819fe..9ebf21592990 100644 --- 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 index 264e74a028cd..63eda6376eff 100644 --- 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`
`); + }); + 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 index 92fc9722c4ea..0500790c3dbd 100644 --- 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 index f4b1b35b5fcb..259ff436d8c0 100644 --- 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 index d65d2fd46ae1..3822e09042e3 100644 --- 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`
`); + }); + + it('returns true if the target element exists (single)', () => { + const element = html`
`; + expect(action.hasResolvableAction(element, 'event1')).to.equal(true); + }); + + it('returns true if the target element exists (action up the tree)', () => { + const wrapper = html`
`; + const child = html`
`; + 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`
+
`; + expect(action.hasResolvableAction(element, 'event1')).to.equal(true); + }); + + it('returns false if the target element does not exist (one)', () => { + const element = html`
`; + expect(action.hasResolvableAction(element, 'event1')).to.equal(false); + }); + + it('returns false if the target element does not exist (multiple)', () => { + const element = + html`
`; + expect(action.hasResolvableAction(element, 'event1')).to.equal(false); + }); + + it('returns false if target element does not have the target action', () => { + const element = html`
`; + expect(action.hasResolvableAction(element, 'event2')).to.equal(false); + }); + +}); + describe('Action method', () => { let sandbox;