-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from 5 commits
b07076d
a396169
4ea8797
ad813af
9ebc4cd
907397a
6323909
eb06a6d
330e49a
2cbd928
5ed7700
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -203,6 +203,32 @@ 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 {!Window} win | ||
* @param {HTMLHeadElement|Element|ShadowRoot} head | ||
* @param {string} extensionId | ||
* @return {!Promise<boolean>} | ||
*/ | ||
export function isExtensionScriptInNode(win, head, extensionId) { | ||
return dom.waitForBodyPromise(win.document) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this will do the right thing in PWA, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
.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 | ||
|
@@ -222,7 +248,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(); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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, | ||
win: {document: {documentElement: {body: {}}}}, | ||
}; | ||
}); | ||
|
||
/** | ||
* @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'); | ||
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'); | ||
installGlobalSubmitListenerForDoc(ampdoc).then(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have to return the promise in order for Mocha to wait for it. Otherwise, the nested There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
expect(rootNode.addEventListener).called; | ||
}); | ||
}); | ||
}); | ||
|
||
it('should check target and action attributes', () => { | ||
tgt.removeAttribute('action'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
ampFormInstalled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done