Blink doesnt respect the form.submit() when called on "focus" event for invalid form

Blink doesnt respect the form.submit() when called on "focus" event for invalid form

Blink doent respect the form.submit() when called on focus event on invalid form.
    When an form submit button is clicked and if the form is invalid it will focus
    the field that shows error. If the user is calling the form.submit() on focus event
    on that field then it should submit the form. But currently the form does not get
    submitted. This patch removes this bug by completely removing the variable
    "m_isSubmittingOrPreparingForSubmission".

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

[harpreet.sk, 2014-03-24 11:27:09]

Please take a look.

[keishi, 2014-03-25 01:28:49]

lgtm

[tkent, 2014-03-25 01:56:42]

https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:293: if (!validateInteractively(event)) {
Any JavaScript code can run in validateInteractively because of invalid and
focus events, and it can call form.submit().  Is it ok?

[harpreet.sk, 2014-03-26 09:16:06]

The coming patch will contain the changes mention in comment

https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:293: if (!validateInteractively(event)) {
On 2014/03/25 01:56:43, tkent wrote:
> Any JavaScript code can run in validateInteractively because of invalid and
> focus events, and it can call form.submit().  Is it ok?

According to the scenario which you mentioned if form.submit() is called in case
of focus or invalid events it will submit the form which is not the expected
behaviour. So in order to behave correctly i am removing the variable
"m_isSubmitting" completely and introducing a new variable "m_invalidForm" which
will be set to true in case of form being invalidate and it's condition is
checked in submit() api.

[tkent, 2014-03-27 00:35:30]

https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormElement.cpp:293: if (!validateInteractively(event)) {
On 2014/03/26 09:16:07, harpreet.sk wrote:
> On 2014/03/25 01:56:43, tkent wrote:
> > Any JavaScript code can run in validateInteractively because of invalid and
> > focus events, and it can call form.submit().  Is it ok?
> 
> According to the scenario which you mentioned if form.submit() is called in
case
> of focus or invalid events it will submit the form which is not the expected
> behaviour. So in order to behave correctly i am removing the variable
> "m_isSubmitting" completely and introducing a new variable "m_invalidForm"
which
> will be set to true in case of form being invalidate and it's condition is
> checked in submit() api.

form.submit() should submit the form even if the form is invalid.
I didn't say form should not be submitted.  I was just afraid any behavior
changes by this CL.

[harpreet.sk, 2014-03-27 07:27:59]

On 2014/03/27 00:35:30, tkent wrote:
>
https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
> File Source/core/html/HTMLFormElement.cpp (right):
> 
>
https://codereview.chromium.org/199633017/diff/20001/Source/core/html/HTMLFor...
> Source/core/html/HTMLFormElement.cpp:293: if (!validateInteractively(event)) {
> On 2014/03/26 09:16:07, harpreet.sk wrote:
> > On 2014/03/25 01:56:43, tkent wrote:
> > > Any JavaScript code can run in validateInteractively because of invalid
and
> > > focus events, and it can call form.submit().  Is it ok?
> > 
> > According to the scenario which you mentioned if form.submit() is called in
> case
> > of focus or invalid events it will submit the form which is not the expected
> > behaviour. So in order to behave correctly i am removing the variable
> > "m_isSubmitting" completely and introducing a new variable "m_invalidForm"
> which
> > will be set to true in case of form being invalidate and it's condition is
> > checked in submit() api.
> 
> form.submit() should submit the form even if the form is invalid.
> I didn't say form should not be submitted.  I was just afraid any behavior
> changes by this CL.


Actually I was talking about the scenario as given below:

<form id="form">
  <label for="choose">Would you prefer a banana or a cherry?</label>
  <input id="choose" name="i_like" value="dvdvdv" pattern="banana|cherry"
onfocus="onFocus(event)" required>
  <button>Submit</button>
</form>
<iframe name="form_target"></iframe>

    <script type="text/javascript" charset="utf-8">

    var form = document.getElementById('form');
     
    form.onFocus = function(event) {
            form.action = 'content.html';
            form.target = 'form_target';

            form.submit();
    };
    </script>


Case 1: If we click on the submit button it will go to the
"validateInteractrively" api and will invalidate the form as value set is not
according to the pattern . While invalidating it will focus the input field
where error occurred. Since the field get focus it will call the JavaScript
which we have written and call form.submit(). In that case it should not submit
the form. With my  previous patch (ie patch set 2) it was submitting the form
which was wrong . With patch set 3 it will not submit the form. I cross checked
the behavior in chrome as well it works the same way . It will not submit the
form if we click on submit button. Actually i was talking about case 1 in my
previous comment.

Case 2: This case is what you are talking about. If we directly focus the input
element (not clicking on the submit button and clicking on input field ) in that
case it should submit the form even if form is invalid. With my current patch
(ie patch set 3) it works fine.

I tried with most of the test cases and it works fine in all cases. I cross
checked the behavior in chrome and other browser . It works similar to them.
Even all the form layout test are working fine with the given change.

[tkent, 2014-03-27 07:40:29]

On 2014/03/27 07:27:59, harpreet.sk wrote:
> Actually I was talking about the scenario as given below:
> 
> <form id="form">
>   <label for="choose">Would you prefer a banana or a cherry?</label>
>   <input id="choose" name="i_like" value="dvdvdv" pattern="banana|cherry"
> onfocus="onFocus(event)" required>
>   <button>Submit</button>
> </form>
> <iframe name="form_target"></iframe>
> 
>     <script type="text/javascript" charset="utf-8">
> 
>     var form = document.getElementById('form');
>      
>     form.onFocus = function(event) {
>             form.action = 'content.html';
>             form.target = 'form_target';
> 
>             form.submit();
>     };
>     </script>
> 
> 
> Case 1: If we click on the submit button it will go to the
> "validateInteractrively" api and will invalidate the form as value set is not
> according to the pattern . While invalidating it will focus the input field
> where error occurred. Since the field get focus it will call the JavaScript
> which we have written and call form.submit(). In that case it should not
submit
> the form. With my  previous patch (ie patch set 2) it was submitting the form
> which was wrong . With patch set 3 it will not submit the form. I cross
checked
> the behavior in chrome as well it works the same way . It will not submit the
> form if we click on submit button. Actually i was talking about case 1 in my
> previous comment.

In this case, IE11 and Firefox 28 submit the form. It's a bug of the current
Chrome. We have no reason to reject form submission by form.submit().

Of course this bug is not your fault. But changing the flag behavior with a
wrong assumption is risky.

[harpreet.sk, 2014-03-27 08:11:52]

On 2014/03/27 07:40:29, tkent wrote:
> On 2014/03/27 07:27:59, harpreet.sk wrote:
> > Actually I was talking about the scenario as given below:
> > 
> > <form id="form">
> >   <label for="choose">Would you prefer a banana or a cherry?</label>
> >   <input id="choose" name="i_like" value="dvdvdv" pattern="banana|cherry"
> > onfocus="onFocus(event)" required>
> >   <button>Submit</button>
> > </form>
> > <iframe name="form_target"></iframe>
> > 
> >     <script type="text/javascript" charset="utf-8">
> > 
> >     var form = document.getElementById('form');
> >      
> >     form.onFocus = function(event) {
> >             form.action = 'content.html';
> >             form.target = 'form_target';
> > 
> >             form.submit();
> >     };
> >     </script>
> > 
> > 
> > Case 1: If we click on the submit button it will go to the
> > "validateInteractrively" api and will invalidate the form as value set is
not
> > according to the pattern . While invalidating it will focus the input field
> > where error occurred. Since the field get focus it will call the JavaScript
> > which we have written and call form.submit(). In that case it should not
> submit
> > the form. With my  previous patch (ie patch set 2) it was submitting the
form
> > which was wrong . With patch set 3 it will not submit the form. I cross
> checked
> > the behavior in chrome as well it works the same way . It will not submit
the
> > form if we click on submit button. Actually i was talking about case 1 in my
> > previous comment.
> 
> In this case, IE11 and Firefox 28 submit the form. It's a bug of the current
> Chrome. We have no reason to reject form submission by form.submit().
> 
> Of course this bug is not your fault. But changing the flag behavior with a
> wrong assumption is risky.


So you want the form to be submitted for case 1 . If that the case then we can
completely remove the variable "m_isSubmitting" and no need to introduce a new
variable . By removing the variable it will submit the form in the above case
also. what say? . I introduce new variable because i thought that form should
not be submitted.

[harpreet.sk, 2014-03-27 10:11:21]

On 2014/03/27 08:11:52, harpreet.sk wrote:
> On 2014/03/27 07:40:29, tkent wrote:
> > On 2014/03/27 07:27:59, harpreet.sk wrote:
> > > Actually I was talking about the scenario as given below:
> > > 
> > > <form id="form">
> > >   <label for="choose">Would you prefer a banana or a cherry?</label>
> > >   <input id="choose" name="i_like" value="dvdvdv" pattern="banana|cherry"
> > > onfocus="onFocus(event)" required>
> > >   <button>Submit</button>
> > > </form>
> > > <iframe name="form_target"></iframe>
> > > 
> > >     <script type="text/javascript" charset="utf-8">
> > > 
> > >     var form = document.getElementById('form');
> > >      
> > >     form.onFocus = function(event) {
> > >             form.action = 'content.html';
> > >             form.target = 'form_target';
> > > 
> > >             form.submit();
> > >     };
> > >     </script>
> > > 
> > > 
> > > Case 1: If we click on the submit button it will go to the
> > > "validateInteractrively" api and will invalidate the form as value set is
> not
> > > according to the pattern . While invalidating it will focus the input
field
> > > where error occurred. Since the field get focus it will call the
JavaScript
> > > which we have written and call form.submit(). In that case it should not
> > submit
> > > the form. With my  previous patch (ie patch set 2) it was submitting the
> form
> > > which was wrong . With patch set 3 it will not submit the form. I cross
> > checked
> > > the behavior in chrome as well it works the same way . It will not submit
> the
> > > form if we click on submit button. Actually i was talking about case 1 in
my
> > > previous comment.
> > 
> > In this case, IE11 and Firefox 28 submit the form. It's a bug of the current
> > Chrome. We have no reason to reject form submission by form.submit().
> > 
> > Of course this bug is not your fault. But changing the flag behavior with a
> > wrong assumption is risky.
> 
> 
> So you want the form to be submitted for case 1 . If that the case then we can
> completely remove the variable "m_isSubmitting" and no need to introduce a new
> variable . By removing the variable it will submit the form in the above case
> also. what say? . I introduce new variable because i thought that form should
> not be submitted.




As per our discussion i had filed the bug for the same. A new patch is uploaded
which removes that bug.

[tkent, 2014-03-31 07:18:05]

The code change looks good.
We need a test.

[harpreet.sk, 2014-03-31 12:46:08]

On 2014/03/31 07:18:05, tkent wrote:
> The code change looks good.
> We need a test.


Layout Test has been added for the patch. Please take a look.

[tkent, 2014-04-01 02:23:14]

https://codereview.chromium.org/199633017/diff/90001/LayoutTests/fast/forms/s...
File LayoutTests/fast/forms/submit-onFocus-invalidForm.html (right):

https://codereview.chromium.org/199633017/diff/90001/LayoutTests/fast/forms/s...
LayoutTests/fast/forms/submit-onFocus-invalidForm.html:16: function
asyncTest(query) {
This function and 'count' are unnecessary. We can just check |query ===
'?query=AAA'| in onload handler of 'target'.

[harpreet.sk, 2014-04-01 05:46:07]

Patch set 6 addresses the change asked in comment in patch set 5. Please take a
look.

https://codereview.chromium.org/199633017/diff/90001/LayoutTests/fast/forms/s...
File LayoutTests/fast/forms/submit-onFocus-invalidForm.html (right):

https://codereview.chromium.org/199633017/diff/90001/LayoutTests/fast/forms/s...
LayoutTests/fast/forms/submit-onFocus-invalidForm.html:16: function
asyncTest(query) {
On 2014/04/01 02:23:15, tkent wrote:
> This function and 'count' are unnecessary. We can just check |query ===
> '?query=AAA'| in onload handler of 'target'.

Done.

[tkent, 2014-04-01 05:50:34]

lgtm

[commit-bot: I haz the power, 2014-04-01 05:50:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/199633017/100001

[commit-bot: I haz the power, 2014-04-01 06:03:03]

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

[commit-bot: I haz the power, 2014-04-01 06:03:03]

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

[tkent, 2014-04-01 06:03:27]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-04-01 06:03:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/199633017/100001

[commit-bot: I haz the power, 2014-04-01 06:56:35]

Change committed as 170520