1008719 - Update CSS escaping to not escape vendor identifiers

Status: RESOLVED FIXED

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

Description

[Florian Bender]

+++ This bug was initially created as a clone of Bug #955860 +++

(In reply to 446240525 from comment #37)
> Simon has updated the spec.
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=25426
> 
> The implementation should be updated too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that this would affect all CSS serialization, not just CSS.escape, right?

Comment 2

[Mathias Bynens]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Note that this would affect all CSS serialization, not just CSS.escape,
> right?

Right.

As for `CSS.escape`, I’ve updated my tests: https://github.com/mathiasbynens/CSS.escape/blob/master/tests/tests.js Feel free to use them for any follow-up patches.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: CSS syntax got changed to allow identifiers starting with "--", so update our escaping code accordingly

Comment 4

[Mathias Bynens]

Comment on attachment 8422198 [details] [diff] [review]
CSS syntax got changed to allow identifiers starting with "--", so update our escaping code accordingly

Review of attachment 8422198 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM but note that `CSS.escape` should also be changed so that U+0080 to U+009F are not escaped anymore, as per Simon’s other recent update (https://www.w3.org/Bugs/Public/show_bug.cgi?id=25500).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hrm.  That's a somewhat more worrisome change, esp given what the CSS 2.1 grammar used to say here.

I guess our parser does treat those as ident chars....

Comment 6

[Boris Zbarsky [:bzbarsky]]

Anyway, I'm out of time for figuring out why the tests for those chars are failing, and don't think that's all that important in practice, so back into the pile this goes.

Comment 7

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

Why don't you just land the patch that worked?  I think Mathias's review- was out-of-line, since I don't think review- is an appropriate annotation for a patch that fixes some things but not everything.

And maybe file a separate bug for the U+0080 - U+009F changes, which are much lower priority?

Comment 8

[Mathias Bynens]

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #7)
> Why don't you just land the patch that worked?  I think Mathias's review-
> was out-of-line, since I don't think review- is an appropriate annotation
> for a patch that fixes some things but not everything.
> 
> And maybe file a separate bug for the U+0080 - U+009F changes, which are
> much lower priority?

I didn’t think this would warrant a separate bug, since all it takes is a one-line fix to this line: https://hg.mozilla.org/mozilla-central/rev/1826224359f3#l4.51 After that, the tests should be updated like so: https://github.com/mathiasbynens/CSS.escape/compare/83d21b1c0f42653587b5e75c16215bd7631d3e29...v0.2.0#diff-2

Comment 9

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

comment 6 says otherwise.  Anyway, please take it to a separate bug; it was not a part of this bug prior to comment 4, is higher risk (comment 5), lower value (unlikely to be used), and more work (comment 6).

Comment 10

[Boris Zbarsky [:bzbarsky]]

> After that, the tests should be updated like so

You might be surprised to discover that we have other tests for CSS escaping in the tree.  Or not surprised, had you read the patch.

As for comment 7, I really was out of the time I've allocated to spend on this.  But yes, I agree, comment 4 should be a separate bug.  Note that afaict that bug would affect string escaping too, not just ident escaping.

But OK, let's try this again...

Comment 11

[Boris Zbarsky [:bzbarsky]]

By the way, David, feel free to steal the review, but I assumed that both you and heycam are on vacation, hence dholbert as the victim.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8422198 [details] [diff] [review]
CSS syntax got changed to allow identifiers starting with "--", so update our escaping code accordingly

>+++ b/layout/style/nsStyleUtil.cpp
> nsStyleUtil::AppendEscapedCSSIdent(const nsAString& aIdent, nsAString& aReturn)
> {
>   // The relevant parts of the CSS grammar are:
>-  //   ident    [-]?{nmstart}{nmchar}*
>+  //   ident    ([-]?{nmstart}|[-][-]){nmchar}*
>   //   nmstart  [_a-z]|{nonascii}|{escape}
>   //   nmchar   [_a-z0-9-]|{nonascii}|{escape}
>   //   nonascii [^\0-\177]
>   //   escape   {unicode}|\\[^\n\r\f0-9a-f]
>   //   unicode  \\[0-9a-f]{1,6}(\r\n|[ \n\r\t\f])?
>   // from http://www.w3.org/TR/CSS21/syndata.html#tokenization

That CSS21 #tokenization URL still has the old grammar that you're overwriting here -- [-]?{nmstart}{nmchar}* -- and I imagine it will continue to have that old grammar.  So, that URL reference is now stale/incorrect, as part of this documentation.

Could you replace (or supplement) that URL with a reference to the dev spec, or the w3c bug, or other some updated URL to make this clearer?

>diff --git a/layout/style/test/test_parser_diagnostics_unprintables.html b/layout/style/test/test_parser_diagnostics_unprintables.html
>--- a/layout/style/test/test_parser_diagnostics_unprintables.html
>+++ b/layout/style/test/test_parser_diagnostics_unprintables.html
>@@ -69,17 +69,17 @@ const substitutions = [
>   { t: "\\32 ",  i: "\\32 ",  s: "2"  },
>   { t: "\\33 ",  i: "\\33 ",  s: "3"  },
>   { t: "\\34 ",  i: "\\34 ",  s: "4"  },
>   { t: "\\35 ",  i: "\\35 ",  s: "5"  },
>   { t: "\\36 ",  i: "\\36 ",  s: "6"  },
>   { t: "\\37 ",  i: "\\37 ",  s: "7"  },
>   { t: "\\38 ",  i: "\\38 ",  s: "8"  },
>   { t: "\\39 ",  i: "\\39 ",  s: "9"  },
>-  { t: "-\\-",   i: "-\\-",   s: "--" },
>+  { t: "-\\-",   i: "--",   s: "--" },
>   { t: "-\\30 ", i: "-\\30 ", s: "-0" },
>   { t: "-\\31 ", i: "-\\31 ", s: "-1" },

Nit: in the modified line here, add two spaces before "s:", to keep it lining up with the tests above/below it.

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Could you replace (or supplement) that URL with a reference to the dev spec

Will do.  Note that the new spec has no formal grammar, just a state-machine parser; it took a bit of thinking to correctly modify the production based on that.

> Nit: in the modified line here, add two spaces before "s:"

Will do.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out because something in the push was causing mochitest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b99b42f1337

https://tbpl.mozilla.org/php/getParsedLog.php?id=39747566&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39747507&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39747838&tree=Mozilla-Inbound

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This push caused a large number of B2G debug mochitest failures that contributed to a large bustage pileup on inbound and led to having to revert to a last-good cset, inconveniencing other innocent developers as well. These same failures occurred with yesterday's landing. Please give this a full Try run before attempting to re-land.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2

https://tbpl.mozilla.org/php/getParsedLog.php?id=39821101&tree=Mozilla-Inbound

Comment 16

[Boris Zbarsky [:bzbarsky]]

The b2g bits were bug 1011810.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0a7747bc93

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3d0a7747bc93

Comment 18

[Boris Zbarsky [:bzbarsky]]

Fwiw, I don't think this needs a relnote.  It's a minor bugfix.  There are way more important things going on that never get relnoted.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Agree with comment #18.

Comment 20

[Jean-Yves Perrier [:teoli]]

Added a comment in the browser compat table of :
https://developer.mozilla.org/en-US/docs/Web/API/CSS.escape
and in
https://developer.mozilla.org/en-US/Firefox/Releases/32