Bug 94633 - CSSStyleDeclaration.cssText should not contain extraneous whitespace in final delimiter
: CSSStyleDeclaration.cssText should not contain extraneous whitespace in final...
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: WebExposed
:
:
  Show dependency treegraph
 
Reported: 2012-08-21 13:35 PST by
Modified: 2012-11-16 11:05 PST (History)


Attachments
Patch (139.78 KB, patch)
2012-08-21 14:13 PST, Glenn Adams
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (1.34 MB, application/zip)
2012-08-21 15:11 PST, WebKit Review Bot
no flags Details
Patch (145.27 KB, patch)
2012-08-21 16:31 PST, Glenn Adams
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (430.60 KB, application/zip)
2012-08-21 18:19 PST, WebKit Review Bot
no flags Details
Patch (146.33 KB, patch)
2012-08-22 19:36 PST, Glenn Adams
no flags Review Patch | Details | Formatted Diff | Diff
Patch (150.14 KB, patch)
2012-08-24 02:52 PST, Glenn Adams
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-08-21 13:35:34 PST
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
------- Comment #1 From 2012-08-21 14:13:19 PST -------
Created an attachment (id=159768) [details]
Patch
------- Comment #2 From 2012-08-21 15:11:38 PST -------
(From update of attachment 159768 [details])
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
------- Comment #3 From 2012-08-21 15:11:44 PST -------
Created an attachment (id=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
------- Comment #4 From 2012-08-21 15:33:49 PST -------
(From update of attachment 159768 [details])
Attachment 159768 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13564021
------- Comment #5 From 2012-08-21 15:41:36 PST -------
(From update of attachment 159768 [details])
remove review request pending fix for cr-linux and win fails
------- Comment #6 From 2012-08-21 16:31:46 PST -------
Created an attachment (id=159796) [details]
Patch
------- Comment #7 From 2012-08-21 18:19:27 PST -------
(From update of attachment 159796 [details])
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
------- Comment #8 From 2012-08-21 18:19:32 PST -------
Created an attachment (id=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
------- Comment #9 From 2012-08-22 19:36:36 PST -------
Created an attachment (id=160070) [details]
Patch
------- Comment #10 From 2012-08-23 20:57:33 PST -------
(From update of attachment 160070 [details])
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.
------- Comment #11 From 2012-08-23 23:28:42 PST -------
(From update of attachment 160070 [details])
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.
------- Comment #12 From 2012-08-24 00:20:32 PST -------
(From update of attachment 160070 [details])
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.
------- Comment #13 From 2012-08-24 00:33:26 PST -------
(From update of attachment 160070 [details])
remove cq? pending minor fixes per comments
------- Comment #14 From 2012-08-24 02:52:27 PST -------
Created an attachment (id=160368) [details]
Patch
------- Comment #15 From 2012-08-24 05:55:16 PST -------
(From update of attachment 160368 [details])
update to use StringBuilder per comments; fix indents;
------- Comment #16 From 2012-08-24 16:44:07 PST -------
(From update of attachment 160368 [details])
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.
------- Comment #17 From 2012-08-24 16:58:23 PST -------
(From update of attachment 160368 [details])
Clearing flags on attachment: 160368

Committed r126656: <http://trac.webkit.org/changeset/126656>
------- Comment #18 From 2012-08-24 16:58:29 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #19 From 2012-09-02 21:57:33 PST -------
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
------- Comment #20 From 2012-11-16 11:05:53 PST -------
This has caused bug 102435.