Bug 61674

Summary:

<input> checkbox and radio attribute value default value incorrect

Product:

WebKit

Reporter:

rasamassen

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Minor

CC:

abarth, commit-queue, kling, logingx, naiem.shaik, tkent

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Proposed Patch	tkent: review-
Incorporating review comments	none
Incorporating review comments	tkent: review-
Updated patch with modified changelog and testcases	none

rasamassen

Reported 2011-05-27 16:05:30 PDT

Per http://www.w3.org/TR/html5/number-state.html#checkbox-state and http://www.w3.org/TR/html5/number-state.html#radio-button-state: The value IDL attribute is in mode default/on: On getting, if the element has a value attribute, it must return that attribute's value; otherwise, it must return the string "on". Currently default value is empty string.

Attachments
Proposed Patch (6.00 KB, patch) 2011-05-31 20:37 PDT, Naiem	tkent: review-	Details Formatted Diff Diff
Incorporating review comments (6.51 KB, patch) 2011-06-01 00:12 PDT, Naiem	no flags	Details Formatted Diff Diff
Incorporating review comments (4.49 KB, patch) 2011-06-01 03:40 PDT, Naiem	tkent: review-	Details Formatted Diff Diff
Updated patch with modified changelog and testcases (5.62 KB, patch) 2011-06-01 19:33 PDT, Naiem	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

rasamassen

Comment 1 2011-05-30 17:02:28 PDT

Test: <!doctype html> <script> var el = document.createElement("input"); el.setAttribute("type", "checkbox"); alert(el.value); </script> Chrome 11: "" Opera 11: "on" Firefox 4: "on" IE9: "on"

Naiem

Comment 2 2011-05-31 20:37:16 PDT

Created attachment 95537 [details] Proposed Patch By default the radio button and checkbox are initialized to false and in the fallbackvalue function if element is not checked we return an empty string, so i am using a variable for maintaining state if value is initialize

Adam Barth

Comment 3 2011-05-31 22:10:23 PDT

Comment on attachment 95537 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95537&action=review > Source/WebCore/ChangeLog:6 > + Default value of Radio button and checkbox should be on > + https://bugs.webkit.org/show_bug.cgi?id=61674 Can you reference a spec to explain why this is the case? > Source/WebCore/html/HTMLInputElement.cpp:97 > , m_wasModifiedByUser(false) > + , m_isInitialized(false) Is there a better name than m_isInitialized? How is this different from m_wasModifiedByUser, for example?

Adam Barth

Comment 4 2011-05-31 22:11:17 PDT

There's some good information in the bug comments that should go in the ChangeLog, like the spec references and the compatibility testing.

Kent Tamura

Comment 5 2011-05-31 23:36:14 PDT

Comment on attachment 95537 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95537&action=review > LayoutTests/ChangeLog:11 > + * fast/html/check-box-input-types-expected.txt: Added. > + * fast/html/check-box-input-types.html: Added. > + * fast/html/radio-input-types-expected.txt: Added. > + * fast/html/radio-input-types.html: Added. The test should be in LayoutTests/fast/forms/. Also, the names are not good. I propose: fast/forms/checkbox-default-value.html fast/forms/radio-default-value.html > LayoutTests/fast/html/check-box-input-types.html:8 > +function print(message) > +{ > + var paragraph = document.createElement("li"); > + paragraph.appendChild(document.createTextNode(message)); > + document.getElementById("console").appendChild(paragraph); > +} You don't need to make your own function. You can use debug() function in fast/js/resource/js-test-pre.js. In this case, you had better use shouldBe() function. > LayoutTests/fast/html/check-box-input-types.html:19 > +<body onload="test()"> You don't need to run the test in onload handler.

Naiem

Comment 6 2011-06-01 00:12:53 PDT

Created attachment 95554 [details] Incorporating review comments

Naiem

Comment 7 2011-06-01 00:13:35 PDT

I will submit a new patch with Kent's comments.

Kent Tamura

Comment 8 2011-06-01 00:49:05 PDT

> On getting, if the element has a value attribute, it must return that attribute's value; otherwise, it must return the string "on". So, the checkedness shouldn't affect the value, right? Why you need to check if setChecked() is called or not?

Naiem

Comment 9 2011-06-01 03:40:28 PDT

Created attachment 95578 [details] Incorporating review comments Updated the patch as per Kent's comments. The value field should be returned "on" irrespective of the state checked or unchecked. So no need to verify for the checked status.

Kent Tamura

Comment 10 2011-06-01 18:50:14 PDT

Comment on attachment 95578 [details] Incorporating review comments View in context: https://bugs.webkit.org/attachment.cgi?id=95578&action=review > Source/WebCore/ChangeLog:17 > + Reviewed by NOBODY (OOPS!). > + As per http://www.w3.org/TR/html5/number-state.html#checkbox-state and > + http://www.w3.org/TR/html5/number-state.html#radio-button-state: > + The value IDL attribute is in mode default/on: > + If the element has a value attribute, it must return that attribute's > + value; otherwise, it must return the string "on". > + Currently default value is empty string;Default value of Radio button > + and checkbox should be "on" > + This works as per spec in IE9,Firefox and Opera > + https://bugs.webkit.org/show_bug.cgi?id=61674 > + > + Tests: fast/forms/checkbox-default-value.html > + fast/forms/radio-default-value.html > + > + * html/BaseCheckableInputType.cpp: The standard format of ChangeLog entry is: Reviewed by NOBODY (OOPS!). <One-line summary of the change> <Bug URL> <Detail of the change> Tests: ... * <Updated file list> > LayoutTests/fast/forms/checkbox-default-value.html:9 > + shouldBe('el.value', '"on"'); Let's test more cases. shouldBe('el.setAttribute("value", "foo"); el.value', '"foo"'); shouldBe('el.checked = true; el.value', '"foo"'); shouldBe('el.removeAttribute("value"); el.value', '"on"'); shouldBe('el.value = "foo"; el.getAttribute("value")', '"foo"');

Naiem

Comment 11 2011-06-01 19:33:20 PDT

Created attachment 95705 [details] Updated patch with modified changelog and testcases Kent, thanks for your comments attaching the new patch with changes you mentioned.

Kent Tamura

Comment 12 2011-06-01 19:35:02 PDT

Comment on attachment 95705 [details] Updated patch with modified changelog and testcases ok, it's great. Thank you for fixing this!

WebKit Commit Bot

Comment 13 2011-06-02 05:01:08 PDT

Comment on attachment 95705 [details] Updated patch with modified changelog and testcases Clearing flags on attachment: 95705 Committed r87896: <http://trac.webkit.org/changeset/87896>

WebKit Commit Bot

Comment 14 2011-06-02 05:01:14 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.