543428 - selectorText and cssText strip backslash from "\:" in type element selectors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

In a selector contains a type element selector itself containing backslash+colon because the element name contains a colon, the output of that selector through the CSS OM (cssText or selectorText) returns a version of the selector without the backslash. It's then impossible to reuse the selector or the associated rule.
nsCSSSelector::AppendToStringWithoutCombinatorsOrNegations is faulty here, it
should not append tag names through toString() but through another method dealing
with characters inside tag names that can exist in tag names and must be escaped
in CSS idents.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that the same thing needs to happen for namespace prefixes.  And IDs. And classnames.  And pseudoclass names.

What else should this apply to?  Which chars need to be escaped?

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #1)
> Note that the same thing needs to happen for namespace prefixes.  And IDs. And
> classnames.  And pseudoclass names.
> 
> What else should this apply to?  Which chars need to be escaped?

I did not have enough to dig, but I suspect that escaped sequences representing unicode chars are replaced by the char itself... An escape sequence representing a acute-e é would be serialized into a é making the serialization invalid again :-( I think we probably have to escape all chars outside of - _ a-Z and A-z, and if they're outside of the ascii range turn the result into an escaped hex sequence.

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Please note that Opera seems to escape all chars outside of [\-_a-zA-Z] with an hex sequence.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We need a function for serializing an identifier.  I thought we had a bug on this somewhere, but I guess not.  It could probably live right next to nsStyleUtil::AppendEscapedCSSString.

(In reply to comment #1)
> Note that the same thing needs to happen for namespace prefixes.  And IDs. And
> classnames.  And pseudoclass names.
> 
> What else should this apply to?  Which chars need to be escaped?

Also values of a whole bunch of properties that take arbitrary identifiers, such as counter-*, font-family (sort of), etc.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and all characters from U+80 and higher are all valid in identifiers; we don't need to escape them.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This should address the bulk of the problems.  The only thing I know I've missed is 'font-family', although it wouldn't surprise me if there's more.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And yes, I know the computed style code is ugly, but that's what we get for shoehorning all the computed style values into a broken API that doesn't do things like distinguish string vs. identifier.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

The previous patch unnecessarily escaped non-ascii characters.  This fixes it, and adds some tests that we don't over-escape.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note to self; the "escape digits when first == true" code needs to escape '-' as well.  Will get back to this after dinner.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> Note to self; the "escape digits when first == true" code needs to escape '-'
> as well.  Will get back to this after dinner.

No, I don't think it does.  '-' gets escaped in the code below as '\-'; it doesn't need *numerical* escaping, which is what that clause is for.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> it doesn't need *numerical* escaping

Ah, indeed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 425077 [details] [diff] [review]
patch

>+++ b/layout/style/nsStyleUtil.cpp
>+    } else {
>+      if (!((*in == PRUnichar('_')) ||

Might be worth it to stick *in in a local at the top of that else block; may be clearer and might optimize better.

r=bzbarsky with that.  I have no problem with us adding a SetIdent API for computed style in a different bug, btw.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/41394e846c1b

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I added a note to bug 280443 about the "XXX" comment introduced in this patch.