[Webkit-unassigned] [Bug 27760] HTMLOptionElement::value() returns incorrect value

Tue Jul 28 11:32:37 PDT 2009

https://bugs.webkit.org/show_bug.cgi?id=27760

Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33613|review?                     |review-
               Flag|                            |

--- Comment #3 from Nikolas Zimmermann <zimmermann at kde.org>  2009-07-28 11:32:37 PDT ---
(From update of attachment 33613)
> 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 at 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 at 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 at 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.