Bug 61674 - <input> checkbox and radio attribute value default value incorrect
Summary: <input> checkbox and radio attribute value default value incorrect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-27 16:05 PDT by rasamassen
Modified: 2011-06-02 05:01 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description rasamassen 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.
Comment 1 rasamassen 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"
Comment 2 Naiem 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
Comment 3 Adam Barth 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?
Comment 4 Adam Barth 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.
Comment 5 Kent Tamura 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.
Comment 6 Naiem 2011-06-01 00:12:53 PDT
Created attachment 95554 [details]
Incorporating review comments
Comment 7 Naiem 2011-06-01 00:13:35 PDT
I will submit a new patch with Kent's comments.
Comment 8 Kent Tamura 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?
Comment 9 Naiem 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.
Comment 10 Kent Tamura 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"');
Comment 11 Naiem 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.
Comment 12 Kent Tamura 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!
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2011-06-02 05:01:14 PDT
All reviewed patches have been landed.  Closing bug.