The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 31

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

({dev-doc-complete})

Trunk
mozilla31
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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

3 years ago
http://lists.w3.org/Archives/Public/www-style/2014Mar/0467.html
(Assignee)

Updated

3 years ago
Blocks: 773296
(Assignee)

Comment 2

3 years ago
Created attachment 8393996 [details] [diff] [review]
patch
Attachment #8393996 - Flags: review?(dbaron)
(Assignee)

Comment 3

3 years ago
(Tab says he will be making the spec changes tomorrow, btw.)
(Assignee)

Comment 4

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

One reftest failure, which I've fixed in the patch and tested locally.
Keywords: dev-doc-needed
(Assignee)

Comment 5

3 years ago
The spec changes have been made now: http://dev.w3.org/csswg/css-variables/
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.
Attachment #8393996 - Flags: review?(dbaron) → review-
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8400248 [details] [diff] [review]
patch (v2)

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);

r=dbaron
Attachment #8400248 - Flags: review?(dbaron) → review+
(Assignee)

Comment 10

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=000eb9e08919
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/822d8b27f427
(Assignee)

Comment 12

3 years ago
needinfo? for syncing the W3C test changes.
Flags: needinfo?(dbaron)
https://hg.csswg.org/test/rev/45be745b1772
Flags: needinfo?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/822d8b27f427
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

Updated

3 years ago
Keywords: site-compat
https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables
https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility
Keywords: dev-doc-needed → addon-compat, dev-doc-complete
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.
Keywords: addon-compat, site-compat

Updated

3 years ago
Depends on: 992333
Tagging as [qa-] but please let me know if this needs QA attention before we release.
status-firefox31: --- → fixed
Whiteboard: [qa-]
Flags: in-testsuite+

Updated

2 years ago
Depends on: 1175192
You need to log in before you can comment on or make changes to this bug.