Bug 64733

Summary: Forms with display:none on the submit button do not get submitted on enter
Product: WebKit Reporter: Rachel Blum <groby>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: ap, arv, darin, dglazkov, kaustubh.ra, ojan, rniwa, robertknight
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 39021    
Attachments:
Description Flags
Patch
rniwa: review-
Updated Patch
none
test none

Description Rachel Blum 2011-07-18 10:53:13 PDT
1. Create a form with a hidden (display:none;) submit button
2. Press return on a form field to submit the form
3. Form does not submit 


See http://jsfiddle.net/B59rV/2/ for a repro case.

Verified against nightly r91186
Comment 1 Alexey Proskuryakov 2011-07-18 14:15:56 PDT
Duplicate of bug 45224? The history of that bug is super confusing though - it was filed in Bugzilla without any details, but the related chromium bug kept changing beneath it.
Comment 2 Rachel Blum 2011-07-19 10:47:51 PDT
Looks similar to bug 45224, yes, but it's definitely working in other browsers.

All tests on OSX 10.6.8

FF 5.0: PASS 
Opera 11.50(b1074): PASS

Webkit nightly (r91186): FAIL
Safari5.0.5(6533.21.1): FAIL 
Chrome 12.0.742.122: FAIL

The related bug in Chromium is http://crbug.com/89586 

reduced testcase: http://jsfiddle.net/groby/47ALP/3/
Comment 3 Kaustubh Atrawalkar 2011-09-05 03:03:08 PDT
Just wanted to know is this required as per the specs?
Comment 4 Kaustubh Atrawalkar 2011-09-05 04:51:02 PDT
Created attachment 106322 [details]
Patch

Ryosuke, can u review this patch ? There was extra check for renderer() on submit button which can be ignored in display:none case.
Comment 5 Alexey Proskuryakov 2011-09-05 12:11:35 PDT
Kaustubh, could you please follow the history of the renderer check in svn to see why it was added? The check certainly looks like it's intentional, not "extra".
Comment 6 Ryosuke Niwa 2011-09-05 13:19:42 PDT
(In reply to comment #5)
> Kaustubh, could you please follow the history of the renderer check in svn to see why it was added? The check certainly looks like it's intentional, not "extra".

Two changesets touched that line:
http://trac.webkit.org/changeset/59173
http://trac.webkit.org/changeset/5367

And the latter is the one that introduced renderer check.  As far as I can tell the check was introduced because the the code started calling element->renderer()->absolutePosition(x,y); and static_cast<QButton *>(static_cast<RenderWidget *>(element->renderer())->widget())->simulateClick();

But given that the test passes, dispatchSimulatedClick doesn't appear to be dependent on renderer at this point.  So I'd say this fix is correct.
Comment 7 Ryosuke Niwa 2011-09-05 13:24:44 PDT
Comment on attachment 106322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106322&action=review

r- due to various nits.

> Source/WebCore/ChangeLog:4
> +        on enter.

Nit: it seems like "on enter" can hit on the previous line.

> Source/WebCore/ChangeLog:9
> +        Removed check for render() on submit button. Even if submit button has

Nit: s/render/renderer/

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:3
> +    <script>

Nit: I don't think we normally indent script element or the actual script like this.

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:18
> +<body onload="test()">

Do we really need to wait until the page is loaded?  Can't we just put script element below the form element and call submit?

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:19
> +    <form onsubmit="log('Test Passed'); return false;">

Nit: We normally just print "PASS" instead of "Test Passed".

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:21
> +        <input id="txtbox1"/>

Nit: Please don't use abbreviations such as txt.

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:22
> +        <input id="txtbox2"/>

Nit: Why do we need to have two input elements?
Comment 8 Kaustubh Atrawalkar 2011-09-05 23:18:28 PDT
Hi Alexy, 
Adding to Ryosuke's comments, this check for renderer() is only to check if the submit button is displayed or not. The earlier check if (formElement->isSuccessfulSubmitButton()) does check if the submit button is there or not. There might be cases where user just want to show the form with submit action but dont want to show typical submit button. In that case the he can add a button with display:none which will destroy its renderer. And he wont be able to submit form. Removing this check will support as per the bug requirement.

Also from the specs http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#implicit-submission
on the last line it says - "If the form has no submit button, then the implicit submission mechanism must just submit the form element from the form element itself."

(In reply to comment #6)
> (In reply to comment #5)
> > Kaustubh, could you please follow the history of the renderer check in svn to see why it was added? The check certainly looks like it's intentional, not "extra".
> 
> Two changesets touched that line:
> http://trac.webkit.org/changeset/59173
> http://trac.webkit.org/changeset/5367
> 
> And the latter is the one that introduced renderer check.  As far as I can tell the check was introduced because the the code started calling element->renderer()->absolutePosition(x,y); and static_cast<QButton *>(static_cast<RenderWidget *>(element->renderer())->widget())->simulateClick();
> 
> But given that the test passes, dispatchSimulatedClick doesn't appear to be dependent on renderer at this point.  So I'd say this fix is correct.
Comment 9 Kaustubh Atrawalkar 2011-09-05 23:51:12 PDT
> > LayoutTests/fast/forms/hidden-submit-button-form-action.html:22
> > +        <input id="txtbox2"/>
> 
> Nit: Why do we need to have two input elements?

The reason being the issue is not reproducible for only one input box element inside form. I m trying to figure out why so? Will update soon.
Comment 10 Alexey Proskuryakov 2011-09-06 00:09:45 PDT
> And the latter is the one that introduced renderer check.  As far as I can tell the check was introduced because the the code started calling element->renderer()->absolutePosition(x,y); and static_cast<QButton *>(static_cast<RenderWidget *>(element->renderer())->widget())->simulateClick();

Thanks, that makes sense.

The key to going forward with this is to check IE. Per Dimitri, this is expected behavior since it matches IE. I see that http://crbug.com/89586 claims identical behavior in IE and Firefox, but did we confirm that?
Comment 11 Kaustubh Atrawalkar 2011-09-06 00:46:38 PDT
For Single input box, there is check added in http://trac.webkit.org/changeset/58520. And which makes single input box forms to be submitted on enter. (Use case would be "Search" field )
Comment 12 Kaustubh Atrawalkar 2011-09-06 03:28:18 PDT
Created attachment 106402 [details]
Updated Patch

Updated patch with Ryosuke's comments.
Comment 13 Ryosuke Niwa 2011-09-06 08:35:25 PDT
Comment on attachment 106402 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106402&action=review

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:1
> +<html>

Nit: is there a reason we don't have a DOCTYPE here? As far as I can tell, the bug reproduces on both quirks and strict modes.

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:4
> +    <form onsubmit="log('PASS'); return false;">
> +        This tests that hitting the enter key on a input box element with hidden submit button submits the form.<br>

Nit: I don't think there's need for indenting elements like this.

The reason I advocate for not-indenting text/elements is that indentation takes space. Since the "svn up" time is constantly complained about, we should try to minimize the file size as much.

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:11
> +    var textbox2 = document.getElementById('textbox2');

Nit: I don't think there's need for indenting scripts.

> LayoutTests/fast/forms/hidden-submit-button-form-action.html:18
> +    function log(msg) {
> +    var console = document.getElementById('console');

If you just outdent function log(msg) { and }, then the function definition will be properly indented as in:
function log(msg) {
    var console = document.getElementById('console');
    console.innerHTML = console.innerHTML + msg + "<br>";
}
Comment 14 Ryosuke Niwa 2011-09-06 08:36:22 PDT
(In reply to comment #9)
> > > LayoutTests/fast/forms/hidden-submit-button-form-action.html:22
> > > +        <input id="txtbox2"/>
> > 
> > Nit: Why do we need to have two input elements?
> 
> The reason being the issue is not reproducible for only one input box element inside form. I m trying to figure out why so? Will update soon.

THAT is surely odd.  Did you figure out why?
Comment 15 Ryosuke Niwa 2011-09-06 08:37:01 PDT
(In reply to comment #10) 
> The key to going forward with this is to check IE. Per Dimitri, this is expected behavior since it matches IE. I see that http://crbug.com/89586 claims identical behavior in IE and Firefox, but did we confirm that?

New behavior matches Firefox 3.6 at least.
Comment 16 Darin Adler 2011-09-06 09:02:51 PDT
Comment on attachment 106402 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106402&action=review

> Source/WebCore/html/HTMLFormElement.cpp:190
>          if (formElement->isSuccessfulSubmitButton()) {
> -            if (formElement->renderer()) {
> -                formElement->dispatchSimulatedClick(event);
> -                return;
> -            }
> +            formElement->dispatchSimulatedClick(event);
> +            return;
>          } else if (formElement->canTriggerImplicitSubmission())

In the WebKit project our coding style is to not do else after return. So this patch should remove the else and put the if following it on a new line.

> LayoutTests/ChangeLog:11
> +        * fast/forms/hidden-submit-button-form-action-expected.txt: Added.

"hidden" is not a good name for "display: none" since CSS also has "visibility: hidden"

So that’s not a good name for this test.

>> LayoutTests/fast/forms/hidden-submit-button-form-action.html:4
>> +        This tests that hitting the enter key on a input box element with hidden submit button submits the form.<br>
> 
> Nit: I don't think there's need for indenting elements like this.
> 
> The reason I advocate for not-indenting text/elements is that indentation takes space. Since the "svn up" time is constantly complained about, we should try to minimize the file size as much.

I don’t want to start an argument about something irrelevant, but I don’t think we should try to economize on bytes by not indenting. The extra characters from indenting are not a factor in "svn up".
Comment 17 Alexey Proskuryakov 2011-09-06 10:22:31 PDT
Comment on attachment 106402 [details]
Updated Patch

Talked to Darin in person, and canceling r+ for now - until IE behavior is checked. Please feel free to land with Darin's comments addressed if IE agrees with the new behavior.
Comment 18 Ryosuke Niwa 2011-09-06 10:40:15 PDT
The current behavior matches that of MSIE 9.
Comment 19 Alexey Proskuryakov 2011-09-06 11:12:11 PDT
Given that the combined market share of IE and WebKit browsers is much higher than that of Gecko (Firefox), it's likely best for compatibility to WONTFIX this bug.
Comment 20 Kaustubh Atrawalkar 2011-09-06 13:02:38 PDT
> > The reason being the issue is not reproducible for only one input box element inside form. I m trying to figure out why so? Will update soon.
> 
> THAT is surely odd.  Did you figure out why?

Ryosuke, there is a change in which it checks for the 
} else if (formElement->canTriggerImplicitSubmission()) 
    ++submissionTriggerCount; 
} 
if (fromImplicitSubmissionTrigger && submissionTriggerCount == 1) 
 prepareSubmit(event); 

This is why when there is only one input box element, submissionTriggerCount become 1 and it triggers prepareSubmit(event) which cause the use case to work with only one input box. This is designed keeping "Searchbox" in mind, where u can have submit action on enter key press. Also I will take care of indentation as well just to keep code as simple as possible. 

From the specs mentioned in "http://www.w3.org/TR/html5/association-of-controls-and-forms.html#implicit-submission" I think the form submission should be possible even if there is no active submit button. The spec clearly mentions if there is a submit button in the form's tree order (may it be displayed or not is upto developer's choice) the default action on enter should be submitting the form. 
So, IMHO, i think this bug is valid and should be fixed.
Comment 21 Ryosuke Niwa 2011-09-06 13:13:30 PDT
(In reply to comment #20)
> Ryosuke, there is a change in which it checks for the 
> } else if (formElement->canTriggerImplicitSubmission()) 
>     ++submissionTriggerCount; 
> } 
> if (fromImplicitSubmissionTrigger && submissionTriggerCount == 1) 
>  prepareSubmit(event); 
> 
> This is why when there is only one input box element, submissionTriggerCount become 1 and it triggers prepareSubmit(event) which cause the use case to work with only one input box. This is designed keeping "Searchbox" in mind, where u can have submit action on enter key press. Also I will take care of indentation as well just to keep code as simple as possible. 

I see. Very interesting design.  It'll be nice to mention that comment in the change log.

> From the specs mentioned in "http://www.w3.org/TR/html5/association-of-controls-and-forms.html#implicit-submission" I think the form submission should be possible even if there is no active submit button. The spec clearly mentions if there is a submit button in the form's tree order (may it be displayed or not is upto developer's choice) the default action on enter should be submitting the form. 

I agree that fixing this bug seems logical but I'm also worried about the Web compatibility as Alexey pointed out.

+ojan, +arv since they tend to know about these compat. issues better than me.
Comment 22 Ojan Vafai 2011-09-06 17:47:16 PDT
I'm inclined to agree with Alexey. Eventually we want all browsers to agree on a behavior though. We need more data:
1. Are there sites that we know break due to this bug? That would provide evidence for matching FF/Opera.
2. What is the IE8 behavior? IE9 is a totally different engine and much of the existing web is written towards IE6-8.

Assuming IE6-9 and WebKit agree, we should do the following:
1. File Mozilla/Opera bugs to change behavior.
2. Email whatwg suggesting that the spec be changed.

If IE6-8 have the FF/Opera behavior it's less clear to me what the best path forward is. Still a whatwg email is probably in order to make sure all browser vendors agree on a path forward.
Comment 23 Ryosuke Niwa 2011-09-06 18:05:30 PDT
Created attachment 106527 [details]
test

I'm attaching the test because I've got tired of extracting it every time I test it on new browser.
Comment 24 Ryosuke Niwa 2011-09-06 18:23:27 PDT
(In reply to comment #22)
> 2. What is the IE8 behavior? IE9 is a totally different engine and much of the existing web is written towards IE6-8.

I've checked behaviors of IE6, IE7, and IE8.  They all agree with IE9 on both quirks and strict modes.  So there's a significant compat. implications if we change our behavior.
Comment 25 Kaustubh Atrawalkar 2011-09-12 02:44:27 PDT
I have started a thread at whatwg.org about this. http://forums.whatwg.org/bb3/viewtopic.php?f=1&t=4718
Comment 26 Kaustubh Atrawalkar 2011-09-23 21:26:18 PDT
Whatwg.org discussion - http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-September/033281.html
Comment 27 Ryosuke Niwa 2011-09-24 09:57:48 PDT
Upon further investigation, I found that:

WebKit does implicit submission in the following conditions:
One text fields
Two text fields
One text field with one visible submit button
Two text fields with one visible submit button
Two text fields with one visibility:hidden submit button
One text field with one display:none submit button

However, it doesn't submit when we have:
Two text fields with one display:none submit button

Given that WebKit implicitly submits form even in the presence of a visible submit button or an invisible submit button (visibility: hidden, or display: none with exactly one text field), I don't see why we should avoid implicit submission only when there are multiple form controls and a display:none submit button.
Comment 28 Darin Adler 2011-09-24 11:42:46 PDT
I’m curious: Which of those behaviors are different from IE?
Comment 29 Ryosuke Niwa 2011-09-24 13:35:35 PDT
(In reply to comment #28)
> I’m curious: Which of those behaviors are different from IE?

IE does implicit submission in the following conditions:
* One text field
* One text field with one visible submit button
* One text field with one display:none submit button
* Two text fields with one visible submit button

It does not do implicit submissio in the following conditions:

* Two text fields
* Two text fields with one visibility:hidden submit button
* Two text fields with one display:none submit button

Basically, IE only does implicit submission when there is exactly one text field (you can have checkbox, hidden, etc...) or there is a visible button (i.e. not diplay: none nor visibility: hidden).

I'd say IE's behavior is much saner.
Comment 30 Ryosuke Niwa 2011-09-24 13:41:58 PDT
Firefox 5 does implicit submission in the following conditions:
* One text field
* One text field with one visible submit button
* One text field with one display:none submit button
* Two text fields with one visible submit button
* Two text fields with one visibility:hidden submit button
* Two text fields with one display:none submit button

It does not do implicit submissio in the following conditions:

* Two text fields

In summary, Firefox special cases one text field (can have checkbox, hidden, etc...) like MSIE and requires a submit button with more than two text fields regardless of presence of visibility: hidden or display: none.
Comment 31 Ryosuke Niwa 2011-09-24 13:56:30 PDT
One correction. WebKit does not implicitly submit when there are two text fields without any submit buttons.

So WebKit matches IE quite well except:
* Two text fields with one visibility:hidden submit button

Basically IE treats visibility: hidden like display: none for the purpose of determining whether a submit button is visible or not. This seems reasonable given a user won't be able to see a submit button with visibility: hidden.

Alternatively we can change our behavior and match Firefox (and possibily Opeara).

Given the market share of Trident and WebKit combined, I'm more inclined to prefer the former option.
Comment 32 Kaustubh Atrawalkar 2011-09-24 22:55:22 PDT
(In reply to comment #31)
> One correction. WebKit does not implicitly submit when there are two text fields without any submit buttons.
> 
> So WebKit matches IE quite well except:
> * Two text fields with one visibility:hidden submit button
> 
> Basically IE treats visibility: hidden like display: none for the purpose of determining whether a submit button is visible or not. This seems reasonable given a user won't be able to see a submit button with visibility: hidden.
> 
> Alternatively we can change our behavior and match Firefox (and possibily Opeara).
> 

Just to add more analysis
Opera - Does Implicit submission in following cases -
* One text field
* One text field with one visible submit button
* One text field with one display:none submit button
* Two text fields with one visible submit button
* Two text fields with one visibility:hidden submit button
* Two text fields with one display:none submit button

Only in following case it does not -
* Two text fields

> Given the market share of Trident and WebKit combined, I'm more inclined to prefer the former option.

http://www.w3schools.com/browsers/browsers_stats.asp
Considering Increasing share of Firefox (more than 40%) and decreasing share of IE (less than 25%) & also for Mobiles & Tablets there is Opera Mini which is increasing in use. IMHO we can match compliance with FF & Opera & also the specs which clearly allows to implicitly submit without respecting visibility/display of submit button.
Comment 33 Alexey Proskuryakov 2011-09-24 23:28:26 PDT
> http://www.w3schools.com/browsers/browsers_stats.asp
> Considering Increasing share of Firefox (more than 40%) and decreasing share of IE (less than 25%) & also for Mobiles & Tablets there is Opera Mini which is increasing in use. 

This site only has data about its own users. See <http://www.netmarketshare.com/browser-market-share.aspx?qprid=1&qpcustomb=0> for more balanced data. IE and WebKit browsers have roughly 75% share, and growing.

Investigating Opera behavior in particular is not very interesting in compatibility studies. Besides low market share, Opera has many site specific hacks, which can mask compatibility risks from following its behavior.
Comment 34 Kaustubh Atrawalkar 2011-10-09 23:46:41 PDT
Just reopening the bug to conclude. Should we mark this as WONTFIX or can this be fixed?
Comment 35 Eric Seidel (no email) 2011-12-09 14:57:45 PST
Comment on attachment 106402 [details]
Updated Patch

What's the status of this fix?  It looks like it never got set r?
Comment 36 Ryosuke Niwa 2011-12-09 15:07:44 PST
Given that both IE and WebKit have been disabling implicit form submission for years when the button has display: none, I don't think we can change our behavior here.

Please reopen the bug if and only if you find an empirical evidence that this change improves the Web compatibility.
Comment 37 Robert Knight 2016-12-05 06:52:56 PST
This issue was fixed in Chrome in Feb 2014 citing web compatibility, although no affected sites are explicitly named - https://bugs.chromium.org/p/chromium/issues/detail?id=89586

Since this behavior now differs from both Firefox and Chrome, would you reconsider re-opening this?