Bug 27760 - HTMLOptionElement::value() returns incorrect value
Summary: HTMLOptionElement::value() returns incorrect value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-28 02:35 PDT by Kent Tamura
Modified: 2009-07-29 01:12 PDT (History)
2 users (show)

See Also:


Attachments
patch (7.10 KB, patch)
2009-07-28 03:48 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
patch (8.71 KB, patch)
2009-07-28 18:21 PDT, Kent Tamura
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-07-28 02:35:35 PDT
Suppose that we have 4 OPTION elements as follows:
 <option id="o1">text</option>
 <option id="o2" value="value">text</option>
 <option id="o3" label="label">text</option>
 <option id="o4" value="value" label="label">text</option>

The current WebKit returns the following values for .value and .label:
o1: value=text label=
o2: value=value label=
o3: value=label label=label
o4: value=value label=label

Apparently the value of o3 is incorrect.  It should be "text" or "".  I think r40130 introduced this bug.

FYI:
On Firefox 3.5 and Opera 10:
o1: value=text label=
o2: value=value label=
o3: value=text label=label
o4: value=value label=label

On IE8:
o1: value= label=
o2: value=value label=
o3: value= label=label
o4: value=value label=label

As for HTML5, .value and .label just reflect their attribute values.
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-option-value
So IE8 behavior is compliant.
Comment 1 Kent Tamura 2009-07-28 02:43:50 PDT
Correction:
In the description, IE8 meant IE7.  IE8's behavior is exactly same as Firefox 3.5 and Opera 10.
Comment 2 Kent Tamura 2009-07-28 03:48:47 PDT
Created attachment 33613 [details]
patch
Comment 3 Nikolas Zimmermann 2009-07-28 11:32:37 PDT
Comment on attachment 33613 [details]
patch

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index d934c7d..6a83b21 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> +2009-07-28  Kent Tamura  <tkent@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix a bug that HTMLOptionElement::value() returns an incorrect value.
> +        https://bugs.webkit.org/show_bug.cgi?id=27760
> +
> +        * fast/forms/option-value-and-label-expected.txt: Added.
> +        * fast/forms/option-value-and-label.html: Added.
> +
>  2009-07-27  Antonio Gomes   <antonio.gomes@openbossa.org>
>  
>          Reviewed by Adam Treat.
> diff --git a/LayoutTests/fast/forms/option-value-and-label-expected.txt b/LayoutTests/fast/forms/option-value-and-label-expected.txt
> new file mode 100644
> index 0000000..99dffe3
> --- /dev/null
> +++ b/LayoutTests/fast/forms/option-value-and-label-expected.txt
> @@ -0,0 +1,17 @@
> +Test for .value and .label of OPTION element
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +PASS o1.value is "text"
> +PASS o1.label is ""
> +PASS o2.value is "value"
> +PASS o2.label is ""
> +PASS o3.value is "text"
> +PASS o3.label is "label"
> +PASS o4.value is "value"
> +PASS o4.label is "label"
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
> diff --git a/LayoutTests/fast/forms/option-value-and-label.html b/LayoutTests/fast/forms/option-value-and-label.html
> new file mode 100644
> index 0000000..afe3e30
> --- /dev/null
> +++ b/LayoutTests/fast/forms/option-value-and-label.html
> @@ -0,0 +1,41 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
> +<script src="../../fast/js/resources/js-test-pre.js"></script>
> +</head>
> +<body>
> +<p id="description"></p>
> +<div id="console"></div>
> +
> +<select>
> +<option id="o1">text</option>
> +<option id="o2" value="value">text</option>
> +<option id="o3" label="label">text</option>
> +<option id="o4" value="value" label="label">text</option>
> +</select>
> +
> +<script>
> +description('Test for .value and .label of OPTION element');
> +
> +var o1 = document.getElementById('o1');
> +shouldBe('o1.value', '"text"');
> +shouldBe('o1.label', '""');
> +
> +var o2 = document.getElementById('o2');
> +shouldBe('o2.value', '"value"');
> +shouldBe('o2.label', '""');
> +
> +var o3 = document.getElementById('o3');
> +shouldBe('o3.value', '"text"');
> +shouldBe('o3.label', '"label"');
> +
> +var o4 = document.getElementById('o4');
> +shouldBe('o4.value', '"value"');
> +shouldBe('o4.label', '"label"');
> +
> +var successfullyParsed = true;
> +</script>
> +<script src="../../fast/js/resources/js-test-post.js"></script>
> +</body>
> +</html>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 467345a..b75fe69 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,23 @@
> +2009-07-28  Kent Tamura  <tkent@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix a bug that HTMLOptionElement::value() returns an incorrect value.
> +        https://bugs.webkit.org/show_bug.cgi?id=27760
> +
> +        Test: fast/forms/option-value-and-label.html
> +
> +        * dom/OptionElement.cpp:
> +        (WebCore::OptionElement::collectOptionLabel):
> +        (WebCore::OptionElement::collectOptionText):
> +        (WebCore::OptionElement::collectOptionTextRespectingGroupLabel):
> +        (WebCore::OptionElement::collectOptionValue):
> +        * dom/OptionElement.h:
> +        * html/HTMLOptionElement.cpp:
> +        (WebCore::HTMLOptionElement::text):
> +        * wml/WMLOptionElement.cpp:
> +        (WebCore::WMLOptionElement::text):

Can you please come up with a more detailed ChangeLog, it helps in tricky cases like this..
;
> +    static String collectOptionText(const Element*, String);
Use "const String&" here.
  
>  String HTMLOptionElement::text() const
>  {
> -    return OptionElement::collectOptionText(m_data, this);
> +    return OptionElement::collectOptionLabel(m_data, this);
>  }

>  String WMLOptionElement::text() const
>  {
> -    return OptionElement::collectOptionText(m_data, this);
> +    return OptionElement::collectOptionLabel(m_data, this);
>  }

Hmm, very unfortunate that the naming is misleading nowadays, maybe this indicates we need better function names here.
I like the patch, though we clearly need a better naming scheme here, it's confusing. Or a detailed ChangeLog describing why it is as it is :-)

r-, for the ChangeLog & "const String&" issue.
Comment 4 Kent Tamura 2009-07-28 18:21:20 PDT
Created attachment 33685 [details]
patch
Comment 5 Kent Tamura 2009-07-28 18:22:56 PDT
Thank you for the comments.
I have updated the patch.
 - Improve ChangeLogs
 - Use const String& for a parameter
 - Rename method names, and restructuring some methods.
Comment 6 Nikolas Zimmermann 2009-07-28 19:29:32 PDT
Comment on attachment 33685 [details]
patch

Looks perfect, r=me.
Comment 7 Kent Tamura 2009-07-28 21:29:24 PDT
Would you commit it for me please?
I'm not a committer.
Comment 8 David Levin 2009-07-29 00:20:37 PDT
Assigned to levin for landing.
Comment 9 David Levin 2009-07-29 01:12:01 PDT
Committed as http://trac.webkit.org/changeset/46524