Bug 94633 – CSSStyleDeclaration.cssText should not contain extraneous whitespace in final delimiter

• Component: CSS
• Status: RESOLVED FIXED

[Glenn Adams]

According to newly drafted text in CSSOM [1] for serializing CSS style declarations block (i.e., CSSStyleDeclaration), no whitespace should follow the final SEMICOLON in a non-empty value returned by the cssText getter. At present, other common browsers (FF, IE, Opera) either follow this behavior or have adopted it for an upcoming revision (Opera) [2]. However, WK is returning an extra SPACE after the SEMICOLON at the end of non-empty cssText. An incoming CSSWG test for this behavior can be found at [3].

[1] http://dvcs.w3.org/hg/csswg/raw-file/tip/cssom/Overview.html#serialize-a-css-declaration-block
[2] http://lists.w3.org/Archives/Public/www-style/2012Jul/0675.html
[3] http://hg.csswg.org/test/raw-file/tip/contributors/gadams/incoming/cssom/cssstyledeclaration-csstext-final-delimiter.xht

[Glenn Adams]

Created attachment 159768 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159768 [details]
Patch

Attachment 159768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13551462

New failing tests:
editing/pasteboard/data-transfer-items.html
fast/events/ondrop-text-html.html
editing/pasteboard/onpaste-text-html.html

[WebKit Review Bot]

Created attachment 159776 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 159768 [details]
Patch

Attachment 159768 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13564021

[Glenn Adams]

Comment on attachment 159768 [details]
Patch

remove review request pending fix for cr-linux and win fails

[Glenn Adams]

Created attachment 159796 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159796 [details]
Patch

Attachment 159796 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13567069

New failing tests:
fast/events/ondrop-text-html.html
editing/pasteboard/onpaste-text-html.html

[WebKit Review Bot]

Created attachment 159833 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Glenn Adams]

Created attachment 160070 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 160070 [details]
Patch

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

> Source/WebCore/css/CSSStyleSheet.cpp:305
> +    String text = selector;
> +    text += " { ";
> +    text += style;
> +    if (!style.isEmpty())
> +        text += " ";
> +    text += "}";
> +    insertRule(text, index, ec);

Although the existing code uses it a lot, using String += is very inefficient. For new code we try to use StringBuilder.

[Benjamin Poulain]

Comment on attachment 160070 [details]
Patch

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

> Although the existing code uses it a lot, using String += is very inefficient. For new code we try to use StringBuilder.

+1 to that!

Please use preferably the string operators or StringBuilder. String::append() and String::operator+=() are very inefficient for adding several strings.

> Source/WebCore/css/StylePropertySet.cpp:669
> +                result.append(" ");

This should be 
result.append(' ');

> Source/WebCore/css/StylePropertySet.cpp:827
> +            result.append(" ");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:832
> +        result.append(";");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:841
> +            result.append(" ");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:852
> +        result.append(";");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:856
> +                result.append(" ");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:861
> +                result.append(" ");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:869
> +            result.append(" ");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:880
> +        result.append(";");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:884
> +                result.append(" ");

ditto.

> Source/WebCore/css/StylePropertySet.cpp:889
> +                result.append(" ");

ditto.

[Benjamin Poulain]

Comment on attachment 160070 [details]
Patch

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

Some more nitpick :)

> Source/WebCore/css/CSSFontFaceRule.cpp:57
> +      result += " ";

Indent is wrong here.

> Source/WebCore/css/CSSPageRule.cpp:88
> +      result += " ";

Indent.

> Source/WebCore/css/CSSProperty.cpp:56
> +    return cssName() + ": " + m_value->cssText() + (isImportant() ? " !important" : "") + ";";

";" -> ';'

> Source/WebCore/css/CSSStyleRule.cpp:121
> +      result += " ";

Indent.

> Source/WebCore/css/WebKitCSSKeyframeRule.cpp:86
> +      result += " ";

Indent.

[Glenn Adams]

Comment on attachment 160070 [details]
Patch

remove cq? pending minor fixes per comments

[Glenn Adams]

Created attachment 160368 [details]
Patch

[Glenn Adams]

Comment on attachment 160368 [details]
Patch

update to use StringBuilder per comments; fix indents;

[Benjamin Poulain]

Comment on attachment 160368 [details]
Patch

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

Simon had already reviewed this. The strings fix looks good to me, so let's r+ again.

Thank you for updating the patch for my comments after Simon's review.

> Source/WebCore/css/CSSProperty.cpp:65
> -    return cssName() + ": " + m_value->cssText() + (isImportant() ? " !important" : "") + "; ";
> +    StringBuilder result;
> +    result.append(cssName());
> +    result.appendLiteral(": ");
> +    result.append(m_value->cssText());
> +    if (isImportant())
> +        result.appendLiteral(" !important");
> +    result.append(';');
> +    return result.toString();

Using "operator+" was actually ok here. The StringOperators do a pretty good job.
Just a note though, no problem with using StringBuilder here.

[WebKit Review Bot]

Comment on attachment 160368 [details]
Patch

Clearing flags on attachment: 160368

Committed r126656: <http://trac.webkit.org/changeset/126656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Glenn Adams]

retroactively adding WebExposed keyword, since this patch affects the values returned from CSSStyleDeclaration.cssText; authors who previously relied upon WK's insertion of an extraneous space will need to account for this space no longer appearing after a final (semicolon) delimiter

[Alexey Proskuryakov]

This has caused bug 102435.