Closed Bug 985838 Opened 10 years ago Closed 10 years ago

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


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox31 --- fixed


(Reporter: heycam, Assigned: heycam)



(Keywords: dev-doc-complete, Whiteboard: [qa-])


(1 file, 1 obsolete file)

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 "--"
  * a custom property consisting only of the prefix, "--", is allowed
  * idents in the CSS parser now allow things like "--" and "--0"
Blocks: 773296
Attached patch patch (obsolete) — Splinter Review
Attachment #8393996 - Flags: review?(dbaron)
(Tab says he will be making the spec changes tomorrow, btw.)

One reftest failure, which I've fixed in the patch and tested locally.
The spec changes have been made now:
Comment on attachment 8393996 [details] [diff] [review]


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


>           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

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.
Attachment #8393996 - Flags: review?(dbaron) → review-
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.
Attached patch patch (v2)Splinter Review
Only change is moving/renaming VAR_PREFIX_LENGTH to nsCSSProps.h CSS_CUSTOM_NAME_PREFIX_LENGTH.
Attachment #8393996 - Attachment is obsolete: true
Attachment #8400248 - Flags: review?(dbaron)
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);

Attachment #8400248 - Flags: review?(dbaron) → review+
needinfo? for syncing the W3C test changes.
Flags: needinfo?(dbaron)
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Keywords: site-compat
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.
Depends on: 992333
Tagging as [qa-] but please let me know if this needs QA attention before we release.
Whiteboard: [qa-]
Flags: in-testsuite+
Depends on: 1175192
You need to log in before you can comment on or make changes to this bug.