[webkit-reviews] review granted: [Bug 64733] Forms with display:none on the submit button do not get submitted on enter : [Attachment 106402] Updated Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 64733: Forms with display:none on the submit button do not get submitted on
enter
https://bugs.webkit.org/show_bug.cgi?id=64733

Attachment 106402: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=106402&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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".