Bug 95284 - Make it possible to use CSS Variables inside Calc expressions.
Summary: Make it possible to use CSS Variables inside Calc expressions.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-28 21:40 PDT by Luke Macpherson
Modified: 2012-08-30 18:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.79 KB, patch)
2012-08-28 21:48 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (15.94 KB, patch)
2012-08-29 18:05 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (16.73 KB, patch)
2012-08-29 20:20 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (21.42 KB, patch)
2012-08-30 17:52 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-08-28 21:40:53 PDT
Make it possible to use CSS Variables inside Calc expressions.
Comment 1 Luke Macpherson 2012-08-28 21:48:21 PDT
Created attachment 161133 [details]
Patch
Comment 2 Mike Lawther 2012-08-28 22:51:26 PDT
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'
Comment 3 Tony Chang 2012-08-29 11:02:38 PDT
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?
Comment 4 Luke Macpherson 2012-08-29 16:17:32 PDT
(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.
Comment 5 Luke Macpherson 2012-08-29 18:05:44 PDT
Created attachment 161374 [details]
Patch
Comment 6 Luke Macpherson 2012-08-29 18:10:24 PDT
(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.
Comment 7 Mike Lawther 2012-08-29 18:56:16 PDT
(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)).
Comment 8 Luke Macpherson 2012-08-29 20:20:56 PDT
Created attachment 161392 [details]
Patch
Comment 9 Tony Chang 2012-08-30 10:09:45 PDT
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.
Comment 10 Luke Macpherson 2012-08-30 17:52:45 PDT
Created attachment 161597 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-08-30 18:13:27 PDT
Comment on attachment 161597 [details]
Patch for landing

Clearing flags on attachment: 161597

Committed r127220: <http://trac.webkit.org/changeset/127220>
Comment 12 WebKit Review Bot 2012-08-30 18:13:31 PDT
All reviewed patches have been landed.  Closing bug.