Summary: | Forms with display:none on the submit button do not get submitted on enter | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rachel Blum <groby> | ||||||||
Component: | Forms | Assignee: | 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
Rachel Blum
2011-07-18 10:53:13 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. 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/ Just wanted to know is this required as per the specs? 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.
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". (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 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? 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. > > 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.
> 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? 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 ) Created attachment 106402 [details]
Updated Patch
Updated patch with Ryosuke's comments.
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>"; } (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? (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 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 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.
The current behavior matches that of MSIE 9. 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. > > 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. (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. 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. 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.
(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. I have started a thread at whatwg.org about this. http://forums.whatwg.org/bb3/viewtopic.php?f=1&t=4718 Whatwg.org discussion - http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-September/033281.html 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. I’m curious: Which of those behaviors are different from IE? (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. 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. 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. (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. > 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. Just reopening the bug to conclude. Should we mark this as WONTFIX or can this be fixed? Comment on attachment 106402 [details]
Updated Patch
What's the status of this fix? It looks like it never got set r?
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. 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? |