loose ends in GetSourceSelectorText()




CSS Parsing and Computation
16 years ago
13 years ago


(Reporter: rbs, Assigned: Daniel Kraft)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [good first bug])


(2 attachments, 6 obsolete attachments)



16 years ago
I had observed two bugs in GetSourceSelectorText() in the past and meant to 
file an entry in bugzilla for them (in expectation of what to do for the MathML 
bug 142592 which was just filed...).

1. GetSourceSelectorText() doesn't reset its output:
   So doing rule->GetSourceSelectorText(aSelector) in a loop does strange things
   if the (presumably nsAutoString) aSelector is declared outside the loop.

2. It doesn't quote its result, e.g., it will return
   *[color=blue] (i.e., no quotes). This is not clean when the value has
    multiple words.

Comment 1

16 years ago
nit while here, s/aIsNegated/isNegated/ in:
nsresult nsCSSSelector::ToString()
  PRBool aIsNegated = PRBool(0 < aNegatedIndex);


16 years ago
Blocks: 142592
Whiteboard: [good first bug]
Note: the code in question is in nsCSSSelector::ToStringInternal in

Comment 3

13 years ago
The problem is that the string is appended without any quotes or
backslash-escaping, ..., isn't it?
Are there already any methods which do CSS escaping? If not, I'll write one.
Where should I put it (because it may be of interest also for other use cases)
and how to name it?
> The problem is that the string is appended without any quotes or
> backslash-escaping, ..., isn't it?


> Are there already any methods which do CSS escaping?

nsROCSSPrimitiveValue::GetEscapedURI has some code that does it...

In this case, I would say the way to go is to put quotes around the value and
escape quotes in the value; the sets of allowed characters in a string and in
the thing inside url() are sufficiently different that I'm not sure
GetEscapedURI will be of much help.

Comment 5

13 years ago
Yes, put quotes around and escape. I didn't know if there's already an useable
escaping function (of course for CSS strings), so that I wouldn't have to write
code already present. So I'll write some sort of
and then do the fixing.
Putting it on nsROCSSPrimitiveValue wouldn't really help here.  Those are only
used for computed style.

Comment 7

13 years ago
So where to put? As helping function, not visible to others? But I don't think
this is really what we want...
I think a static function in this file would do just fine, actually...

Comment 9

13 years ago
Created attachment 171745 [details] [diff] [review]
First patch

This is it. But I wasn't really able to test me code further than compiling it,
because I don't know how to make Mozilla serialize a CSS rule to a file.
Attachment #171745 - Flags: review?(rbs)
Create a file with a <style> element with your rule in it, and a script that does:


That should let you test this....

Comment 11

13 years ago
It seems to work.

Comment 12

13 years ago
You should ask dbaron for r=. He may have a bigger picture in mind regarding this.

I am sure you can make the string-fu better, such as using AppendLiteral()
instead two calls to Append(ch). And "aReturn=EmptyString()" should be

Comment 13

13 years ago
Created attachment 171898 [details] [diff] [review]
Some more changes

OK, did some more changes.
Attachment #171745 - Attachment is obsolete: true
Attachment #171898 - Flags: review?(dbaron)


13 years ago
Attachment #171745 - Flags: review?(rbs)
Comment on attachment 171898 [details] [diff] [review]
Some more changes

This looks good (I'm glad somebody is fixing this), but:
 * the escaping function needs to escape CR and LF, and may as well escape
everything below 0x20 (convert to numeric escapes).  nsTextFormatter::snprintf
may be helpful.
 * this escaping function should probably go in nsStyleUtil; it's useful in
other places
Attachment #171898 - Flags: review?(dbaron) → review-

Comment 15

13 years ago
There's just one problem: When using snprintf I've to allocate a temporary
buffer (5 characters long), but I can't throw an Out-Of-Memory-Error on failure,
as the "main function", where from the escaping is used, is declared as void!

Comment 16

13 years ago
Or should I use the stack for that buffer and ignore eventual errors in snprintf?
Use the stack for the buffer.

Comment 18

13 years ago
Created attachment 172749 [details] [diff] [review]

This should escape everthing below 0x20 except of NUL. Should NUL be also
escaped (as special case), or is this not important?
Attachment #171898 - Attachment is obsolete: true
Attachment #172749 - Flags: review?(dbaron)
Comment on attachment 172749 [details] [diff] [review]

>+  if (*in < 0x20)

Actually, you should probably exclude space (" && *in != ' '")

>+   nsTextFormatter::snprintf(buf, 5, NS_LITERAL_STRING("\\%hX ").get(), *in);

use NS_ARRAY_LENGTH(buf) instead of repeating "5".

>+  } else switch (*in)
>+  {

bring the "{" up onto the same line

Attachment #172749 - Flags: review?(dbaron) → review+
Comment on attachment 172749 [details] [diff] [review]

Oh, and please convert your new function to two-space indentation for
consistency with the rest of the file, and don't add a blank line to the end of

Comment 21

13 years ago
You also need to avoid breakages by keeping existing users in sync:

    selector.Assign(NS_LITERAL_STRING("*[") + attrName +
                    NS_LITERAL_STRING("=") + attrValue +


    selector.Assign(NS_LITERAL_STRING("*[") + attrName +
                    NS_LITERAL_STRING("=\"") + attrValue +

Comment 22

13 years ago
Created attachment 172790 [details] [diff] [review]

Better now? But why should I exclude spaces, they are 0x20 - and *in<0x20 is,
in my opinion, false for spaces. Where's the problem?
Attachment #172749 - Attachment is obsolete: true
oh, right, never mind the spaces thing :-)


13 years ago
Attachment #172790 - Flags: superreview?(bzbarsky)
Comment on attachment 172790 [details] [diff] [review]

>Index: layout/style/nsCSSStyleRule.cpp
>@@ -676,37 +679,37 @@ void nsCSSSelector::ToStringInternal(nsA
>+      aString.Append(NS_LITERAL_STRING("\"]"));

Why not:



>@@ -1550,8 +1553,9 @@ NS_NewCSSStyleRule(nsICSSStyleRule** aIn
> }

David asked that this line not be added...

>Index: layout/mathml/base/src/nsMathMLFrame.cpp

>     // check for duplicate, if a similar rule is already there, don't bother to add another one
>     // XXX bug 142648 - GetSourceSelectorText is in the format *[color=blue] (i.e., no quotes...) 
>     // XXXrbs need to keep this in sync with the fix for bug 142648

Remove the XXX part of the comment?

And perhaps call nsStyleUtil::EscapeCSSString on the attrValue here, and use
the escaped value in the Assign() call, just in case?

sr=bzbarsky with those fixed.
Attachment #172790 - Flags: superreview?(bzbarsky) → superreview+

Comment 25

13 years ago
Created attachment 172895 [details] [diff] [review]

If this is ok, please may someone check this in?
Attachment #172790 - Attachment is obsolete: true
Assignee: dbaron → domob
Checked in on trunk for 1.8b.  This doesn't address problem 1 from comment 0,
does it?

Comment 27

13 years ago
No, I did nothing according to comment 1. You thought of something like
aString.Truncate() right at the beginning, or isn't it that simple?
Yeah, just a .Truncate() would do it (and checking callers to ensure they don't
depend on the current behavior).

Comment 29

13 years ago
Created attachment 172907 [details] [diff] [review]
second fix

This adds the truncate.
I've looked a bit around on lxr, the callers I found (in nsCSSStyleRule.cpp
itself and nsCSSStyleSheet.cpp) don't mind. Although, I don't think I found
every one.
Attachment #172907 - Flags: review?(bzbarsky)
Comment on attachment 172907 [details] [diff] [review]
second fix

Attachment #172907 - Flags: superreview+
Attachment #172907 - Flags: review?(bzbarsky)
Attachment #172907 - Flags: review+
Comment on attachment 172907 [details] [diff] [review]
second fix

Actually, no.  This is wrong, since it'll break linked selectors (see the mNext
call, eg).

You want to truncate in ToString(), methinks.  And test.  ;)
Attachment #172907 - Flags: superreview-
Attachment #172907 - Flags: superreview+
Attachment #172907 - Flags: review-
Attachment #172907 - Flags: review+

Comment 32

13 years ago
Created attachment 172945 [details] [diff] [review]

In ToString() it isn't possible, either, since SelectorLists aren't serialized
correctly then. So I inserted it for nsCSSSelectorList::ToString. This works,
but if nsCSSSelector::ToString may be called directly, it isn't ensured that
aString is truncated. Should I introduce a new parameter (maybe optional, so we
don't have to change any users), which controls if aString is truncated there?


13 years ago
Attachment #172907 - Attachment is obsolete: true

Comment 33

13 years ago
Created attachment 173147 [details] [diff] [review]
optional argument

This one works with things like
h1, h2+h3,
c d > c + b a[abc~='\A ']
Attachment #172945 - Attachment is obsolete: true
Attachment #173147 - Flags: review?(bzbarsky)

Comment 34

13 years ago
Simply do this, and let the recursion voodoo do the rest:

nsCSSSelector::ToString(nsAString& aString, nsICSSStyleSheet* aSheet) const
+ if (!mNext)
+   aString.Truncate();
  ToStringInternal(aString, aSheet, IsPseudoElement(mTag), 0);

Comment 35

13 years ago
Mis-placed... That conditional truncate should be in |ToStringInternal| instead.

Comment 36

13 years ago
I don't think that would work together with Selector-Lists, would it?
Yes, that would break for selector lists.

I'm going to try to look at the patch tonight.
Comment on attachment 173147 [details] [diff] [review]
optional argument

r+sr=bzbarsky, but no need for the const on the PRBool
Attachment #173147 - Flags: superreview+
Attachment #173147 - Flags: review?(bzbarsky)
Attachment #173147 - Flags: review+

Comment 39

13 years ago
My style of coding is to put as many const's as possible (because it's very
unlikely that anyone would want to change aAppend). I think this can only help
the compiler, and if not, it can't be a problem. Or am I wrong?
I think, typically, that things that are passed by value are not passed as const
in Mozilla...  I'll make that change before checking in.
In particular, const on function parameters is a little messy because of
compiler inconsistencies -- some compilers consider it part of the signature,
even though it's not supposed to be (i.e., they incorrectly consider void
foo(const int) and void foo (int) to be different signatures).

Comment 42

13 years ago
OK, do as you think. This is just what I normally do (in projects only
considered to build under cygwin GCC).
Checked in.  I believe this can be marked fixed?

Comment 44

13 years ago
I think so.
Fixed.  Thank you for the patches, Daniel!
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.