Bug 57746 - HTMLOptionElement::text incorrectly normalize U+3000 to U+0020.
Summary: HTMLOptionElement::text incorrectly normalize U+3000 to U+0020.
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:
: 38226 (view as bug list)
Depends on: 65180
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-04 01:24 PDT by Kent Tamura
Modified: 2011-10-11 02:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (28.04 KB, patch)
2011-07-25 05:19 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (27.35 KB, patch)
2011-07-26 00:02 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (28.10 KB, patch)
2011-07-27 04:52 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-04-04 01:24:00 PDT
Reported in Chrome Help Forum (Japanese): http://www.google.com/support/forum/p/chrome/thread?tid=1e819edbe70751b4&hl=ja

Specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#dom-option-text
> must return the value of the textContent IDL attribute on the element with leading and
> trailing space characters removed, and with any sequences of two or more space characters
> replaced by a single U+0020 SPACE character.

> The space characters, for the purposes of this specification, are U+0020 SPACE,
> U+0009 CHARACTER TABULATION (tab), U+000A LINE FEED (LF), U+000C FORM FEED (FF),
> and U+000D CARRIAGE RETURN (CR).

We use isSpaceOrNewLine() for HTMLOptionElement::text.
Comment 1 Shinya Kawanaka 2011-07-25 05:19:37 PDT
Created attachment 101863 [details]
Patch
Comment 2 Kent Tamura 2011-07-25 22:45:43 PDT
Comment on attachment 101863 [details]
Patch

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

> LayoutTests/fast/forms/option-strip-unicode-spaces.html:11
> +<script src="script-tests/option-strip-unicode-spaces.js"></script>

You don't need to have a separated JavaScript file.  Please embed the JavaScript code here.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:3
> +// see http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character

Start the comment with a capital letter.
// See http:...

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:4
> +var spaces = [

The name should be 'HTMLSpaces' or something.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:15
> +var nonSpaces = [

The name should be 'nonHTMLSpaces' or something.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:47
> +debug('insert one space before/after/between the text');

A heading should be start with a capital letter.

> LayoutTests/fast/forms/script-tests/option-strip-unicode-spaces.js:67
> +debug('insert one non space before/after/between the text');

'one non space' is confusing.  'one non-HTML space' might be better.

> Source/WebCore/dom/OptionElement.cpp:38
> +// For removing whitespaces of text attribute in option element, only these 5 characters are treated as space.
> +// See also http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#dom-option-text
> +static bool isOptionTextSpace(UChar c)
> +{

We already have isHTMLSpace() in WebCore/html/parserHTMLParserIdioms.h.
Comment 3 Kent Tamura 2011-07-25 22:46:39 PDT
(In reply to comment #2)
> We already have isHTMLSpace() in WebCore/html/parserHTMLParserIdioms.h.

WebCore/html/parser/HTMLParserIdioms.h.
Comment 4 Shinya Kawanaka 2011-07-26 00:02:41 PDT
Created attachment 101975 [details]
Patch
Comment 5 Kent Tamura 2011-07-26 00:57:02 PDT
Comment on attachment 101975 [details]
Patch

ok.
Thank you for fixing this.
Comment 6 WebKit Review Bot 2011-07-26 02:10:33 PDT
Comment on attachment 101975 [details]
Patch

Clearing flags on attachment: 101975

Committed r91746: <http://trac.webkit.org/changeset/91746>
Comment 7 WebKit Review Bot 2011-07-26 02:10:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Csaba Osztrogonác 2011-07-26 07:15:40 PDT
Did you realized if you broke the Snow Leopard build?
Are you working on fixing it now?
Comment 9 Csaba Osztrogonác 2011-07-26 08:06:01 PDT
Reopen, because it was rolled out by http://trac.webkit.org/changeset/91756
Comment 10 Mihnea Ovidenie 2011-07-26 08:09:07 PDT
(In reply to comment #9)
> Reopen, because it was rolled out by http://trac.webkit.org/changeset/91756

Hi,

I fixed the build on my machine by adding the new functions to JavaScriptCore.exp:

diff --git a/Source/JavaScriptCore/JavaScriptCore.exp b/Source/JavaScriptCore/JavaScriptCore.exp
index 4a7a3db..d6995a6 100644
--- a/Source/JavaScriptCore/JavaScriptCore.exp
+++ b/Source/JavaScriptCore/JavaScriptCore.exp
@@ -584,9 +584,11 @@ __ZNK3WTF6String12toUIntStrictEPbi
 __ZNK3WTF6String13toInt64StrictEPbi
 __ZNK3WTF6String14threadsafeCopyEv
 __ZNK3WTF6String15stripWhiteSpaceEv
+__ZNK3WTF6String15stripWhiteSpaceEPFbtE
 __ZNK3WTF6String16removeCharactersEPFbtE
 __ZNK3WTF6String17crossThreadStringEv
 __ZNK3WTF6String18simplifyWhiteSpaceEv
+__ZNK3WTF6String18simplifyWhiteSpaceEPFbtE
 __ZNK3WTF6String19characterStartingAtEj
 __ZNK3WTF6String4utf8Eb
 __ZNK3WTF6String5asciiEv

Regards,
Comment 11 Csaba Osztrogonác 2011-07-26 08:12:45 PDT
rs=me to land it again with this fix. Thanks for the fix.
Comment 12 Shinya Kawanaka 2011-07-27 04:52:54 PDT
Created attachment 102127 [details]
Patch
Comment 13 Kent Tamura 2011-07-27 04:54:25 PDT
Comment on attachment 102127 [details]
Patch

ok
Comment 14 Shinya Kawanaka 2011-07-27 05:00:49 PDT
(In reply to comment #9)
> Reopen, because it was rolled out by http://trac.webkit.org/changeset/91756

Csaba,

Sorry for breaking the build. I have enhanced my workflow less likely to break the build.
Anyway, I resubmit the patch which should be buildable in apple port on Snow Leopard.
Comment 15 WebKit Review Bot 2011-07-27 06:07:55 PDT
Comment on attachment 102127 [details]
Patch

Clearing flags on attachment: 102127

Committed r91837: <http://trac.webkit.org/changeset/91837>
Comment 16 WebKit Review Bot 2011-07-27 06:08:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Darin Adler 2011-07-27 12:13:31 PDT
Comment on attachment 102127 [details]
Patch

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

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:319
>  PassRefPtr<StringImpl> StringImpl::stripWhiteSpace()
>  {
> +    return stripWhiteSpace(isSpaceOrNewline);
> +}

I think it will be a significant slowdown to call a function for every character, so I think this is not a good change for existing callers of this function.
Comment 18 Kent Tamura 2011-07-27 20:51:36 PDT
(In reply to comment #17)
> >  PassRefPtr<StringImpl> StringImpl::stripWhiteSpace()
> >  {
> > +    return stripWhiteSpace(isSpaceOrNewline);
> > +}
> 
> I think it will be a significant slowdown to call a function for every character, so I think this is not a good change for existing callers of this function.

ok, let's address this slowdown issue in Bug 65300.
Comment 19 Kent Tamura 2011-10-11 02:24:09 PDT
*** Bug 38226 has been marked as a duplicate of this bug. ***