Bug 27760

Summary:

HTMLOptionElement::value() returns incorrect value

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Forms

Assignee:

David Levin <levin>

Status:

RESOLVED FIXED

Severity:

Major

CC:

levin, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	none
patch	zimmermann: review+

Kent Tamura

Reported 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.

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 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.

Kent Tamura

Comment 2 2009-07-28 03:48:47 PDT

Created attachment 33613 [details] patch

Nikolas Zimmermann

Comment 3 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.

Kent Tamura

Comment 4 2009-07-28 18:21:20 PDT

Created attachment 33685 [details] patch

Kent Tamura

Comment 5 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.

Nikolas Zimmermann

Comment 6 2009-07-28 19:29:32 PDT

Comment on attachment 33685 [details] patch Looks perfect, r=me.

Kent Tamura

Comment 7 2009-07-28 21:29:24 PDT

Would you commit it for me please? I'm not a committer.

David Levin

Comment 8 2009-07-29 00:20:37 PDT

Assigned to levin for landing.

David Levin

Comment 9 2009-07-29 01:12:01 PDT

Committed as http://trac.webkit.org/changeset/46524

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