Bug 69455 – Option.value should trim extra internal html spaces

• Component: Forms
• Status: RESOLVED FIXED

[Sachin Puranik]

If the value content attribute is not present option.value return the textContent. As per spec, we should skip the extra internal html white-space.

<select>
<option id="o1">  text   with   extra             while   spaces  </option>
</select>

<script>
description('Test for space striping .label attribute of OPTION element');

var o1 = document.getElementById('o1');
o1.label must return "text with extra while spaces". Note that all the intermediate html spaces are trimmed.

[Sachin Puranik]

Created attachment 109848 [details]
Patch fixing the space triming issue for value attribute.

Hi,

1. Restructured the code as per comments in the bug #68684.
2. Fixed the internal space striping issue for option.value.
3. Added a test case.


One more question I have?

<select>
<option id="o1">Hello      Spaces</option>
</select>
<script>
var o = document.getElementById('o3');
alert(o.value);
o.value="";
alert(o.value);
</script>

Now first alert will be "Hello Spaces".
But after setting a "" value to option.value , it always returns "". is this correct behaviour. Now we can never go back to the original state and we will never get the option.value as  "Hello Spaces". is this correct? I feel this is inconsistent.

[Darin Adler]

Comment on attachment 109848 [details]
Patch fixing the space triming issue for value attribute.

The real issue is testing this on other browsers. I see no comments in the bug or change log that state whether this behavior is consistent with the other browsers.

[Sachin Puranik]

Hi,
Below is the behavior in the various browsers. I am taking an example for value attribute and assuming that "label" attribute also need not be any different.

Attached is the test case I have tested.

IE.(Win) ( I have tested only on IE8 will also update the comment @ the IE 9)
1. in case "value/label" content attribute is not present, it returns NULL and Not the "textContent". Does not meet spec.

Hence this is not the appropriate case to be considered for comparison.

FireFox:(Win - 7.01)
1. In case "value" attribute is not present, it returns the "textContent" with inner html spaces trimmed.( This is issue in the Webkit which is fixed with the attached patch) But after this if we set option.value="" then it starts returning "". 

Opera:(Win - 11.51)
1. In case "value" attribute is not present, it returns the "textContent" with  internal html space trimmed. But after this if we set option.value="" then it starts returning "". 

Chrome/Webkit: (Win - 12.0.742.112)
Same as opera , except a small bug. The internal html space on "textContent" is not trimmed. This is fixed in the attached patch. This change is needed in webkit.


I will update the IE9 behavior in the bug tonight.

[Sachin Puranik]

Created attachment 110344 [details]
test case I used to test with various browsers

test case to check the consistency of value attribute.

[Sachin Puranik]

Below are the spec lines for this attribute.

"The value attribute provides a value for element. The value of an option element is the value of the value content attribute, if there is one, or, if there is not, the value of the element's text IDL attribute."

All the browsers are considering the "empty string" as a VALUE for "value attribute" if set to option.value="".

[Sameer Patil]

> IE.(Win) ( I have tested only on IE8 will also update the comment @ the IE 9)
IE9 behavior is also same as IE8 here.

[Sachin Puranik]

Hi,
Can you please review this bug and updated comments.
Thanks.

[Kent Tamura]

Comment on attachment 109848 [details]
Patch fixing the space triming issue for value attribute.

View in context: https://bugs.webkit.org/attachment.cgi?id=109848&action=review

> Source/WebCore/ChangeLog:13
> +        * dom/OptionElement.cpp: code restructure.

Please write what is changed and the reason.

> Source/WebCore/ChangeLog:16
> +        (WebCore::HTMLOptionElement::value): Fixed the issue

Please write what's changed.

> Source/WebCore/dom/OptionElement.h:44
> +    virtual String label() const = 0;

Why was this added?

> Source/WebCore/html/HTMLOptionElement.cpp:227
> -    String label = m_data.label();
> -    if (!label.isNull())
> -        return label;
> - 
> -    label = collectOptionInnerText(this).stripWhiteSpace(isHTMLSpace);
> -    label = label.simplifyWhiteSpace(isHTMLSpace);
> +    if (!m_data.label().isNull())
> +        return m_data.label();
>  
> -    return label;
> +    return collectOptionInnerText(this).stripWhiteSpace(isHTMLSpace).simplifyWhiteSpace(isHTMLSpace);

This part is not related to the bug.  Don't include this part in the patch.

> LayoutTests/fast/forms/option-value-trim-html-spaces.html:4
> +<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">

Why iso-8859-1? This files contains only ASCII characters.

[Sachin Puranik]

Created attachment 111420 [details]
updated patch - Incorporating review comments. Made a patch specific to bug

Removed the unrelated code change.
Changed the test case as suggested.

[WebKit Review Bot]

Attachment 111420 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/html/HTMLOptionElement.cpp:159:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sachin Puranik]

Created attachment 111421 [details]
Fixed the Style error

Fixed the style error.

[Kent Tamura]

Comment on attachment 111421 [details]
Fixed the Style error

Looks good.

[Sachin Puranik]

Thanks for a quick review.

[WebKit Review Bot]

Comment on attachment 111421 [details]
Fixed the Style error

Clearing flags on attachment: 111421

Committed r97741: <http://trac.webkit.org/changeset/97741>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.