Bug 69455 - Option.value should trim extra internal html spaces
Summary: Option.value should trim extra internal html spaces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-05 12:59 PDT by Sachin Puranik
Modified: 2011-10-18 05:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch fixing the space triming issue for value attribute. (6.77 KB, patch)
2011-10-05 13:26 PDT, Sachin Puranik
tkent: review-
Details | Formatted Diff | Diff
test case I used to test with various browsers (408 bytes, text/html)
2011-10-10 03:13 PDT, Sachin Puranik
no flags Details
updated patch - Incorporating review comments. Made a patch specific to bug (5.32 KB, patch)
2011-10-18 04:09 PDT, Sachin Puranik
no flags Details | Formatted Diff | Diff
Fixed the Style error (5.33 KB, patch)
2011-10-18 04:18 PDT, Sachin Puranik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sachin Puranik 2011-10-05 12:59:18 PDT
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.
Comment 1 Sachin Puranik 2011-10-05 13:26:59 PDT
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.
Comment 2 Darin Adler 2011-10-05 13:36:59 PDT
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.
Comment 3 Sachin Puranik 2011-10-10 03:09:14 PDT
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.
Comment 4 Sachin Puranik 2011-10-10 03:13:01 PDT
Created attachment 110344 [details]
test case I used to test with various browsers

test case to check the consistency of value attribute.
Comment 5 Sachin Puranik 2011-10-10 03:25:56 PDT
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="".
Comment 6 Sameer Patil 2011-10-10 20:41:11 PDT
> IE.(Win) ( I have tested only on IE8 will also update the comment @ the IE 9)
IE9 behavior is also same as IE8 here.
Comment 7 Sachin Puranik 2011-10-13 23:23:05 PDT
Hi,
Can you please review this bug and updated comments.
Thanks.
Comment 8 Kent Tamura 2011-10-13 23:35:36 PDT
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.
Comment 9 Sachin Puranik 2011-10-18 04:09:40 PDT
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.
Comment 10 WebKit Review Bot 2011-10-18 04:12:35 PDT
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.
Comment 11 Sachin Puranik 2011-10-18 04:18:17 PDT
Created attachment 111421 [details]
Fixed the Style error

Fixed the style error.
Comment 12 Kent Tamura 2011-10-18 04:50:42 PDT
Comment on attachment 111421 [details]
Fixed the Style error

Looks good.
Comment 13 Sachin Puranik 2011-10-18 05:04:13 PDT
Thanks for a quick review.
Comment 14 WebKit Review Bot 2011-10-18 05:38:18 PDT
Comment on attachment 111421 [details]
Fixed the Style error

Clearing flags on attachment: 111421

Committed r97741: <http://trac.webkit.org/changeset/97741>
Comment 15 WebKit Review Bot 2011-10-18 05:38:23 PDT
All reviewed patches have been landed.  Closing bug.