Bug 34529 - [CSSOM] issues with cssText and selectorText
Summary: [CSSOM] issues with cssText and selectorText
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Yuzo Fujishima
URL: http://glazman.org/forCSSWG/selectorT...
Keywords:
: 33725 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-02-03 08:09 PST by Simon Fraser (smfr)
Modified: 2010-06-08 22:08 PDT (History)
11 users (show)

See Also:


Attachments
Fix for Bug 34529 - [CSSOM] issues with cssText and selectorText (21.90 KB, patch)
2010-05-18 03:58 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Style fix (21.87 KB, patch)
2010-05-18 04:07 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Add another test (22.16 KB, patch)
2010-05-18 04:19 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Use Vector<UChar> for better performance (22.81 KB, patch)
2010-05-18 20:28 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Use Vector<UChar> for better performance (22.81 KB, patch)
2010-05-18 20:29 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Try to fix Qt compilation error (24.75 KB, patch)
2010-05-18 21:09 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Add test descriptions (24.97 KB, patch)
2010-05-18 22:53 PDT, Yuzo Fujishima
hamaji: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-02-03 08:09:17 PST
Resent-From: 	www-style list <www-style@w3.org>
	From: 	Daniel Glazman <daniel.glazman@disruptive-innovations.com>
	Subject: 	[CSSOM] issues with cssText and selectorText
	Date: 	February 3, 2010 7:58:15 am PST
	To: 	www-style list <www-style@w3.org>
Message-Id: 	<4B699D17.5060009@disruptive-innovations.com>

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>
Comment 1 Anne van Kesteren 2010-03-19 00:11:34 PDT
FWIW, http://dev.w3.org/csswg/cssom/ now defines the rules for how to serialize these things.
Comment 2 Yuzo Fujishima 2010-05-17 22:35:34 PDT
*** Bug 33725 has been marked as a duplicate of this bug. ***
Comment 3 Yuzo Fujishima 2010-05-18 03:58:46 PDT
Created attachment 56354 [details]
Fix for Bug 34529 -  [CSSOM] issues with cssText and selectorText
Comment 4 Yuzo Fujishima 2010-05-18 04:07:03 PDT
Created attachment 56357 [details]
Style fix
Comment 5 Yuzo Fujishima 2010-05-18 04:19:41 PDT
Created attachment 56358 [details]
Add another test
Comment 6 Simon Fraser (smfr) 2010-05-18 08:19:55 PDT
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.
Comment 7 Darin Adler 2010-05-18 09:36:05 PDT
(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>.
Comment 8 Yuzo Fujishima 2010-05-18 20:28:04 PDT
Created attachment 56446 [details]
Use Vector<UChar> for better performance
Comment 9 Yuzo Fujishima 2010-05-18 20:29:46 PDT
Created attachment 56447 [details]
Use Vector<UChar> for better performance
Comment 10 Yuzo Fujishima 2010-05-18 20:31:12 PDT
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>.
Comment 11 Early Warning System Bot 2010-05-18 20:38:09 PDT
Attachment 56447 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2320319
Comment 12 Yuzo Fujishima 2010-05-18 21:09:50 PDT
Created attachment 56450 [details]
Try to fix Qt compilation error
Comment 13 Shinichiro Hamaji 2010-05-18 21:42:45 PDT
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') ?
Comment 14 Yuzo Fujishima 2010-05-18 21:56:09 PDT
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
Comment 15 Yuzo Fujishima 2010-05-18 22:53:23 PDT
Created attachment 56457 [details]
Add test descriptions
Comment 16 Shinichiro Hamaji 2010-06-02 21:57:48 PDT
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.
Comment 17 Yuzo Fujishima 2010-06-08 21:44:37 PDT
Committed r60880: <http://trac.webkit.org/changeset/60880>
Comment 18 WebKit Review Bot 2010-06-08 22:08:27 PDT
http://trac.webkit.org/changeset/60880 might have broken Qt Linux Release