Full Text Bug Listing

Rachel Blum

Reported 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

Alexey Proskuryakov

Comment 1 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.

Rachel Blum

Comment 2 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/

Kaustubh Atrawalkar

Comment 3 2011-09-05 03:03:08 PDT

Just wanted to know is this required as per the specs?

Kaustubh Atrawalkar

Comment 4 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.

Alexey Proskuryakov

Comment 5 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".

Ryosuke Niwa

Comment 6 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.

Ryosuke Niwa

Comment 7 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?

Kaustubh Atrawalkar

Comment 8 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.

Kaustubh Atrawalkar

Comment 9 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.

Alexey Proskuryakov

Comment 10 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?

Kaustubh Atrawalkar

Comment 11 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 )

Kaustubh Atrawalkar

Comment 12 2011-09-06 03:28:18 PDT

Created attachment 106402 [details] Updated Patch Updated patch with Ryosuke's comments.

Ryosuke Niwa

Comment 13 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>"; }

Ryosuke Niwa

Comment 14 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?

Ryosuke Niwa

Comment 15 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.

Darin Adler

Comment 16 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".

Alexey Proskuryakov

Comment 17 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.

Ryosuke Niwa

Comment 18 2011-09-06 10:40:15 PDT

The current behavior matches that of MSIE 9.

Alexey Proskuryakov

Comment 19 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.

Kaustubh Atrawalkar

Comment 20 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.

Ryosuke Niwa

Comment 21 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.

Ojan Vafai

Comment 22 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.

Ryosuke Niwa

Comment 23 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.

Ryosuke Niwa

Comment 24 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.

Kaustubh Atrawalkar

Comment 25 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

Kaustubh Atrawalkar

Comment 26 2011-09-23 21:26:18 PDT

Whatwg.org discussion - http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-September/033281.html

Ryosuke Niwa

Comment 27 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.

Darin Adler

Comment 28 2011-09-24 11:42:46 PDT

I’m curious: Which of those behaviors are different from IE?

Ryosuke Niwa

Comment 29 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.

Ryosuke Niwa

Comment 30 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.

Ryosuke Niwa

Comment 31 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.

Kaustubh Atrawalkar

Comment 32 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.

Alexey Proskuryakov

Comment 33 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.

Kaustubh Atrawalkar

Comment 34 2011-10-09 23:46:41 PDT

Just reopening the bug to conclude. Should we mark this as WONTFIX or can this be fixed?

Eric Seidel (no email)

Comment 35 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?

Ryosuke Niwa

Comment 36 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.

Robert Knight

Comment 37 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?

Attachments
Patch (3.24 KB, patch) 2011-09-05 04:51 PDT, Kaustubh Atrawalkar	rniwa: review-	Details Formatted Diff Diff
Updated Patch (3.94 KB, patch) 2011-09-06 03:28 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
test (700 bytes, text/html) 2011-09-06 18:05 PDT, Ryosuke Niwa	no flags	Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Bug 64733