Expose submit event with on=submit:el.action syntax. #3739
Conversation
|
Should there be events for success and failure states as well? |
| @@ -72,7 +86,8 @@ | |||
| <form method="post" | |||
| action="/form/html/post" | |||
| action-xhr="/form/echo-json/post" | |||
| target="_blank"> | |||
| target="_blank" | |||
| on="submit:lightbox1"> | |||
There was a problem hiding this comment.
So, is this a "onsubmit" like in the browser? Or logically a "submit success"? I.e. do we wait for submit to finish?
There was a problem hiding this comment.
browser not success. So this would happen regardless of the result of the submission but not when the submission doesn't actually happen (client-side validation errors would not trigger a submit event) .
There was a problem hiding this comment.
Ok. In that case, we should just add "submit" even right away, right where we add this.addEvent('tap');. Any reason not to do it there?
There was a problem hiding this comment.
I just figured no reason to listen to that event if the users do haven't included the amp-form. But I guess we're already kinda doing that with document-submit so might as well add it for the events right away.
I was also worried about the sequence in which events handlers will be called, my understanding if the other ones are capture they should be called first, no?
There was a problem hiding this comment.
Correct. Although needed to be tested. You can also call actionService.trigger() from your existing global submit handler. Or even from both: global and the one in the AmpForm. As long as one even is always sent and no more than one. But I'd generally gravitate toward the global handlers, either via addEvent or global submit handler.
mkhatib
changed the title
|
Added the |
| * event. | ||
| * | ||
| * For example, action service shouldn't trigger 'submit' event if form is actually | ||
| * invalid. stopImmediatePropagation allows us to make sure we don't trigger it |
There was a problem hiding this comment.
Well, it depends on registration order. With this registered in capture phase, and the action registered in bubble phase, it doesn't though.
There was a problem hiding this comment.
Not entirely sure if I understand but even with capture, how would you stop the event from propagating to the listener in actions-impl?
It seems for tap event we're using e.defaultPrevented to prevent triggering in actions. But we can't use this with submit event because we always preventDefault when we do xhr submission, so we can't use that signal to prevent action from triggering.
There was a problem hiding this comment.
For the XHR it might actually make sense to set a property on the event.
However, what I meant is that when stopImmediatePropagation is important over stopPropagation you need to be super careful about order. But the way you currently set up handlers registration order does not make a difference.
|
I didn't follow the full conversation, but this looks good to me in general. Are you adding tests? |
|
Added tests. PTAL. |
|
@cramforce is this good to go? |
|
LGTM |
* master: (236 commits) trim all the columns (ampproject#3894) Refactoring: Turn private custom element methods into functions. (ampproject#3882) Lower the load priority of ad shaped iframes. (ampproject#3863) JsDoc fix (ampproject#3892) Add screenshots for Opera to AMP Validator extension. (ampproject#3866) Fix renaming of generated JSCompiler_prototypeAlias variable. (ampproject#3887) fix typo in amp-sidebar.md (ampproject#3833) Validator Roll-up (ampproject#3885) [CryptoService] Leverage browser native Crypto API to hash strings. (ampproject#3850) Size update (ampproject#3883) copy amp-ad docs to builtins (ampproject#3879) move doc to extension (ampproject#3878) [amp-experiment] Exposes isDismissed() method in AmpUserNotification (ampproject#3832) fix action-impl warning on dist (ampproject#3867) Add params for microad (ampproject#3827) Fixed some A4A tests. (ampproject#3859) Updates to colanalytics vendor config for amp-analytics. (ampproject#3849) Changes to implement A4A (AMP ads for AMP pages) (ampproject#3534) Addresses comment left over from PR#3841 (ampproject#3853) Expose submit event with on=submit:el.action syntax. (ampproject#3739) ...
Not very familiar with action service (can we do deep dive next week on it? 😄).
This works, but not entirely sure if I am missing something or if I should be doing this differently.
ITI: #3343