🐛async-input: Fixed NonXHR GET on amp-form with Async Input Elements #20362
Conversation
extensions/amp-form/0.1/amp-form.js
Outdated
| if (this.xhrAction_) { | ||
| return this.handleXhrSubmit_(trust); | ||
| } else if (this.method_ == 'POST') { | ||
| this.handleNonXhrPost_(); | ||
| } else if (this.method_ == 'GET') { | ||
| this.handleNonXhrGet_(event); | ||
| this.handleNonXhrGet_(true); |
There was a problem hiding this comment.
nit: add a boolean explainer comment this.handleNonXhrGet_(/*shouldSubmitFormElement*/ true);
|
LGTM! Does the bug affect Async var substitution? |
| @@ -2114,7 +2118,7 @@ describes.repeated('', { | |||
| 'formFields[name]': 'John Miller', | |||
| 'formFields[email]': 'j@hnmiller.com', | |||
| }; | |||
| expect(form.submit).to.have.been.called; | |||
| expect(form.submit).to.have.not.been.called; | |||
There was a problem hiding this comment.
I am confused, I think form.submit should be called since it's a non-XHR get, and the form.submit call is necessary to submit the request and navigate the browser, right?
There was a problem hiding this comment.
So from my understanding, on Non-XHR Get. The form.submit isn't called in the case where there are no Async Inputs (this test). As the Event itself handles the submission.
However, in the case where there are Async Inputs (not this test), we need to manually call form.submit, as the event was preventDefault(), in order to let the AsyncInput do it's getValue().
Glad you noticed this and asked questions to request changes 😄 Let me know if this makes sense. If this is confusing, perhaps for the Non XHR Tests we should add comments specifying why the form submit should/shouldn't be called? 🤔
There was a problem hiding this comment.
Ah I missed that there was an event that was propagating that would eventually cause the page to submit, thanks for explaining! Comments could be good but don't let that block you submitting this PR
There was a problem hiding this comment.
@cvializ You are welcome! And sounds good! I'll go ahead and merge this, as I think the amp-form tests need an improvement in general, so I'll cross that bridge when it comes 😂
Thanks for the review!
…lements (ampproject#20362)" This reverts commit de0ea8e.
…mpproject#20362) * Fixed the event on the handleNonXhrGet * Fixed presubmit and tests * Fixed other Non Xhr Tests, and made PR Comments
…lements (ampproject#20362)" (ampproject#20601) This reverts commit de0ea8e.
relates to #19334
Discovered this bug recently, I think it happened from a comment that I forgot to test afterwards, and just now came across it by accidentally breaking my manual testing muscle memory 😂
Previously, the
this.form_.submitwasn't getting called because the event was passed in both places (though it was not really needed). Thus I went ahead and made things more clear with a boolean, and all works now.Thankfully, this bug ONLY affects the Async Input case, and not the regular case 😄