Bug 95284 – Make it possible to use CSS Variables inside Calc expressions.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Make it possible to use CSS Variables inside Calc expressions.

[Luke Macpherson]

Created attachment 161133 [details]
Patch

[Mike Lawther]

Comment on attachment 161133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161133&action=review

Shadow review.

lgtm, please consider nits though

> Source/WebCore/css/CSSCalculationValue.cpp:111
> +bool CSSCalcValue::hasVariableReference() const

suggestion: could this be 'hasVariables' or 'containsVariables' or 'hasCssVariableReference'? 'hasVariableReference' could be misinterpreted as 'has a reference, and it is variable/mutable', rather than 'there are CSS variables here'.

> Source/WebCore/css/CSSCalculationValue.h:108
> +    String customSerializeResolvingVariables(const HashMap<AtomicString, String>&) const;

Here it is 'customSerializeResolvingVariables', while in CalcExpressionNode it is 'serializeResolvingVariables'. Other methods like 'customCssText' are the same in both - I don't understand why these are different?

> Source/WebCore/css/CSSPrimitiveValue.cpp:191
> +        return CSSPrimitiveValue::CSS_UNKNOWN; // The type of an calculation containing a variable cannot be known until the value of the variable is determined.

typo: 'an caculation' -> 'a calculation'

[Tony Chang]

Comment on attachment 161133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161133&action=review

> Source/WebCore/ChangeLog:19
> +        (WebCore::CSSCalcValue::customSerializeResolvingVariables):
> +        (WebCore::CSSCalcValue::hasVariableReference):
> +        (CSSCalcPrimitiveValue):
> +        (WebCore::CSSCalcPrimitiveValue::serializeResolvingVariables):

Please fill in this section of the ChangeLog.

> Source/WebCore/css/CSSCalculationValue.cpp:106
> +    bool expressionHasSingleTerm = expression[0] != '(';
> +    if (expressionHasSingleTerm)
> +        result.append('(');
> +    result.append(expression);
> +    if (expressionHasSingleTerm)

This code seems to be duplicated with customCSSText.  Can you make a helper function that takes |expression| as an input param?

> Source/WebCore/css/CSSCalculationValue.cpp:271
> +        else
> +#endif
>          switch (op) {

This is hard to follow since the else clause is long and not indented. Maybe make a new method that returns newCategory (so the variables code would just be an early return)?

> LayoutTests/fast/css/variables/calc.html:8
> +  border-width: -webkit-calc(-webkit-var(a) + -webkit-var(b)) -webkit-calc(-webkit-var(c) * -webkit-var(d));

Huh, the -webkit makes it look like it's a negative value.  Just for completeness, can you add a test case with --webkit-var to see if it negates it?  Also, can you add some test cases for when variables are not defined or are invalid values?

[Luke Macpherson]

(In reply to comment #2)
> (From update of attachment 161133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161133&action=review
> 
> Shadow review.
> 
> lgtm, please consider nits though
> 
> > Source/WebCore/css/CSSCalculationValue.cpp:111
> > +bool CSSCalcValue::hasVariableReference() const
> 
> suggestion: could this be 'hasVariables' or 'containsVariables' or 'hasCssVariableReference'? 'hasVariableReference' could be misinterpreted as 'has a reference, and it is variable/mutable', rather than 'there are CSS variables here'.

I see your point, but hasVariableReference is already the name used everywhere else in the CSSValue hierarchy. I can change that, but it would be a job for a different patch.

> > Source/WebCore/css/CSSCalculationValue.h:108
> > +    String customSerializeResolvingVariables(const HashMap<AtomicString, String>&) const;
> 
> Here it is 'customSerializeResolvingVariables', while in CalcExpressionNode it is 'serializeResolvingVariables'. Other methods like 'customCssText' are the same in both - I don't understand why these are different?

custom* is used because CSSValue does manual devirtualization, so a different function name is needed. Because CalcExpressionNode does use virtual functions this is not necessary in that case. If in a future patch CSSCalculationValue is changed not to extend CSSValue we can drop the "custom" prefix.

> > Source/WebCore/css/CSSPrimitiveValue.cpp:191
> > +        return CSSPrimitiveValue::CSS_UNKNOWN; // The type of an calculation containing a variable cannot be known until the value of the variable is determined.
> 
> typo: 'an caculation' -> 'a calculation'

Heh, your typo there confused me for a second. s/an/a done.

[Luke Macpherson]

Created attachment 161374 [details]
Patch

[Luke Macpherson]

(In reply to comment #3)
> Huh, the -webkit makes it look like it's a negative value.  Just for completeness, can you add a test case with --webkit-var to see if it negates it?  Also, can you add some test cases for when variables are not defined or are invalid values?

See: http://www.w3.org/TR/css3-values/#calc - as I understand it (from Mike's verbal description) css calc expressions do not support unary negation, although you can write 0 - x to achieve the same result.

[Mike Lawther]

(In reply to comment #6)
> (In reply to comment #3)
> > Huh, the -webkit makes it look like it's a negative value.  Just for completeness, can you add a test case with --webkit-var to see if it negates it?  Also, can you add some test cases for when variables are not defined or are invalid values?
> 
> See: http://www.w3.org/TR/css3-values/#calc - as I understand it (from Mike's verbal description) css calc expressions do not support unary negation, although you can write 0 - x to achieve the same result.

The gotcha is that you can't negate a parenthesised expression - ie calc(-(10px * 2)) isn't allowed. You need to do calc(-1 * (10px * 2)) or calc(0 - (10px * 2)).

[Luke Macpherson]

Created attachment 161392 [details]
Patch

[Tony Chang]

Comment on attachment 161392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161392&action=review

The change looks fine, but please add more test cases before landing.  There should be test cases for:
1) Referencing a variable that doesn't exist.
2) Referencing a variable that is not a valid value (e.g., has a string value).
3) --webkit-var() just to codify the behavior.  It's fine that that fails, but we should still have a test for it.
4) Maybe a test case where the variable itself is a calc expression?  Also doesn't have to work, but would be nice to have a test case.

> Source/WebCore/css/CSSCalculationValue.cpp:80
> +static String buildCssText(String expression)

Nit: const String& for the input param.  It saves a bit of internal ref counting.

> Source/WebCore/css/CSSCalculationValue.cpp:330
> +    static String buildCssText(String leftExpression, String rightExpression, CalcOperator op)

Nit: const String& for input params.

[Luke Macpherson]

Created attachment 161597 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 161597 [details]
Patch for landing

Clearing flags on attachment: 161597

Committed r127220: <http://trac.webkit.org/changeset/127220>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.