142648 - loose ends in GetSourceSelectorText()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[rbs]

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

[rbs]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note: the code in question is in nsCSSSelector::ToStringInternal in
nsCSSStyleRule.cpp.

Comment 3

[Daniel Kraft]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> The problem is that the string is appended without any quotes or
> backslash-escaping, ..., isn't it?

Absolutely.

> 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

[Daniel Kraft]

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
nsROCSSPrimitiveValue::GetEscapedString
and then do the fixing.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Putting it on nsROCSSPrimitiveValue wouldn't really help here.  Those are only
used for computed style.

Comment 7

[Daniel Kraft]

So where to put? As helping function, not visible to others? But I don't think
this is really what we want...

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think a static function in this file would do just fine, actually...

Comment 9

[Daniel Kraft]

Attachment: 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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Create a file with a <style> element with your rule in it, and a script that does:

  alert(document.styleSheets[0].cssRules[0].cssText);

That should let you test this....

Comment 11

[Daniel Kraft]

It seems to work.

Comment 12

[rbs]

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
aReturn.Truncate().

Comment 13

[Daniel Kraft]

Attachment: Some more changes

OK, did some more changes.

Comment 14

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

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

Comment 15

[Daniel Kraft]

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

[Daniel Kraft]

Or should I use the stack for that buffer and ignore eventual errors in snprintf?

Comment 17

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

Use the stack for the buffer.

Comment 18

[Daniel Kraft]

Attachment: patch

This should escape everthing below 0x20 except of NUL. Should NUL be also
escaped (as special case), or is this not important?

Comment 19

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

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

>+  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

r=dbaron

Comment 20

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

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

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
nsCSSStyleRule.cpp

Comment 21

[rbs]

You also need to avoid breakages by keeping existing users in sync:
http://lxr.mozilla.org/mozilla/source/layout/mathml/base/src/nsMathMLFrame.cpp#661

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

becomes

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

Comment 22

[Daniel Kraft]

Attachment: corrected

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?

Comment 23

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

oh, right, never mind the spaces thing :-)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172790 [details] [diff] [review]
corrected

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

Why not:

  aString.AppendLiteral("\"]");

?

>@@ -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.

Comment 25

[Daniel Kraft]

Attachment: corrected

If this is ok, please may someone check this in?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk for 1.8b.  This doesn't address problem 1 from comment 0,
does it?

Comment 27

[Daniel Kraft]

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?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yeah, just a .Truncate() would do it (and checking callers to ensure they don't
depend on the current behavior).

Comment 29

[Daniel Kraft]

Attachment: 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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172907 [details] [diff] [review]
second fix

r+sr=bzbarsky

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 32

[Daniel Kraft]

Attachment: patch

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?

Comment 33

[Daniel Kraft]

Attachment: optional argument

This one works with things like
h1, h2+h3,
c d > c + b a[abc~='\A ']

Comment 34

[rbs]

Simply do this, and let the recursion voodoo do the rest:

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

Comment 35

[rbs]

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

Comment 36

[Daniel Kraft]

I don't think that would work together with Selector-Lists, would it?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Yes, that would break for selector lists.

I'm going to try to look at the patch tonight.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173147 [details] [diff] [review]
optional argument

r+sr=bzbarsky, but no need for the const on the PRBool

Comment 39

[Daniel Kraft]

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?

Comment 40

[Boris Zbarsky [:bzbarsky]]

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.

Comment 41

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

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

[Daniel Kraft]

OK, do as you think. This is just what I normally do (in projects only
considered to build under cygwin GCC).

Comment 43

[Boris Zbarsky [:bzbarsky]]

Checked in.  I believe this can be marked fixed?

Comment 44

[Daniel Kraft]

I think so.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Fixed.  Thank you for the patches, Daniel!