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 4fdf688819fec..9ebf215929903 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 264e74a028cd3..63eda6376effa 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 92fc9722c4ead..0500790c3dbd3 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 f4b1b35b5fcb4..259ff436d8c01 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 d65d2fd46ae17..3822e09042e34 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;