Closed Bug 1008719 Opened 5 years ago Closed 5 years ago

Update CSS escaping to not escape vendor identifiers

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
relnote-firefox --- -

People

(Reporter: fb+mozdev, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

+++ 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.
Note that this would affect all CSS serialization, not just CSS.escape, right?
Flags: needinfo?(446240525)
(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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(446240525)
Whiteboard: [need review]
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).
Attachment #8422198 - Flags: review?(dholbert) → review-
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....
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.
Assignee: bzbarsky → nobody
Status: ASSIGNED → NEW
Whiteboard: [need review]
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?
Flags: needinfo?(bzbarsky)
Summary: Update `CSS.escape` to recent spec changes → Update CSS escaping to not escape vendor identifiers
(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 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).
> 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...
Flags: needinfo?(bzbarsky)
Attachment #8422198 - Flags: review- → review?(dholbert)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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 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
Attachment #8422198 - Flags: review?(dholbert) → review+
> 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.
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
Flags: in-testsuite+
Whiteboard: [need review]
https://hg.mozilla.org/mozilla-central/rev/3d0a7747bc93
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
relnote-firefox: --- → ?
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.
You need to log in before you can comment on or make changes to this bug.