| 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: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
rasamassen
2011-05-27 16:05:30 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"
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
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? There's some good information in the bug comments that should go in the ChangeLog, like the spec references and the compatibility testing. 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. Created attachment 95554 [details]
Incorporating review comments
I will submit a new patch with Kent's comments. > 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?
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.
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"'); Created attachment 95705 [details]
Updated patch with modified changelog and testcases
Kent, thanks for your comments attaching the new patch with changes you mentioned.
Comment on attachment 95705 [details]
Updated patch with modified changelog and testcases
ok, it's great.
Thank you for fixing this!
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> All reviewed patches have been landed. Closing bug. |