1175192 - Bug 985838 did not allow all identifiers to begin with two hyphens

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Oriol Brufau [:Oriol]]

According to bug 985838,
> idents in the CSS parser now allow things like "--"

I'm not entirely sure if that's true (the spec is not clear at all), but in any case, after fixing bug 985838, this selector works:
> [id=--]

However, this does not work:
> #--

This is problematic, because CSS.escape no longer escapes hyphens at the beginning, so the following may throw
> document.querySelector('#' + CSS.escape(id))

Note that, since Chromium implemented its new CSS parser (https://codereview.chromium.org/1015303002), it allows both #-- and [id=--]

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: test.htm

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I'm not entirely sure if that's true (the spec is not clear at all)

Seems pretty clear for this case.  In particular, http://dev.w3.org/csswg/css-syntax/#consume-token the U+0023 section uses http://dev.w3.org/csswg/css-syntax/#check-if-three-code-points-would-start-an-identifier which treats "--" as starting an identifier.  So "#--" should create an "id" token per spec.  Yay lack of test coverage.  :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Consistently allow identifiers to start with -- in the CSS parser

Comment 4

[Cameron McCormack (:heycam)]

Attachment: test 2

Looks like my one non-variable test change in test_parse_rule.html in bug 985838 was wholly inadequate.

There is another slight difference from the spec, although it's a real edge case.  The three character sequence "-\\\n" should tokenize as '-' delim, '\\' delim (and an error), "\n" white space.  But I think we currently tokenize it as '-' ident, '\\' delim, "\n" white space.

AFAICT the only place where a '\\' delim will not cause a parse error is in the value of a custom property.  Because of the rules about inserting /**/ between certain adjacent tokens when expanding out var() functions, whether the '-' (incorrectly) becomes an ident or (correctly) becomes a delim can determine whether that /**/ is generated.

The attachment illustrates.  I think the spec should require the computed value of --b to be serialized as "1-\\\nx", whereas we currently serialize it as "1/**/-\\\nx".

Do you want to fix that as part of this patch?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> There is another slight difference from the spec,

Ah, I tried to figure out where the third char bit was really needed and couldn't think of a place; I don't know the custom property parsing well enough.

I could go either way, but I suspect you're better positioned than me to figure out which all places need to check for the \n (e.g. the ones I'm touching don't seem to be related to custom property stuff)...

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> I could go either way, but I suspect you're better positioned than me to
> figure out which all places need to check for the \n (e.g. the ones I'm
> touching don't seem to be related to custom property stuff)...

Sounds good, I filed bug 1175753 to do that.

Comment 7

[Oriol Brufau [:Oriol]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Seems pretty clear for this case.  In particular,
> http://dev.w3.org/csswg/css-syntax/#consume-token the U+0023 section uses
> http://dev.w3.org/csswg/css-syntax/#check-if-three-code-points-would-start-
> an-identifier which treats "--" as starting an identifier.  So "#--" should
> create an "id" token per spec.

OK, thanks, I wasn't looking at the right spec.

Why hasn't your patch been implemented? Is bug 1175753 a dependency?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Why hasn't your patch been implemented?

Oh, good catch.  I thought I'd checked this in...  Doing that now.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b09041ef8f2

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b09041ef8f2