985838 - change "var-" prefix of CSS Variables to "--"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Recently decided changes to the CSS Variables spec:

  * the prefix of the custom property change from "var-" to "--"
  * inside the var() you use the full custom property name (i.e. with the "--"
    prefix)
  * a custom property consisting only of the prefix, "--", is allowed
  * idents in the CSS parser now allow things like "--" and "--0"

Comment 1

[John Daggett (:jtd)]

http://lists.w3.org/Archives/Public/www-style/2014Mar/0467.html

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 3

[Cameron McCormack (:heycam)]

(Tab says he will be making the spec changes tomorrow, btw.)

Comment 4

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=a1d4d59e5baf

One reftest failure, which I've fixed in the patch and tested locally.

Comment 5

[Cameron McCormack (:heycam)]

The spec changes have been made now: http://dev.w3.org/csswg/css-variables/

Comment 6

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

Comment on attachment 8393996 [details] [diff] [review]
patch

CSSVariableDeclarations.h:

  I find it a little odd that the thing stored excludes the -- prefix,
  but I guess that's ok.  Less memory use, at least.

CSSParserImpl::ResolveValueWithVariableR...:

>           if (!GetToken(true) ||
>-              mToken.mType != eCSSToken_Ident) {
>-            // "var(" must be followed by an identifier.
>+              mToken.mType != eCSSToken_Ident ||
>+              !nsCSSProps::IsCustomPropertyName(mToken.mIdent)) {
>+            // "var(" must be followed by an identifier, and it must be a
>+            // custom property name.
>             return false;
>           }

Where does the spec say this?

As I read the spec, var(foo) is valid but never matches ... but it could
fall back.


I guess if that isn't the case, you'll have to change
CSSVariableDeclarations to store the -- prefix.


Same in CSSParserImpl::ParseValueWithVariables


I'm a little concerned about the number of places where VAR_PREFIX_LENGTH
is defined.  Maybe put it in nsCSSProperty.h and give it a slightly more
global name (CSS_VAR_PREFIX_LENGTH?)?


Please needinfo? me after you land so I can sync the tests to the w3c
repo.


review- only because of the issue of whether var() functions with
an ident not starting with -- are valid.  Do you think we should fix
the code for this or ask to have the spec changed?

Otherwise looks good, and sorry for taking so long to get to this.

Comment 7

[Cameron McCormack (:heycam)]

I asked Tab and he is going to make it so that var(foo), i.e. without the "--" prefix, is invalid at parsing time, not computed value time.

I will define the VAR_PREFIX_LENGTH somewhere more globally.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch (v2)

Only change is moving/renaming VAR_PREFIX_LENGTH to nsCSSProps.h CSS_CUSTOM_NAME_PREFIX_LENGTH.

Comment 9

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

Comment on attachment 8400248 [details] [diff] [review]
patch (v2)

Please fix the overly long line in Declaration::AppendPropertyAndValueToString, maybe by adding a variable:
  const nsSubstring& variableName =
    Substring(aProperty, CSS_CUSTOM_NAME_PREFIX_LENGTH);

r=dbaron

Comment 10

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=000eb9e08919

Comment 11

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/822d8b27f427

Comment 12

[Cameron McCormack (:heycam)]

needinfo? for syncing the W3C test changes.

Comment 13

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

https://hg.csswg.org/test/rev/45be745b1772

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/822d8b27f427

Comment 15

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables
https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility

Comment 16

[Kohei Yoshino]

Looks like CSS variables are enabled by default since Firefox 31, so this change won't raise any compatibility issues. Updating the keywords and doc.

Comment 17

[u279076]

Tagging as [qa-] but please let me know if this needs QA attention before we release.