Summary: | Option.value should trim extra internal html spaces | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sachin Puranik <jcqt43> | ||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, darin, mkrp87, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Sachin Puranik
2011-10-05 12:59:18 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 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.
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. Created attachment 110344 [details]
test case I used to test with various browsers
test case to check the consistency of value attribute.
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="". > IE.(Win) ( I have tested only on IE8 will also update the comment @ the IE 9)
IE9 behavior is also same as IE8 here.
Hi, Can you please review this bug and updated comments. Thanks. 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. 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.
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.
Created attachment 111421 [details]
Fixed the Style error
Fixed the style error.
Comment on attachment 111421 [details]
Fixed the Style error
Looks good.
Thanks for a quick review. Comment on attachment 111421 [details] Fixed the Style error Clearing flags on attachment: 111421 Committed r97741: <http://trac.webkit.org/changeset/97741> All reviewed patches have been landed. Closing bug. |