Don't delay submit in onsubmit event handler

Value set in form submission depends on return value of onSubmit

OnSubmit is cancellable i.e. if return false or defaultPrevented, it should abort form submission.

Allows cancelling or proceeding depending onSubmit value as well handles form submission from JS.

R=tkent, keishi1
BUG=196640
TEST=Submitted values differs on return type of onsubmit. If true, changed values are sent. If false it aborts form submission.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169185

[Habib Virji, 2014-02-24 14:38:59]

As raised in the bug, value BBB was sent instead of AAA as sent by FireFox. The
behavior was due to 
"if (m_isSubmittingOrPreparingForSubmission) {
  m_shouldSubmit = true;
  return;
}"

In bug test file, following behavior is noticed:
1. OnSubmit is triggered and inside onSubmit, another JS submit is triggered.
2. JS submit executes HTMLFormElement::submitFromJavaScript and m_shouldSubmit
is set to true inside HTMLFormElement::submit function as shown in above code.
3. OnSubmit return false, but m_shouldSubmit value is set to true, resulting in
execution of HTMLFormElement::submit call from inside
HTMLFormElement::prepareForSubmission.  

m_shouldSubmit is only used in HTMLFormElement::prepareForSubmission and setting
in HTMLFormElement::submit function is resulting in an issue.

The changes in prepareForSubmission are just to make sure how return value gives
different behavior. There is no change of behavior. Spec reference are added to
explain the flow.

Setting m_isSubmittingOrPreparingForSubmission before dispatchEvent allow JS
submit to be trigger as done in FireFox. It though can raises an issue when user
returns true as well as JS submit both, but flag
m_isSubmittingOrPreparingForSubmission handles such scenarios, resulting in only
one submit.

[keishi, 2014-02-25 15:00:57]

> Setting m_isSubmittingOrPreparingForSubmission before dispatchEvent allow JS
> submit to be trigger as done in FireFox. It though can raises an issue when
user
> returns true as well as JS submit both, but flag
> m_isSubmittingOrPreparingForSubmission handles such scenarios, resulting in
only
> one submit.

I don't think this is true. I tried this and
HTMLFormElement::scheduleFormSubmission was being called twice, but the request
is only sent once because NavigationScheduler cancels the first redirect timer.

Submitting when m_shouldSubmit is false is kind of weird. Could we simplify
this?

I don't understand what m_shouldSubmit is doing. I removed it and the return
value for HTMLFormElement::prepareForSubmission and the layout tests seem to
pass. Am I missing something? Here is the CL I tried.
https://codereview.chromium.org/179663002

https://codereview.chromium.org/177683004/diff/1/Source/core/html/HTMLFormEle...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/177683004/diff/1/Source/core/html/HTMLFormEle...
Source/core/html/HTMLFormElement.cpp:314: || event->defaultPrevented()) {
I think prepareForSubmission is always called from defaultEventHandler() so
event->defaultPrevented() should always be false. Maybe this should be an
ASSERT.

[Habib Virji, 2014-02-25 15:26:40]

Thanks for reviewing.

1. I might have missed then. I was looking for whether form gets submitted twice
or once and found only once. So was suspecting
m_isSubmittingOrPreparingForSubmission.

How should JS trigger and return true scenario should be handled? 
If m_isSubmittingOrPreparingForSubmission is true then JS trigger submit will
fail. 
Firefox seem to be handle JS submit event when return is false and sends AAA in
form submission. 
If return is true and include submit, then last modified value is submitted i.e.
BBB value is sent and not AAA.

2. Regarding m_shouldSubmit, it looks like it was to emulate specs which say
submit method flag to be used. Instead m_shouldSubmit is being used. The way it
is used does not make sense. m_submitting as used by you looks appropriate. 

3. Will add assert statement for event->defaultPrevented().

[keishi, 2014-02-26 03:50:16]

> 1. I might have missed then. I was looking for whether form gets submitted
twice
> or once and found only once. So was suspecting
> m_isSubmittingOrPreparingForSubmission.
> 
> How should JS trigger and return true scenario should be handled? 
> If m_isSubmittingOrPreparingForSubmission is true then JS trigger submit will
> fail. 
> Firefox seem to be handle JS submit event when return is false and sends AAA
in
> form submission. 
> If return is true and include submit, then last modified value is submitted
i.e.
> BBB value is sent and not AAA.

I tried returning true.
http://jsfiddle.net/Z878R/1/

My patch matches Firefox's behavior.
When returning true, the submission with AAA gets scheduled but gets canceled
when the submission with BBB is scheduled.
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

I tried IE and Opera(Presto) and they open AAA and BBB in two new windows. I
think this is wrong. According to the
spec(http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#planned-navigation),
when "plan to navigate" is executed, the form element's previous planned
navigation should be removed from the task queue.

Unless it causes a regression I think we can let NavigationScheduler.cpp cancel
the previous submission and respect the last one.

> 2. Regarding m_shouldSubmit, it looks like it was to emulate specs which say
> submit method flag to be used. Instead m_shouldSubmit is being used. The way
it
> is used does not make sense. m_submitting as used by you looks appropriate. 

I think we don't need the "submit method flag" because prepareForSubmission is
only called when "submit method flag" should be false.
I'm not sure whether the m_submitting flag would be conforming to the spec, but
I think it serves to stop <form action="javascript:form.submit()"> from entering
an infinite loop.

[Habib Virji, 2014-02-26 17:17:08]

On 2014/02/26 03:50:16, keishi1 wrote:
> > 1. I might have missed then. I was looking for whether form gets submitted
> twice
> > or once and found only once. So was suspecting
> > m_isSubmittingOrPreparingForSubmission.
> > 
> > How should JS trigger and return true scenario should be handled? 
> > If m_isSubmittingOrPreparingForSubmission is true then JS trigger submit
will
> > fail. 
> > Firefox seem to be handle JS submit event when return is false and sends AAA
> in
> > form submission. 
> > If return is true and include submit, then last modified value is submitted
> i.e.
> > BBB value is sent and not AAA.
> 
> I tried returning true.
> http://jsfiddle.net/Z878R/1/
> 
> My patch matches Firefox's behavior.
> When returning true, the submission with AAA gets scheduled but gets canceled
> when the submission with BBB is scheduled.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> 
> I tried IE and Opera(Presto) and they open AAA and BBB in two new windows. I
> think this is wrong. According to the
>
spec(http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#planned-navigation),
> when "plan to navigate" is executed, the form element's previous planned
> navigation should be removed from the task queue.
> 
> Unless it causes a regression I think we can let NavigationScheduler.cpp
cancel
> the previous submission and respect the last one.
> 
> > 2. Regarding m_shouldSubmit, it looks like it was to emulate specs which say
> > submit method flag to be used. Instead m_shouldSubmit is being used. The way
> it
> > is used does not make sense. m_submitting as used by you looks appropriate. 
> 
> I think we don't need the "submit method flag" because prepareForSubmission is
> only called when "submit method flag" should be false.
> I'm not sure whether the m_submitting flag would be conforming to the spec,
but
> I think it serves to stop <form action="javascript:form.submit()"> from
entering
> an infinite loop.

Is it okay then with my suggested changes or should adapt some from your CL?
Sorry could not fathom what should be next step.
Thanks for sharing your finding, did not knew much about planned-navigation.
Still new, try to grasp flows:)

[keishi, 2014-02-28 09:47:49]

On 2014/02/26 17:17:08, Habib Virji wrote:
> On 2014/02/26 03:50:16, keishi1 wrote:
> > > 1. I might have missed then. I was looking for whether form gets submitted
> > twice
> > > or once and found only once. So was suspecting
> > > m_isSubmittingOrPreparingForSubmission.
> > > 
> > > How should JS trigger and return true scenario should be handled? 
> > > If m_isSubmittingOrPreparingForSubmission is true then JS trigger submit
> will
> > > fail. 
> > > Firefox seem to be handle JS submit event when return is false and sends
AAA
> > in
> > > form submission. 
> > > If return is true and include submit, then last modified value is
submitted
> > i.e.
> > > BBB value is sent and not AAA.
> > 
> > I tried returning true.
> > http://jsfiddle.net/Z878R/1/
> > 
> > My patch matches Firefox's behavior.
> > When returning true, the submission with AAA gets scheduled but gets
canceled
> > when the submission with BBB is scheduled.
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > 
> > I tried IE and Opera(Presto) and they open AAA and BBB in two new windows. I
> > think this is wrong. According to the
> >
>
spec(http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#planned-navigation),
> > when "plan to navigate" is executed, the form element's previous planned
> > navigation should be removed from the task queue.
> > 
> > Unless it causes a regression I think we can let NavigationScheduler.cpp
> cancel
> > the previous submission and respect the last one.
> > 
> > > 2. Regarding m_shouldSubmit, it looks like it was to emulate specs which
say
> > > submit method flag to be used. Instead m_shouldSubmit is being used. The
way
> > it
> > > is used does not make sense. m_submitting as used by you looks
appropriate. 
> > 
> > I think we don't need the "submit method flag" because prepareForSubmission
is
> > only called when "submit method flag" should be false.
> > I'm not sure whether the m_submitting flag would be conforming to the spec,
> but
> > I think it serves to stop <form action="javascript:form.submit()"> from
> entering
> > an infinite loop.
> 
> Is it okay then with my suggested changes or should adapt some from your CL?
> Sorry could not fathom what should be next step.
> Thanks for sharing your finding, did not knew much about planned-navigation.
> Still new, try to grasp flows:)

We should keep patches minimal so I think this patch should just fix the bug by
moving "setting the m_isSubmittingOrPreparingForSubmission to false" before
"dispatching the submit event", and not change m_shouldSubmit.

[keishi, 2014-02-28 10:57:48]

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
File LayoutTests/fast/forms/form-submission-cancelable.html (right):

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:12:
console.log(query.indexOf('query=AAA') == 1);
We should use js-test.js shouldBe* methods if possible to make it clear what is
pass/fail.
We can probably do something like this (I haven't tested this code myself)
<iframe id="target" onload="testSubmission1()"></iframe>
<iframe id="target1" onload="testSubmission2()"></iframe>
<script>
var count = 2;
function testSubmission1() {
  shouldBeEqualToString('event.target.contentWindow.location.search',
'?query=AAA');
  if (--count === 0)
    finishJSTest();
}
function testSubmission2() {
  shouldBeEqualToString('event.target.contentWindow.location.search',
'?query=BBB');
  if (--count === 0)
    finishJSTest();
}
</script>

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:32: description("Test for
the form submit, when prevented should not submit the form");
This bug is not about respecting the return value of the event handler. I think
the problem is that we have code that delays submits within onsubmit event
handlers. I think the description should be like "Test that submit within
onsubmit event handlers are not delayed" or "Test that form submit within
onsubmit event handlers send the the form data at the time they were called".

I think the bug description ("Value set in form submission depends on return
value of onSubmit") should be updated to something like "Don't delay submit in
onsubmit event handler".

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:32: description("Test for
the form submit, when prevented should not submit the form");
nit: We should unify string literals to single quotes in layout tests.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:34: {
nit: No line break before braces in JavaScript.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:35:
testRunner.dumpAsText();
We have js-test.js so this is already done.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:36:
testRunner.setCanOpenWindows();
I think we don't need these lines? (setCanOpenWindows, setPopupBlockingEnabled,
setCloseRemainingWindowsWhenComplete)

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:39:
testRunner.waitUntilDone();
We have js-test.js so we can use
window.jsTestIsAsync = true;

[Habib Virji, 2014-03-04 14:37:52]

Thanks keishi was useful comments. Updated as suggested.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
File LayoutTests/fast/forms/form-submission-cancelable.html (right):

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:12:
console.log(query.indexOf('query=AAA') == 1);
On 2014/02/28 10:57:48, keishi1 wrote:
> We should use js-test.js shouldBe* methods if possible to make it clear what
is
> pass/fail.
> We can probably do something like this (I haven't tested this code myself)
> <iframe id="target" onload="testSubmission1()"></iframe>
> <iframe id="target1" onload="testSubmission2()"></iframe>
> <script>
> var count = 2;
> function testSubmission1() {
>   shouldBeEqualToString('event.target.contentWindow.location.search',
> '?query=AAA');
>   if (--count === 0)
>     finishJSTest();
> }
> function testSubmission2() {
>   shouldBeEqualToString('event.target.contentWindow.location.search',
> '?query=BBB');
>   if (--count === 0)
>     finishJSTest();
> }
> </script>

Done.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:32: description("Test for
the form submit, when prevented should not submit the form");
On 2014/02/28 10:57:48, keishi1 wrote:
> This bug is not about respecting the return value of the event handler. I
think
> the problem is that we have code that delays submits within onsubmit event
> handlers. I think the description should be like "Test that submit within
> onsubmit event handlers are not delayed" or "Test that form submit within
> onsubmit event handlers send the the form data at the time they were called".
> 
> I think the bug description ("Value set in form submission depends on return
> value of onSubmit") should be updated to something like "Don't delay submit in
> onsubmit event handler".

Done.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:34: {
On 2014/02/28 10:57:48, keishi1 wrote:
> nit: No line break before braces in JavaScript.

Done.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:35:
testRunner.dumpAsText();
On 2014/02/28 10:57:48, keishi1 wrote:
> We have js-test.js so this is already done.

Done.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:36:
testRunner.setCanOpenWindows();
On 2014/02/28 10:57:48, keishi1 wrote:
> I think we don't need these lines? (setCanOpenWindows,
setPopupBlockingEnabled,
> setCloseRemainingWindowsWhenComplete)

Done.

https://codereview.chromium.org/177683004/diff/1/LayoutTests/fast/forms/form-...
LayoutTests/fast/forms/form-submission-cancelable.html:39:
testRunner.waitUntilDone();
On 2014/02/28 10:57:48, keishi1 wrote:
> We have js-test.js so we can use
> window.jsTestIsAsync = true;

Done.

[Habib Virji, 2014-03-06 16:03:39]

@keishi1 are the changes okay?

[keishi, 2014-03-07 01:56:31]

Thanks. LGTM.

https://codereview.chromium.org/177683004/diff/30001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/form-submission-cancelable.html (right):

https://codereview.chromium.org/177683004/diff/30001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-submission-cancelable.html:17: <script>
nit: No indent inside <script> tag.

https://codereview.chromium.org/177683004/diff/30001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-submission-cancelable.html:18: description("Test
that form submit within onsubmit event handlers are not delayed and sends the
form data when invoked");
nit: single quotes

[Habib Virji, 2014-03-07 09:12:34]

Thanks.

Corrected indentation and quotes.

https://codereview.chromium.org/177683004/diff/30001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/form-submission-cancelable.html (right):

https://codereview.chromium.org/177683004/diff/30001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-submission-cancelable.html:17: <script>
On 2014/03/07 01:56:31, keishi1 wrote:
> nit: No indent inside <script> tag.

Done.

https://codereview.chromium.org/177683004/diff/30001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-submission-cancelable.html:18: description("Test
that form submit within onsubmit event handlers are not delayed and sends the
form data when invoked");
On 2014/03/07 01:56:31, keishi1 wrote:
> nit: single quotes

Done.

[Habib Virji, 2014-03-09 16:52:14]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-03-09 16:52:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/177683004/40001

[commit-bot: I haz the power, 2014-03-09 17:27:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-09 17:27:15]

Try jobs failed on following builders: linux_blink_rel

[Habib Virji, 2014-03-10 10:37:53]

@tkent: can you have a look if changes are ok?

[Habib Virji, 2014-03-13 14:27:29]

@tkent: can you have a look. the failure on bot is not due to added tests.

[tkent, 2014-03-13 14:32:03]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-13 14:32:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/177683004/60001

[Habib Virji, 2014-03-13 15:03:25]

On 2014/03/13 14:32:03, tkent wrote:
> The CQ bit was checked by mailto:tkent@chromium.org

Thanks

[commit-bot: I haz the power, 2014-03-13 15:21:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 15:21:16]

Try jobs failed on following builders:
  tryserver.chromium on win_blink_rel

[tkent, 2014-03-13 23:29:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:30:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/177683004/60001

[commit-bot: I haz the power, 2014-03-13 23:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:30:51]

Try jobs failed on following builders:
  tryserver.chromium on win_blink_rel

[tkent, 2014-03-13 23:32:27]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/177683004/60001

[commit-bot: I haz the power, 2014-03-14 00:15:50]

Change committed as 169185

[tkent, 2014-03-17 06:39:19]

https://codereview.chromium.org/177683004/diff/60001/LayoutTests/fast/forms/f...
File LayoutTests/fast/forms/form-submission-cancelable.html (right):

https://codereview.chromium.org/177683004/diff/60001/LayoutTests/fast/forms/f...
LayoutTests/fast/forms/form-submission-cancelable.html:46:
document.getElementById('submitButton').click();
Form submission and load events are asynchronous. We can't expect that the order
of load events follows the click order.