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

alabiaga · 2018-08-01T22:17:28Z

register global submit event listener only when the amp-form is installed. If amp-form is not present then document-submit will not validate and hijack form submissions allowing the use of native forms.

Next steps is to possibly move some of the logic from document-submit to the amp-form component. I still think this is possible, with some additional code changes and checks in the amp-form extension.

Tested by running the local gulp server and looking at examples/forms.amp.html. I then edited that file and removed the amp-form script and verified that the xhr service (xhr-impl) and this document-submit global event did not intercept any submits from the form via debug statements.

…ered

dreamofabear · 2018-08-02T16:59:50Z

src/document-submit.js

-  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 getElementServiceIfAvailableForDoc(ampdoc, 'amp-form', 'amp-form')


This will delay installation of the event listener until amp-form extension is installed. We should instead simply check if the extension will be installed and synchronously install or skip the event listener.

I think we can add a new function to element-service.js that uses extensionScriptsInNode() and rework waitForExtensionIfPresent to use it.

done. Thanks

dreamofabear

Can you update the PR description with how you tested this change?

dreamofabear · 2018-08-06T17:22:23Z

src/element-service.js

+ * installation.
+ * @param {HTMLHeadElement|Element|ShadowRoot} head
+ * @param {string} extensionId
+ * return {!Promise<boolean>}


dreamofabear · 2018-08-06T17:23:02Z

src/element-service.js

@@ -222,7 +233,7 @@ function waitForExtensionIfPresent(win, extension, head) {

  // TODO(jpettitt) investigate registerExtension to short circuit
  // the dom call in extensionScriptsInNode()
-  if (!extensionScriptsInNode(head).includes(extension)) {
+  if (!isExtensionScriptInNode(head)) {


You should pass extension here right?

I'd assume tests started to fail given this change. Might want to add some tests if not!

right. gulp check-types task did catch this. Thanks all.

dreamofabear · 2018-08-06T17:24:14Z

test/functional/test-document-submit.js

+      };
+    });
+
+    afterEach(() => sandbox.reset());


Shouldn't be necessary if you don't create the sandbox in beforeEach().

jridgewell · 2018-08-06T18:59:24Z

src/document-submit.js

-  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.
+  if (isExtensionScriptInNode(ampdoc.getHeadNode(), 'amp-form')) {


So installGlobalSubmitListenerForDoc get called super early in the AMP runtime (just after styles are installed). I don't know if <head>'s children will be fully parsed by then, meaning we may miss that <script src="amp-form.js"> is included in the head.

That's the reason getElementServiceIfAvailableForDoc waits for whenBodyAvailable first.

As discussed offline yesterday. Modified new isExtensionScriptInNode method to wait for body before making checking that the script is present in head. Thanks

dreamofabear · 2018-08-07T15:08:42Z

src/document-submit.js

+      ampdoc.win,
+      ampdoc.getHeadNode(),
+      'amp-form')
+      .then(isAmpFormExtension => {


Nit: ampFormInstalled

dreamofabear · 2018-08-07T15:14:56Z

src/element-service.js

+ * @return {!Promise<boolean>}
+ */
+export function isExtensionScriptInNode(win, head, extensionId) {
+  return dom.waitForBodyPromise(win.document)


I don't think this will do the right thing in PWA, since win.document.body != ampdoc.getBody(). 😓

dreamofabear · 2018-08-07T15:16:59Z

test/functional/test-document-submit.js

          sandbox.spy(rootNode, 'addEventListener');
-          installGlobalSubmitListenerForDoc(ampDoc);
-          expect(rootNode.addEventListener).called;
+          installGlobalSubmitListenerForDoc(ampdoc).then(() => {


You have to return the promise in order for Mocha to wait for it. Otherwise, the nested expect() may not be called.

codecov-io · 2018-08-07T18:34:50Z

Codecov Report

Merging #17246 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17246      +/-   ##
==========================================
+ Coverage   77.45%   77.46%   +<.01%     
==========================================
  Files         565      565              
  Lines       41439    41510      +71     
==========================================
+ Hits        32097    32155      +58     
- Misses       9342     9355      +13

Flag	Coverage Δ
#integration_tests	`35.83% <85.71%> (-0.23%)`	⬇️
#unit_tests	`76.52% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0eb99a...eb06a6d. Read the comment docs.

jridgewell · 2018-08-07T18:54:45Z

src/element-service.js

+ * @return {!Promise<boolean>}
+ */
+export function isExtensionScriptInNode(
+  whenbodyAvailPromise, head, extensionId) {


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

alabiaga · 2018-08-07T20:29:59Z

@choumx @jridgewell note the comment in test-document-submit.js. I tried fixing the test but was not able too and was spending too much time figuring out why my fix wasn't working so i left a comment in the meantime. I'll have to revisit. Let me know if I can follow up in a separate PR or if i should just fix it. Thanks

dreamofabear

@alabiaga Fixed the recursive test issue. Previously, the global submit listener installed in _init_tests.js was picking up the submit event. With your change, it was no longer adding the event listener since the amp-form script is not present in the document head.

To fix it, we needed to add the amp-form script to the test window's document head and install the global submit listener given the correct AmpDoc.

alabiaga · 2018-08-09T00:04:37Z

@choumx thanks for the fix. As discussed offline I actually did this very same change and yet it wasn't passing for me.

alabiaga · 2018-08-09T02:01:34Z

@choumx oh i see you installed the global submit event listener in the beforeEach which I didn't do. Thanks again

…ered (ampproject#17246) * install global submit listener only when amp-form extension is registered * fix tests * address comments * fixes * wait for body before checking for script in head * fix test * add comment * address comments * Fix form recursive submit test. * Remove comment.

googlebot added the cla: yes label Aug 1, 2018

alabiaga added 2 commits August 2, 2018 11:01

install global submit listener only when amp-form extension is regist…

b07076d

…ered

fix tests

a396169

alabiaga requested review from jridgewell, westonruter and dreamofabear August 2, 2018 15:16

dreamofabear requested changes Aug 2, 2018

View reviewed changes

address comments

4ea8797

dreamofabear reviewed Aug 6, 2018

View reviewed changes

fixes

ad813af

jridgewell requested changes Aug 6, 2018

View reviewed changes

wait for body before checking for script in head

9ebc4cd

dreamofabear reviewed Aug 7, 2018

View reviewed changes

alabiaga added 2 commits August 7, 2018 13:53

fix test

907397a

add comment

6323909

jridgewell reviewed Aug 7, 2018

View reviewed changes

address comments

eb06a6d

jridgewell approved these changes Aug 7, 2018

View reviewed changes

William Chou added 3 commits August 8, 2018 17:23

Merge branch 'master' into 16761

330e49a

Fix form recursive submit test.

2cbd928

Remove comment.

5ed7700

dreamofabear approved these changes Aug 8, 2018

View reviewed changes

alabiaga merged commit a935150 into ampproject:master Aug 9, 2018

alabiaga deleted the 16761 branch August 9, 2018 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alabiaga commented Aug 1, 2018 •

edited

dreamofabear Aug 2, 2018

alabiaga Aug 6, 2018

dreamofabear left a comment

dreamofabear Aug 6, 2018

alabiaga Aug 6, 2018

dreamofabear Aug 6, 2018

kristoferbaxter Aug 6, 2018

alabiaga Aug 6, 2018

dreamofabear Aug 6, 2018

alabiaga Aug 6, 2018

jridgewell Aug 6, 2018

alabiaga Aug 7, 2018

dreamofabear Aug 7, 2018

alabiaga Aug 7, 2018

dreamofabear Aug 7, 2018

alabiaga Aug 7, 2018

dreamofabear Aug 7, 2018

alabiaga Aug 7, 2018

codecov-io commented Aug 7, 2018 •

edited

jridgewell Aug 7, 2018

alabiaga Aug 7, 2018

alabiaga commented Aug 7, 2018

dreamofabear left a comment

alabiaga commented Aug 9, 2018

alabiaga commented Aug 9, 2018

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

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

Conversation

alabiaga commented Aug 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 7, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Aug 7, 2018

dreamofabear left a comment

Choose a reason for hiding this comment

alabiaga commented Aug 9, 2018

alabiaga commented Aug 9, 2018

alabiaga commented Aug 1, 2018 •

edited

codecov-io commented Aug 7, 2018 •

edited