Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install global submit listener only when amp-form extension is registered #17246

Merged
merged 11 commits into from
Aug 9, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,18 @@ describes.realWin('AmpForm Integration', {

form.dispatchEvent(new Event('submit'));
return fetch.then(() => {
// TODO(alabiaga): the recursive nature in comment below is because of
// the global submit listener via document-submit.js. This is no longer
// valid as the head doesn't have the script present.
// This should be fixed to test this behavior.

// Due to recursive nature of 'on=submit:sameform.submit' we expect
// the action handler to be called twice, the first time for the
// actual user submission.
// The second time in response to the `submit` event being triggered
// and sameform.submit being invoked.
expect(ampForm.handleSubmitAction_).to.have.been.calledTwice;

//expect(ampForm.handleSubmitAction_).to.have.been.calledTwice;
expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
// However, only the first invocation should be handled completely.
// and any subsequent calls should be stopped early-on.
expect(ampForm.handleXhrSubmit_).to.have.been.calledOnce;
Expand Down
15 changes: 14 additions & 1 deletion src/document-submit.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,25 @@ import {
} from './url';
import {Services} from './services';
import {dev, user} from './log';
import {isExtensionScriptInNode} from './element-service';

/**
* @param {!./service/ampdoc-impl.AmpDoc} ampdoc
* @return {!Promise}
*/
export function installGlobalSubmitListenerForDoc(ampdoc) {
ampdoc.getRootNode().addEventListener('submit', onDocumentFormSubmit_, true);
// Register global submit event listener only if the amp-form
// extension is used. Allowing the usage of native forms, otherwise.
return isExtensionScriptInNode(
ampdoc.whenBodyAvailable(),
ampdoc.getHeadNode(),
'amp-form')
.then(ampFormInstalled => {
if (ampFormInstalled) {
ampdoc.getRootNode().addEventListener(
'submit', onDocumentFormSubmit_, true);
}
});
}


Expand Down
29 changes: 28 additions & 1 deletion src/element-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,33 @@ export function extensionScriptsInNode(head) {
return scripts;
}

/**
* Waits for body to be present then verifies that an extension script is
* present in head for installation.
* @param {!Promise<!Element>} whenbodyAvailPromise
* @param {HTMLHeadElement|Element|ShadowRoot} head
* @param {string} extensionId
* @return {!Promise<boolean>}
*/
export function isExtensionScriptInNode(
whenbodyAvailPromise, head, extensionId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass in the ampdoc instead of them providing the whenBody and head.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

return whenbodyAvailPromise
.then(() => {
return extensionScriptInNode(head, extensionId);
});
}

/**
* Verifies that an extension script is present in head for
* installation.
* @param {HTMLHeadElement|Element|ShadowRoot} head
* @param {string} extensionId
* @private
*/
function extensionScriptInNode(head, extensionId) {
return extensionScriptsInNode(head).includes(extensionId);
}

/**
* Waits for an extension if its script is present
* @param {!Window} win
Expand All @@ -222,7 +249,7 @@ function waitForExtensionIfPresent(win, extension, head) {

// TODO(jpettitt) investigate registerExtension to short circuit
// the dom call in extensionScriptsInNode()
if (!extensionScriptsInNode(head).includes(extension)) {
if (!extensionScriptInNode(head, extension)) {
return Promise.resolve();
}

Expand Down
45 changes: 44 additions & 1 deletion test/functional/test-document-submit.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import {Services} from '../../src/services';
import {onDocumentFormSubmit_} from '../../src/document-submit';
import {installGlobalSubmitListenerForDoc, onDocumentFormSubmit_} from '../../src/document-submit';

describe('test-document-submit onDocumentFormSubmit_', () => {
let sandbox;
Expand Down Expand Up @@ -46,6 +46,49 @@ describe('test-document-submit onDocumentFormSubmit_', () => {
sandbox.restore();
});

describe('installGlobalSubmitListenerForDoc', () => {
let ampdoc;
let headNode;
let rootNode;
beforeEach(() => {
headNode = document.createElement('head');
rootNode = document.createElement('html');
ampdoc = {
getHeadNode: () => headNode,
getRootNode: () => rootNode,
whenBodyAvailable: () => Promise.resolve({}),
};
});

/**
* @param {string} extension
*/
const createScript = extension => {
const script = document.createElement('script');
script.setAttribute('src',
'https://cdn.ampproject.org/v0/' + extension + '-0.1.js');
script.setAttribute('custom-element', extension);
return script;
};

it('should not register submit listener if amp-form is not registered.',
() => {
ampdoc.getHeadNode().appendChild(createScript('amp-list'));
sandbox.spy(rootNode, 'addEventListener');
return installGlobalSubmitListenerForDoc(ampdoc).then(() => {
expect(rootNode.addEventListener).not.to.have.been.called;
});
});

it('should register submit listener if amp-form extension is registered.',
() => {
ampdoc.getHeadNode().appendChild(createScript('amp-form'));
sandbox.spy(rootNode, 'addEventListener');
return installGlobalSubmitListenerForDoc(ampdoc).then(() => {
expect(rootNode.addEventListener).called;
});
});
});

it('should check target and action attributes', () => {
tgt.removeAttribute('action');
Expand Down
14 changes: 14 additions & 0 deletions test/functional/test-element-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
getElementServiceIfAvailable,
getElementServiceIfAvailableForDoc,
getElementServiceIfAvailableForDocInEmbedScope,
isExtensionScriptInNode,
} from '../../src/element-service';
import {
installServiceInEmbedScope,
Expand Down Expand Up @@ -330,6 +331,19 @@ describes.realWin('in single ampdoc', {
expect(service).to.deep.equal({str: 'fake1'});
});
});

it('isExtensionScriptInNode', () => {
const extension = document.createElement('script');
extension.setAttribute('custom-element', 'amp-form');
extension.setAttribute('src', 'https://cdn.ampproject.org/v0/amp-form-0.1.js');
ampdoc.getHeadNode().appendChild(extension);
return isExtensionScriptInNode(
ampdoc.whenBodyAvailable(),
ampdoc.getHeadNode(),
'amp-form').then(ampFormInstalled => {
expect(ampFormInstalled).to.equal(true);
});
});
});
});

Expand Down