955860 - Implement `CSS.escape` as per CSSOM

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Mathias Bynens]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.55 Safari/537.36 OPR/19.0.1326.21 (Edition Next)

Steps to reproduce:

Spec: http://dev.w3.org/csswg/cssom/#the-css.escape%28%29-method
Polyfill: http://mths.be/cssescape
Tests: https://github.com/mathiasbynens/CSS.escape/blob/master/tests/tests.js

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You can file bugs in Core, you know :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

nsStyleUtil::AppendEscapedCSSIdent basically does what's specced here, except it will escape U+0000 as "\0 " instead of throwing.

I'd say we just use it as-is for now; I don't see the point of throwing on U+0000.

This is a good simple bug for someone to get their feet wet....

Comment 3

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

The point is that \0 is no longer a valid escape, so that it's not possible to introduce nulls into CSS strings or identifiers.  Seems simple enough to add a boolean to the function to tell it whether to throw on nulls or not.

Comment 4

[Boris Zbarsky [:bzbarsky]]

In that case, is there a use for the "non-throwing" behavior?  We could just add a boolean return value, true for no nulls, false for null encountered, and ignore it at all the current "we know there is no null" callsites, instead of adding an argument.

Comment 5

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

Yeah, that works too, and still doesn't presume confidence that there are no nulls.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Tests: https://github.com/mathiasbynens/CSS.escape/blob/master/tests/tests.js

Those seem to require some sort of "assert" module that's not in the repo, right?   Might be nice to have them use the W3C testharness instead.

Comment 7

[Mathias Bynens]

(In reply to Boris Zbarsky [:bz] from comment #6)
> > Tests: https://github.com/mathiasbynens/CSS.escape/blob/master/tests/tests.js
> 
> Those seem to require some sort of "assert" module that's not in the repo,
> right?

Yeah, the built-in `assert` module that ships with Node.js: http://nodejs.org/api/assert.html

> Might be nice to have them use the W3C testharness instead.

That would be fairly trivial to do:

1. `s/assertEquals/assert_equals/g`;
2. Rewrite the few instances of `assertThrows` to match the testharness `assert_throws` signature.

Comment 8

[sbetageri111]

Attachment: cssEscape.patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361090 [details] [diff] [review]
cssEscape.patch

>+++ b/layout/style/CSS.cpp
>+CSS::Escape(const GlobalObject& aGlobal,const nsAString& aIdent,

Space after comma, please.

>+  bool nullVal = nsStyleUtil::AppendEscapedCSSIdent(aIdent, aReturn);

How about "success" instead?  The boolean is _not_ true when something null, after all...

>+  //nullVal is false if it has aIdentifier has a U+0000 character in it

Fix the grammar and punctuation here?

>+  else
>+    return;

No need for the explicit return.

>+++ b/layout/style/CSS.h

Please fix the indentation of the code being added to this file.

>+++ b/layout/style/nsStyleUtil.cpp
>-
>   // Escape a digit at the start (including after a dash),

Please don't remove that blank line.

>+    if(ch == 0x00)
>+      return false;

Curlies around the if body, and space before "(", please.

>+++ b/layout/style/nsStyleUtil.h

Need to document the return value.

r=me with those nits fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and this still needs a mochitest.

Comment 11

[sbetageri111]

Attachment: cssEscape1.patch

Comment 12

[sbetageri111]

Attachment: cssEscape1.patch

A closing bracket is missing in this patch. Patch cssEscape2 rectifies it.

Comment 13

[sbetageri111]

Attachment: cssEscape2.patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361480 [details] [diff] [review]
cssEscape1.patch

>+++ b/layout/style/CSS.cpp
>+  //AppendEscaped returns false if aIdent contains U+0000
>+  //If it is passed an empty string, it returns true

Please just remove that comment.  I don't think it's adding anything here...

>+++ b/layout/style/nsStyleUtil.h

>+  // Retruns false if |aIdent| contians a null character

"Returns false if aIdent contains U+0000." perhaps?

r=me with those fixed as long as you'll fold all these patches together before pushing, but you still need to address the review comments on layout/style/CSS.h.

Comment 15

[sbetageri111]

Attachment: cssEscape955860.patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8365792 [details] [diff] [review]
cssEscape955860.patch

>+  //AppendEscaped returns false if aIdent contains U+0000

This never got removed.

>+  // Retruns false if |aIdent| contians a U+0000

See comment 14?

r+ as before, once there are tests...

Comment 17

[sbetageri111]

Attachment: bug-955860.patch

Comment 18

[sbetageri111]

Attachment: test_bug955860.html

Comment 19

[sbetageri111]

Attachment: bug-955860.patch

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8373385 [details] [diff] [review]
bug-955860.patch

>+SimpleTest.doesThrow(() => CSS.escape('\0'), "InvalidCharacterError");
>+SimpleTest.doesThrow(() => CSS.escape('a\0'), "InvalidCharacterError");
>+SimpleTest.doesThrow(() => CSS.escape('\0b'), "InvalidCharacterError");
>+SimpleTest.doesThrow(() => CSS.escape('a\0b'), "InvalidCharacterError");
>+SimpleTest.doesThrow(() => CSS.escape(), 'undefined');

The string passed in here should say something about what the particular subtest is testing.  Same thing for the various is() calls.

> +is(CSS.escape(true), 'true',"escapingFailed");

Need space after comma before last argument.  Same elsewhere in this file.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8373385 [details] [diff] [review]
bug-955860.patch

>+  static bool PrefEnabled()

Please don't add that back.  It was removed for a reason.  ;)

>+  // Retruns false if |aIdent| contians a U+0000

"Returns" and "contains".

r=me with those issues an the ones from the previous comment fixed.

Comment 22

[sbetageri111]

Attachment: bug955860.patch

Comment 23

[sbetageri111]

Attachment: bug955860.patch

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8391740 [details] [diff] [review]
bug955860.patch

>+++ b/dom/webidl/CSS.webidl

Why did you remove the spec link?  Please put it back.

>+++ b/layout/base/tests/mochitest.ini
> [test_bug851445.html]
>+[test_bug955860.html]
> support-files = bug851445_helper.html

Please put it after the support-files line, since that line goes with the previous entry?

>+++ b/layout/base/tests/test_bug955860.html
>+is(CSS.escape('0a'), '\\30 a',"escapingFailed Char: 0a");

Still needs space after the second comma here.  See comment 20.

r=me with those fixed.

Comment 25

[sbetageri111]

Attachment: bug955860.patch

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8393389 [details] [diff] [review]
bug955860.patch

r=me.  Are you going to be able to push this, or should I?

Comment 27

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1826224359f3

Comment 28

[Josh Matthews [:jdm]]

If we leave the tags we can use them for collecting stats.

Comment 29

[Mathias Bynens]

Glad to see you used my tests (https://github.com/mathiasbynens/CSS.escape/blob/master/tests/tests.js) for this :)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Oh, I'd missed that the tests were not original.  Mathias, are you OK with them being used here, copyright-wise?  If they're covered by the same MIT license as the rest of that repo, we need to put that license notice in...

Comment 31

[Mathias Bynens]

(In reply to Boris Zbarsky [:bz] from comment #30)
> Oh, I'd missed that the tests were not original.  Mathias, are you OK with
> them being used here, copyright-wise?  If they're covered by the same MIT
> license as the rest of that repo, we need to put that license notice in...

A simple comment mentioning the URL for the original tests or the GitHub repo (as was done here: http://hg.mozilla.org/integration/mozilla-inbound/rev/2411714cd058) would be nice, but either way I’m fine with it. Glad I could help!

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1826224359f3

Comment 33

[Boris Zbarsky [:bzbarsky]]

Added the comment: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9450153e8e

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0b9450153e8e

Comment 35

[ziyunfei]

https://developer.mozilla.org/en-US/docs/Web/API/CSS.escape

Please correct me if I write something wrong.

Comment 36

[Simon Pieters [:zcorpan]]

(In reply to 446240525 from comment #35)
> https://developer.mozilla.org/en-US/docs/Web/API/CSS.escape

> To escape a character means to create a string of "\" followed by that character.

This isn't quite right, it can also be followed by the hex number for the Unicode code point (and it has to if the character you want to escape is e.g. "1").

> CSS.escape("()[]{}")          // "\(\)\[\]{}"

The {} will be escaped also.

> CSS.escape('--a')             // "-\-a"

This is correct now but css-syntax changed to make it a valid ident (for custom properties); CSSOM hasn't been changed to reflect that yet (I filed a bug).

Comment 37

[ziyunfei]

Simon has updated the spec.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25426

The implementation should be updated too.

Comment 38

[Florian Bender]

(In reply to 446240525 from comment #37)
> The implementation should be updated too.

You should always file a new bug for that. I went ahead and created Bug 1008719. If you can, please describe specific differences between this implementation and the spec in Bug 1008719. Thanks!

Comment 39

[Jean-Yves Perrier [:teoli]]

Doc updated (big thanks to Ziyunfei!)
https://developer.mozilla.org/en-US/docs/Web/API/CSS.escape
https://developer.mozilla.org/en-US/docs/Web/API/CSS
and
https://developer.mozilla.org/en-US/Firefox/Releases/31