Bug 57746 – HTMLOptionElement::text incorrectly normalize U+3000 to U+0020.

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

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.

[Shinya Kawanaka]

Created attachment 101863 [details]
Patch

[Kent Tamura]

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.

[Kent Tamura]

(In reply to comment #2)
> We already have isHTMLSpace() in WebCore/html/parserHTMLParserIdioms.h.

WebCore/html/parser/HTMLParserIdioms.h.

[Shinya Kawanaka]

Created attachment 101975 [details]
Patch

[Kent Tamura]

Comment on attachment 101975 [details]
Patch

ok.
Thank you for fixing this.

[WebKit Review Bot]

Comment on attachment 101975 [details]
Patch

Clearing flags on attachment: 101975

Committed r91746: <http://trac.webkit.org/changeset/91746>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Did you realized if you broke the Snow Leopard build?
Are you working on fixing it now?

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/91756

[Mihnea Ovidenie]

(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,

[Csaba Osztrogonác]

rs=me to land it again with this fix. Thanks for the fix.

[Shinya Kawanaka]

Created attachment 102127 [details]
Patch

[Kent Tamura]

Comment on attachment 102127 [details]
Patch

ok

[Shinya Kawanaka]

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

[WebKit Review Bot]

Comment on attachment 102127 [details]
Patch

Clearing flags on attachment: 102127

Committed r91837: <http://trac.webkit.org/changeset/91837>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Kent Tamura]

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

[Kent Tamura]

*** Bug 38226 has been marked as a duplicate of this bug. ***