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>
FWIW, http://dev.w3.org/csswg/cssom/ now defines the rules for how to serialize these things.
*** Bug 33725 has been marked as a duplicate of this bug. ***
Created attachment 56354 [details] Fix for Bug 34529 - [CSSOM] issues with cssText and selectorText
Created attachment 56357 [details] Style fix
Created attachment 56358 [details] Add another test
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.
(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>.
Created attachment 56446 [details] Use Vector<UChar> for better performance
Created attachment 56447 [details] Use Vector<UChar> for better performance
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>.
Attachment 56447 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2320319
Created attachment 56450 [details] Try to fix Qt compilation error
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') ?
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
Created attachment 56457 [details] Add test descriptions
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.
Committed r60880: <http://trac.webkit.org/changeset/60880>
http://trac.webkit.org/changeset/60880 might have broken Qt Linux Release