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

Mon Sep 5 13:24:44 PDT 2011

Ryosuke Niwa <rniwa at webkit.org> has denied 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 106322: Patch
https://bugs.webkit.org/attachment.cgi?id=106322&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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?