Bug 34529 - [CSSOM] issues with cssText and selectorText
: [CSSOM] issues with cssText and selectorText
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All Mac OS X 10.5
: P2 Normal
Assigned To:
: http://glazman.org/forCSSWG/selectorT...
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-03 08:09 PST by
Modified: 2010-06-08 22:08 PST (History)


Attachments
Fix for Bug 34529 - [CSSOM] issues with cssText and selectorText (21.90 KB, patch)
2010-05-18 03:58 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Style fix (21.87 KB, patch)
2010-05-18 04:07 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Add another test (22.16 KB, patch)
2010-05-18 04:19 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Use Vector<UChar> for better performance (22.81 KB, patch)
2010-05-18 20:28 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Use Vector<UChar> for better performance (22.81 KB, patch)
2010-05-18 20:29 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Try to fix Qt compilation error (24.75 KB, patch)
2010-05-18 21:09 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Add test descriptions (24.97 KB, patch)
2010-05-18 22:53 PST, Yuzo Fujishima
hamaji: review+
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 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 From 2010-03-19 00:11:34 PST -------
FWIW, http://dev.w3.org/csswg/cssom/ now defines the rules for how to serialize these things.
------- Comment #2 From 2010-05-17 22:35:34 PST -------
*** Bug 33725 has been marked as a duplicate of this bug. ***
------- Comment #3 From 2010-05-18 03:58:46 PST -------
Created an attachment (id=56354) [details]
Fix for Bug 34529 -  [CSSOM] issues with cssText and selectorText
------- Comment #4 From 2010-05-18 04:07:03 PST -------
Created an attachment (id=56357) [details]
Style fix
------- Comment #5 From 2010-05-18 04:19:41 PST -------
Created an attachment (id=56358) [details]
Add another test
------- Comment #6 From 2010-05-18 08:19:55 PST -------
(From update of attachment 56358 [details])
In general this looks good, but doing lots of String appends is inefficient. You should use StringBuilder instead.
------- Comment #7 From 2010-05-18 09:36:05 PST -------
(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 From 2010-05-18 20:28:04 PST -------
Created an attachment (id=56446) [details]
Use Vector<UChar> for better performance
------- Comment #9 From 2010-05-18 20:29:46 PST -------
Created an attachment (id=56447) [details]
Use Vector<UChar> for better performance
------- Comment #10 From 2010-05-18 20:31:12 PST -------
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 From 2010-05-18 20:38:09 PST -------
Attachment 56447 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2320319
------- Comment #12 From 2010-05-18 21:09:50 PST -------
Created an attachment (id=56450) [details]
Try to fix Qt compilation error
------- Comment #13 From 2010-05-18 21:42:45 PST -------
(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?

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 From 2010-05-18 21:56:09 PST -------
Thank you for the review.

(In reply to comment #13)
> (From update of attachment 56450 [details] [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 From 2010-05-18 22:53:23 PST -------
Created an attachment (id=56457) [details]
Add test descriptions
------- Comment #16 From 2010-06-02 21:57:48 PST -------
(From update of attachment 56457 [details])
(In reply to comment #15)
> Created an attachment (id=56457) [details] [details]
> Add test descriptions

Thanks for your description! This change looks good to me.
------- Comment #17 From 2010-06-08 21:44:37 PST -------
Committed r60880: <http://trac.webkit.org/changeset/60880>
------- Comment #18 From 2010-06-08 22:08:27 PST -------
http://trac.webkit.org/changeset/60880 might have broken Qt Linux Release