Cameron McCormack (:heycam) Assignee
	Description • 10 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"

	John Daggett (:jtd)
	Comment 1 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 3 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 4 • 10 years ago

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

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

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 6 • 10 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 7 • 10 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.

	Cameron McCormack (:heycam) Assignee
	Comment 8 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 9 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 10 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 11 • 10 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 12 • 10 years ago

needinfo? for syncing the W3C test changes.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 13 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 14 • 10 years ago

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

	Kohei Yoshino [:kohei]
	Comment 15 • 10 years ago

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

	Kohei Yoshino [:kohei]
	Comment 16 • 10 years ago

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.

	u279076
	Comment 17 • 10 years ago

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