Closed Bug 955860 Opened 6 years ago Closed 6 years ago

Implement `CSS.escape` as per CSSOM

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mathias, Assigned: sbetageri111)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=bz][lang=c++][good first bug])

Attachments

(1 file, 10 obsolete files)

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.
> 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.
(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.
Attached patch cssEscape.patch (obsolete) — Splinter Review
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.
Attached file cssEscape1.patch (obsolete) —
Attachment #8361477 - Flags: review?(bzbarsky)
Attachment #8361477 - Attachment is obsolete: true
Attachment #8361477 - Attachment is patch: false
Attachment #8361477 - Flags: review?(bzbarsky)
Attached patch cssEscape1.patch (obsolete) — Splinter Review
A closing bracket is missing in this patch. Patch cssEscape2 rectifies it.
Attachment #8361480 - Flags: review?(bzbarsky)
Attached patch cssEscape2.patch (obsolete) — Splinter Review
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
Attached patch cssEscape955860.patch (obsolete) — Splinter Review
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+
Attached patch bug-955860.patch (obsolete) — Splinter Review
Attachment #8365792 - Attachment is obsolete: true
Attachment #8372797 - Flags: review?(bzbarsky)
Attached file test_bug955860.html (obsolete) —
Attachment #8372798 - Flags: review?(bzbarsky)
Attached patch bug-955860.patch (obsolete) — Splinter Review
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+
Attached patch bug955860.patch (obsolete) — Splinter Review
Attachment #8373385 - Attachment is obsolete: true
Attachment #8391734 - Flags: review?(bzbarsky)
Attached patch bug955860.patch (obsolete) — Splinter Review
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+
Attached patch bug955860.patchSplinter Review
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)
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]
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...
(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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
https://developer.mozilla.org/en-US/docs/Web/API/CSS.escape

Please correct me if I write something wrong.
(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).
Simon has updated the spec.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25426

The implementation should be updated too.
Blocks: 1008719
(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!
Depends on: 1157279
You need to log in before you can comment on or make changes to this bug.