The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: glazou, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.3a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
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?
Component: DOM: CSS Object Model → Style System (CSS)
QA Contact: general → style-system
(Reporter)

Comment 2

7 years ago
(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.
(Reporter)

Comment 3

7 years ago
Please note that Opera seems to escape all chars outside of [\-_a-zA-Z] with an hex sequence.
(Assignee)

Comment 4

7 years ago
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.
Blocks: 475216
(Assignee)

Comment 5

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

Updated

7 years ago
Assignee: nobody → dbaron
Priority: -- → P3
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Comment 6

7 years ago
Created attachment 425068 [details] [diff] [review]
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.
Attachment #425068 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #425068 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

7 years ago
Created attachment 425077 [details] [diff] [review]
patch

The previous patch unnecessarily escaped non-ascii characters.  This fixes it, and adds some tests that we don't over-escape.
Attachment #425068 - Attachment is obsolete: true
Attachment #425077 - Flags: review?(bzbarsky)
Note to self; the "escape digits when first == true" code needs to escape '-' as well.  Will get back to this after dinner.
(Assignee)

Comment 10

7 years ago
(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.
> it doesn't need *numerical* escaping

Ah, indeed.
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.
Attachment #425077 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

7 years ago
http://hg.mozilla.org/mozilla-central/rev/41394e846c1b
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

7 years ago
I added a note to bug 280443 about the "XXX" comment introduced in this patch.
You need to log in before you can comment on or make changes to this bug.