Bug 34529 – [CSSOM] issues with cssText and selectorText

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Resent-From: 	www-style list <[e-mail]>
	From: 	Daniel Glazman <[e-mail]>
	Subject: 	[CSSOM] issues with cssText and selectorText
	Date: 	February 3, 2010 7:58:15 am PST
	To: 	www-style list <[e-mail]>
Message-Id: 	<[e-mail]>

Hi there,

I just discovered that Opera is the only "major" browser
that does not return a useless result for selectorText and
cssText in some cases that are not edge cases any more...

If the selector of a rule contains a ident itself containing
a colon, the colon must be escaped in the selector. But its
serialization in selectorText and cssText is NOT escaped. The
same issue happens with many escaped characters.

So the result of cssText and selectorText is useless because
unparseable and invalid...

FWIW, I have written a small test here

 http://glazman.org/forCSSWG/selectorTextBug.html

I said it's not an edge case any more because a small site called
Facebook gives its partners embeddable stylesheets with such selectors.
I discovered them validating a stylesheet in a famous newspaper's
website.
It's also super important to have a bijection here for editing
environments. If cssText and selectorText are unreliable, then building
a conforming style editor is almost impossible.

I think we should clarify the situation here, saying that the result
of selectorText and cssText must be parseable and represent really
what they are the serialization of...

FWIW, both Safari and Mozilla fail on the test above. MSIE of course
is not even able of running the three lines of script...

</Daniel>

[Anne van Kesteren]

FWIW, http://dev.w3.org/csswg/cssom/ now defines the rules for how to serialize these things.

[Yuzo Fujishima]

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

[Yuzo Fujishima]

Created attachment 56354 [details]
Fix for Bug 34529 -  [CSSOM] issues with cssText and selectorText

[Yuzo Fujishima]

Created attachment 56357 [details]
Style fix

[Yuzo Fujishima]

Created attachment 56358 [details]
Add another test

[Simon Fraser (smfr)]

Comment on attachment 56358 [details]
Add another test

In general this looks good, but doing lots of String appends is inefficient. You should use StringBuilder instead.

[Darin Adler]

(In reply to comment #6)
> In general this looks good, but doing lots of String appends is inefficient. You should use StringBuilder instead.

Good point! But for building strings a character at a time, StringBuilder isn't efficient either. Vector<UChar> is probably the type of choice, or even Vector<UChar, 256>.

[Yuzo Fujishima]

Created attachment 56446 [details]
Use Vector<UChar> for better performance

[Yuzo Fujishima]

Created attachment 56447 [details]
Use Vector<UChar> for better performance

[Yuzo Fujishima]

Simon, Darin,

Thank you for reviewing this.
I've changed to use Vector<UChar>. Please take another look.

(In reply to comment #7)
> (In reply to comment #6)
> > In general this looks good, but doing lots of String appends is inefficient. You should use StringBuilder instead.
> 
> Good point! But for building strings a character at a time, StringBuilder isn't efficient either. Vector<UChar> is probably the type of choice, or even Vector<UChar, 256>.

[Early Warning System Bot]

Attachment 56447 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2320319

[Yuzo Fujishima]

Created attachment 56450 [details]
Try to fix Qt compilation error

[Shinichiro Hamaji]

Comment on attachment 56450 [details]
Try to fix Qt compilation error

WebCore/css/CSSOMUtils.cpp:56
 +      append(appendTo, String::format("\\%x ", c));
Do we have test case which checks this path?

WebCore/css/CSSOMUtils.cpp:76
 +          if (c <= 0x1f || (0x30 <= c && c <= 0x39 && (isFirst || (isSecond && isFirstCharHyphen))))
Are there any reason we cannot use character literals such as '0' or '9' (or maybe L'0' or L'9') ?

[Yuzo Fujishima]

Thank you for the review.

(In reply to comment #13)
> (From update of attachment 56450 [details])
> WebCore/css/CSSOMUtils.cpp:56
>  +      append(appendTo, String::format("\\%x ", c));
> Do we have test case which checks this path?
> 

Yes. Tests 1 through 6 run the path.

> WebCore/css/CSSOMUtils.cpp:76
>  +          if (c <= 0x1f || (0x30 <= c && c <= 0x39 && (isFirst || (isSecond && isFirstCharHyphen))))
> Are there any reason we cannot use character literals such as '0' or '9' (or maybe L'0' or L'9') ?

I used character code in hopes of making it easier to see if the
function truthfully implements http://dev.w3.org/csswg/cssom/#common-serializing-idioms

[Yuzo Fujishima]

Created attachment 56457 [details]
Add test descriptions

[Shinichiro Hamaji]

Comment on attachment 56457 [details]
Add test descriptions

(In reply to comment #15)
> Created an attachment (id=56457) [details]
> Add test descriptions

Thanks for your description! This change looks good to me.

[Yuzo Fujishima]

Committed r60880: <http://trac.webkit.org/changeset/60880>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60880 might have broken Qt Linux Release