If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement `CSS.escape` as per CSSOM

RESOLVED FIXED in mozilla31

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Mathias Bynens, Assigned: sbetageri111)

Tracking

({dev-doc-complete})

Trunk
mozilla31
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz][lang=c++][good first bug])

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

4 years ago
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
You can file bugs in Core, you know :)
Component: Untriaged → DOM: CSS Object Model
Product: Firefox → Core
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....
Whiteboard: [mentor=bz][lang=c++][good first bug]
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
Yeah, that works too, and still doesn't presume confidence that there are no nulls.
Keywords: dev-doc-needed
> 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.
(Reporter)

Comment 7

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

Comment 8

4 years ago
Created attachment 8361090 [details] [diff] [review]
cssEscape.patch
Attachment #8361090 - Flags: review?(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.
Attachment #8361090 - Flags: review?(bzbarsky) → review+
Oh, and this still needs a mochitest.
(Assignee)

Comment 11

4 years ago
Created attachment 8361477 [details]
cssEscape1.patch
Attachment #8361477 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8361477 - Attachment is obsolete: true
Attachment #8361477 - Attachment is patch: false
Attachment #8361477 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

4 years ago
Created attachment 8361480 [details] [diff] [review]
cssEscape1.patch

A closing bracket is missing in this patch. Patch cssEscape2 rectifies it.
Attachment #8361480 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

4 years ago
Created attachment 8361482 [details] [diff] [review]
cssEscape2.patch
(Assignee)

Updated

4 years ago
Attachment #8361482 - Flags: review?(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.
Attachment #8361480 - Flags: review?(bzbarsky) → review+
Attachment #8361482 - Flags: review?(bzbarsky) → review+
Assignee: nobody → sbetageri111
(Assignee)

Comment 15

4 years ago
Created attachment 8365792 [details] [diff] [review]
cssEscape955860.patch
Attachment #8361090 - Attachment is obsolete: true
Attachment #8361480 - Attachment is obsolete: true
Attachment #8361482 - Attachment is obsolete: true
Attachment #8365792 - Flags: review?(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...
Attachment #8365792 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 8372797 [details] [diff] [review]
bug-955860.patch
Attachment #8365792 - Attachment is obsolete: true
Attachment #8372797 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

4 years ago
Created attachment 8372798 [details]
test_bug955860.html
Attachment #8372798 - Flags: review?(bzbarsky)
(Assignee)

Comment 19

4 years ago
Created attachment 8373385 [details] [diff] [review]
bug-955860.patch
Attachment #8372797 - Attachment is obsolete: true
Attachment #8372798 - Attachment is obsolete: true
Attachment #8372797 - Flags: review?(bzbarsky)
Attachment #8372798 - Flags: review?(bzbarsky)
Attachment #8373385 - Flags: review?(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 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.
Attachment #8373385 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

4 years ago
Created attachment 8391734 [details] [diff] [review]
bug955860.patch
Attachment #8373385 - Attachment is obsolete: true
Attachment #8391734 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

4 years ago
Created attachment 8391740 [details] [diff] [review]
bug955860.patch
Attachment #8391734 - Attachment is obsolete: true
Attachment #8391734 - Flags: review?(bzbarsky)
Attachment #8391740 - Flags: review?(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.
Attachment #8391740 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 25

4 years ago
Created attachment 8393389 [details] [diff] [review]
bug955860.patch
Attachment #8391740 - Attachment is obsolete: true
Attachment #8393389 - Flags: review?(bzbarsky)
Comment on attachment 8393389 [details] [diff] [review]
bug955860.patch

r=me.  Are you going to be able to push this, or should I?
Attachment #8393389 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(sbetageri111)
(Assignee)

Updated

4 years ago
Flags: needinfo?(sbetageri111) → needinfo?(bzbarsky)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1826224359f3
Flags: needinfo?(bzbarsky) → in-testsuite+
Whiteboard: [mentor=bz][lang=c++][good first bug]
If we leave the tags we can use them for collecting stats.
Whiteboard: [mentor=bz][lang=c++][good first bug]
(Reporter)

Comment 29

4 years ago
Glad to see you used my tests (https://github.com/mathiasbynens/CSS.escape/blob/master/tests/tests.js) for this :)
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...
(Reporter)

Comment 31

4 years ago
(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!
https://hg.mozilla.org/mozilla-central/rev/1826224359f3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Added the comment: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9450153e8e
https://hg.mozilla.org/mozilla-central/rev/0b9450153e8e

Comment 35

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

Please correct me if I write something wrong.

Comment 36

4 years ago
(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

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

The implementation should be updated too.

Updated

3 years ago
Blocks: 1008719

Comment 38

3 years ago
(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!
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
Keywords: dev-doc-needed → dev-doc-complete

Updated

2 years ago
Depends on: 1157279
You need to log in before you can comment on or make changes to this bug.