Last Comment Bug 363249 - implement css3-values calc()
: implement css3-values calc()
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 35 votes (vote)
: ---
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
:
:
Mentors:
http://www.w3.org/TR/css3-values/#calc
Depends on: 332335 565248 585715 776443
Blocks: css3-values 520234 536539 569993 571489 594933
  Show dependency treegraph
 
Reported: 2006-12-08 22:25 PST by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2014-04-25 15:15 PDT (History)
65 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
patch 1: add calc() types to nsCSSValue (7.40 KB, patch)
2010-01-03 13:37 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 2: templates for computing calc trees (8.42 KB, patch)
2010-01-03 13:37 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 3: calc() parsing (17.70 KB, patch)
2010-01-03 13:37 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 4: calc() serialization (5.95 KB, patch)
2010-01-03 13:38 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 5: add mechanism for length-only calc() to SetCoord (10.72 KB, patch)
2010-01-03 13:39 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord (2.38 KB, patch)
2010-01-03 13:39 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 7: add support for calc() to the easiest properties (23.38 KB, patch)
2010-01-03 13:40 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 8: add support for calc() to text-shadow and -moz-box-shadow (9.37 KB, patch)
2010-01-03 13:41 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 9: add support for calc() to font-size (7.58 KB, patch)
2010-01-03 13:41 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 2: templates for computing calc trees (8.42 KB, patch)
2010-04-09 15:33 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 3: calc() parsing (17.48 KB, patch)
2010-04-09 15:34 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 4: calc() serialization (5.95 KB, patch)
2010-04-09 15:35 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 5: add mechanism for length-only calc() to SetCoord (10.72 KB, patch)
2010-04-09 15:35 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord (2.38 KB, patch)
2010-04-09 15:36 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 7: add support for calc() to the easiest properties (24.15 KB, patch)
2010-04-09 15:36 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 8: add support for calc() to text-shadow and -moz-box-shadow (9.37 KB, patch)
2010-04-09 15:37 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 9: add support for calc() to font-size (7.58 KB, patch)
2010-04-09 15:37 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
interdiff on top of patch 3: support min()/max() on numbers (6.95 KB, patch)
2010-05-10 16:18 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 10: remove useless (void) in nsStyleCoord (9.54 KB, patch)
2010-05-10 16:20 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 11: make CalcOps methods non-static and just instantiate ops rather than ComputeData (19.18 KB, patch)
2010-05-10 16:21 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 12: add nsStyleContext::Alloc for allocations scoped to the lifetime of the style context (5.88 KB, patch)
2010-06-15 22:37 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 13: add nsStyleCoord::Array (5.78 KB, patch)
2010-06-15 22:38 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 14: add storage for calc() expressions to nsStyleCoord (6.65 KB, patch)
2010-06-15 22:38 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 15: fix namespace usage in nsRuleNode (4.21 KB, patch)
2010-06-15 22:39 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 16: switch nscoord math to saturating operations (3.60 KB, patch)
2010-06-25 17:29 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 17: build computed-value calc() expressions and store them in nsStyleCoord (9.32 KB, patch)
2010-06-25 17:33 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 18: make ComputeCalc more general so its input can be nsCSSValue or nsStyleCoord (7.07 KB, patch)
2010-06-25 17:33 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 19: add API to compute computed-value calc() expressions to lengths (3.71 KB, patch)
2010-06-25 17:35 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 20: add API to compute length, percentage, or calc() on nsStyleCoord (3.19 KB, patch)
2010-06-25 17:35 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 21: make calc() serialization code a template so it can be used for nsStyleCoord or nsCSSValue (12.13 KB, patch)
2010-06-25 17:37 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
patch 22: handle computed-value calc() expressions in nsComputedDOMStyle (5.65 KB, patch)
2010-06-25 17:38 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
add outline-width and -moz-column-gap calc() support (7.53 KB, patch)
2010-09-07 14:10 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
fix calc() on border-*-width (10.43 KB, patch)
2010-09-07 14:11 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
remove min() and max() parsing and storage (86.19 KB, patch)
2010-09-09 11:21 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
change nsStyleCoord calc() storage as a result of removing min() and max() (42.42 KB, patch)
2010-09-09 12:26 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
change nsStyleCoord calc() storage as a result of removing min() and max() (43.19 KB, patch)
2010-09-10 10:27 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
another piece of min/max code to remove (1.36 KB, patch)
2010-09-12 22:37 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-12-08 22:25:06 PST
We should implement calc() from css3-values:
http://www.w3.org/TR/css3-values/#calc
Comment 1 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:36:20 PST
I have patches for the parts needed to support -moz-calc(), -moz-min(), and -moz-max(), though with some differences from the current spec's grammar (most importantly, no division by lengths, for the reasons given in http://lists.w3.org/Archives/Public/www-style/2010Jan/0007.html , and some other differences as described in posts to www-style in December.

I'm still working on code needed to support calc on properties that use nsStyleCoord; those require a good bit more work.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:37:00 PST
Created attachment 419838 [details] [diff] [review]
patch 1: add calc() types to nsCSSValue
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:37:28 PST
Created attachment 419839 [details] [diff] [review]
patch 2: templates for computing calc trees
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:37:52 PST
Created attachment 419840 [details] [diff] [review]
patch 3: calc() parsing
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:38:25 PST
Created attachment 419841 [details] [diff] [review]
patch 4: calc() serialization
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:39:14 PST
Created attachment 419842 [details] [diff] [review]
patch 5: add mechanism for length-only calc() to SetCoord
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:39:58 PST
Created attachment 419843 [details] [diff] [review]
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:40:45 PST
Created attachment 419844 [details] [diff] [review]
patch 7: add support for calc() to the easiest properties
Comment 9 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:41:20 PST
Created attachment 419845 [details] [diff] [review]
patch 8: add support for calc() to text-shadow and -moz-box-shadow
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-03 13:41:52 PST
Created attachment 419846 [details] [diff] [review]
patch 9: add support for calc() to font-size
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2010-01-21 15:38:30 PST
Comment on attachment 419838 [details] [diff] [review]
patch 1: add calc() types to nsCSSValue

r=bzbarsky
Comment 12 d 2010-02-06 16:25:02 PST
Are the patches ready to be pushed, for early support, and, which properties are left to support?
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-02-06 16:29:22 PST
No.  I need to update a bunch per CSS WG discussion (and also write up a spec proposal to reflect that discussion).
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:33:50 PDT
Created attachment 438182 [details] [diff] [review]
patch 2: templates for computing calc trees

Given that (a) the CSS WG hasn't settled on the key issues (the main proposal to address them didn't match the outline of what the group resolved), (b) my inclination is that what I've implemented actually does address the important use cases, and (c) changing to address these issues probably involves some pretty deep changes depending on what the eventual decision of the group is, I think I'd like to move forward with what I've implemented and revise when the group really comes to consensus that seems like it's going to stick.

I probably should have done this months ago, though I'd been planning at the time to implement something based more on the thoughts the group discussed.  However, having finally had time to get back to this, I spent a bunch of this afternoon thinking about how my implementation would need to change for various possibilities of what the group might settle on, and realized the changes would be rather different for different solutions.

So I'm going to repost the remaining patches for review, merely merged to trunk.


These limit calc() such that:
 * leaf values must be either numbers or values of the property
 * values of the property cannot be divided or multiplied by each other
though the working group seemed to want to remove those limits.

I believe that other than that, they match current working group thinking on calc(), and I have very little confidence that the working group isn't going to change its mind on the above points at least one more time (since the decision procedure used by the group tends to produce different results when repeated, even without new information).


For the record, the existing working group discussion is minuted in http://lists.w3.org/Archives/Public/www-style/2010Jan/0468.html


I'd note that my patches beyond what I'd posted for review (I have a bunch of other in-progress patches beyond those above, to handle calc() for properties for which percentages need to be resolved at layout time... which are the biggest use case) do need further review to address the real issue in http://lists.w3.org/Archives/Public/www-style/2010Apr/0256.html , probably as described in http://lists.w3.org/Archives/Public/www-style/2010Apr/0258.html
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:34:48 PDT
Created attachment 438183 [details] [diff] [review]
patch 3: calc() parsing
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:35:17 PDT
Created attachment 438184 [details] [diff] [review]
patch 4: calc() serialization
Comment 17 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:35:47 PDT
Created attachment 438185 [details] [diff] [review]
patch 5: add mechanism for length-only calc() to SetCoord
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:36:13 PDT
Created attachment 438186 [details] [diff] [review]
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord
Comment 19 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:36:41 PDT
Created attachment 438187 [details] [diff] [review]
patch 7: add support for calc() to the easiest properties
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:37:11 PDT
Created attachment 438188 [details] [diff] [review]
patch 8: add support for calc() to text-shadow and -moz-box-shadow
Comment 21 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-04-09 15:37:38 PDT
Created attachment 438189 [details] [diff] [review]
patch 9: add support for calc() to font-size
Comment 22 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-05-06 22:24:40 PDT
Boris asked for a description of what I'm implementing.  The current spec (which is different from what it was when I last looked) says something a bit closer to what I'm implementing.  What it says is:

> The expression language of these functions is described by the grammar
> and prose below.
>·
> S       : calc | min | max;
> calc    : "calc(" S* sum ")" S*;
> min     : "min(" S* sum [ "," S* sum ]* )"" S*;
> max     : "max(" S* sum [ "," S* sum ]* ")" S*;
> sum     : product [ [ "+" | "-" ] S* product ]*;
> product : unit [ [ "*" | "/" | "mod" ] S* unit ]*;
> unit    : ["+"|"-"]? [ NUMBER S* | DIMENSION S* | PERCENTAGE S* |
>           min | max | "(" S* sum ")" S* ];
>·
> The semantic constraints are as follows: The context of the expression
> imposes a target type, which is one of length, frequency, angle, time,
> or number. Any PERCENTAGE token in the expression also has that type.
> The NUMBER tokens obviously are of type number, and the DIMENSION tokens
> have types that depend on their units (cm, px, deg, etc.). At each
> operator, the types of the left and right side have to be compared and,
> if compatible, they yield a result type, roughly as follows (the
> following ignores precedence rules on the operators for simplicity):
>·
>    1. At ",", "+", "-":
>       check: both sides have the same type
>       return: that type
>    2. At "*":
>       check: at least one side is "number" return: the type of the other
>       side
>    3. At "/":
>       check: either right side is "number," or both have the same type
>       return: if the former, type of left side; otherwise "number"
>    4. At "mod":
>       check: both sides have the same type
>       return: "number"·

My implementation differs in the following ways, I think:

I implement sum and product according to their earlier recursive definitions, but that's functionally equivalent.

I require that for '/' and 'mod' the latter type must be number (i.e., I do not allow division by values).

(Note that if we were to implement division by values, I would also push to drop the constraint that '*' requires one side to be a number.)

I only allow dimensions that are allowed as values of the property.  (Doing otherwise would not make sense without division-by-values.)


The spec also doesn't mention that whitespace is required around + and -, but I believe the group has discussed and agreed to that requirement.  (This is pretty much required by CSS allowing - in identifiers, unless we were to use a different tokenization inside calc().)


I should probably just drop mod entirely; I don't implement the spec's requirement that both sides be the same type, but that is definitely a sensible requirement, so my current implementation is meaningless and I should probably just drop it.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:26:15 PDT
Comment on attachment 438182 [details] [diff] [review]
patch 2: templates for computing calc trees

>+ *   static result_type
>+ *   MergeAdditive(nsCSSUnit aCalcFunction,
>+ *                 result_type aValue1, result_type aValue2);

So I guess the point here is that this isn't so much "additive" as an expression that takes two result_types and returns a result_type...  Sadly, I can't think of a better name offhand.

It might be good to document a bit more about what MergeAdditive would do (e.g. why it needs the aCalcFunction argument and what the allowed values of aCalcFunction are).

>+ *   static result_type
>+ *   MergeMultiplicativeL(nsCSSUnit aCalcFunction,
>+ *                        float aValue1, result_type aValue2);
>+ *
>+ *   static result_type
>+ *   MergeMultiplicativeR(nsCSSUnit aCalcFunction,
>+ *                        result_type aValue1, float aValue2);

Why do we need both?  Looking at your consumers, eCSSUnit_Calc_Times_L could  simply use MergeMultiplicativeR (which would then just be called MergeMultiplicative and have args named "value" and "number" or something) and reverse the order of the two arguments when calling it.  Is there a good reason to have the extra callback?

Again, some more extensive documentation here might be nice.

>+ *   static float ComputeNumber(const nsCSSValue& aValue);

Document what the range of allowed units on aValue is here?

>+    case eCSSUnit_Calc: {
>+      nsCSSValue::Array *arr = aValue.GetArrayValue();
>+      NS_ABORT_IF_FALSE(arr->Count() == 1, "unexpected length");

Hmm.  If eCSSUnit_Calc always has only one entry, why are we using an nsCSSValue::Array for it to start with?  Why not just use an nsCSSValue (whatever one we're sticking in the array)?  I guess we want to be able to tell apart "3em" and "-moz-calc(3em)"?

>+ * A ComputeNumber implementation for callers that can assume numbers
>+ * are already normalized (i.e., anything past the parser).

So....  The callers of ComputeNumber above in fact assume numbers are already normalized.  Do we plan to add ComputeNumber callers that can't assume that?  Will they not be using ComputeCalc?

Holding off on + or - pending answers to the questions above.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:26:37 PDT
Comment on attachment 438183 [details] [diff] [review]
patch 3: calc() parsing

>+CSSParserImpl::ParseCalcAdditiveExpression(nsCSSValue& aValue,

So if I understand this correctly, the syntax this implements requires a space after every multiplicative-expression and before the following '+' and '-', right?  I assume there's an open issue on the spec to make it say so?

I think it's worth documenting the data structure this generates somewhere; in particular the fact that the association is implied by the grouping into arrays.

>+//  * If aVariantMask does not contain VARIANT_NUMBER, this function
>+//    parses the <value-multiplicative-expression> production.
>+CSSParserImpl::ParseCalcMultiplicativeExpression(nsCSSValue& aValue,

>+    if (afterDivision || gotValue) {
>+      variantMask = VARIANT_NUMBER;
>+    } else {
>+      variantMask = aVariantMask | VARIANT_NUMBER;
>+    }
>+    if (!ParseCalcTerm(*storage, variantMask))

So ParseCalcTerm either is told to parse a number or a "number or value".  Where exactly do we enforce that something like "5" isn't parsed as a <value-multiplicative-expression>?  As far as I can see, we'll parse it with ParseCalcTerm (setting |variantMask| to VARIANT_NUMBER in the process), then return true to caller.  Shouldn't we return false if !(aVariantMask & VARIANT_NUMBER) and !gotValue?  Add a test, please.

>+CSSParserImpl::ParseCalcTerm(nsCSSValue& aValue, PRInt32& aVariantMask)
>+      (mToken.mIdent.LowerCaseEqualsLiteral("min") ||
>+       mToken.mIdent.LowerCaseEqualsLiteral("max"))) {

Hmm.  So at toplevel we only accept -moz-min or -moz-max, but inside a term we only accept min or max?  I assume the former is because things are in flux and the latter is so the same value can be used for both -moz-calc and future calc?  It's a little confusing at first glance, but I guess ok....

>+CSSParserImpl::ParseCalcMinMax(nsCSSValue& aValue, nsCSSUnit aUnit,
>+  nsTArray<nsCSSValue> values;

Might make sense to use and nsAutoTArray here.  Perhaps of length 2, since I expect the two-item case to be the most common for min/max?

r=bzbarsky with the above fixed (esp the !gotValue thing in ParseCalcMultiplicativeExpression).
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:30:06 PDT
Comment on attachment 438184 [details] [diff] [review]
patch 4: calc() serialization

r=bzbarsky
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:38:02 PDT
Comment on attachment 438185 [details] [diff] [review]
patch 5: add mechanism for length-only calc() to SetCoord

r=bzbarsky
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:38:50 PDT
Comment on attachment 438186 [details] [diff] [review]
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord

r=bzbarsky
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:42:41 PDT
Comment on attachment 438187 [details] [diff] [review]
patch 7: add support for calc() to the easiest properties

r=bzbarsky
Comment 29 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-05-07 21:43:24 PDT
(In reply to comment #23)
> Hmm.  If eCSSUnit_Calc always has only one entry, why are we using an
> nsCSSValue::Array for it to start with?  Why not just use an nsCSSValue
> (whatever one we're sticking in the array)?  I guess we want to be able to tell
> apart "3em" and "-moz-calc(3em)"?

Yep.

> >+ * A ComputeNumber implementation for callers that can assume numbers
> >+ * are already normalized (i.e., anything past the parser).
> 
> So....  The callers of ComputeNumber above in fact assume numbers are already
> normalized.  Do we plan to add ComputeNumber callers that can't assume that? 
> Will they not be using ComputeCalc?

They don't assume numbers are already normalized.  By normalized I mean that any purely-numeric expression, such as (4+2) has been computed to its value 6.  Maybe I should have called it NumbersAlreadyComputed?

In the current implementation this is required to happen in in the parser (see ReduceNumberCalcOps) (which needs to happen that way so we can check for division by zero), so in all other places, anything that's a number (the left side of Times_L, or the right side of Times_R, Divided, or Modulus) is guaranteed to be a number rather than a calc expression that needs to be computed.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:44:25 PDT
Comment on attachment 438188 [details] [diff] [review]
patch 8: add support for calc() to text-shadow and -moz-box-shadow

r=bzbarsky
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 21:49:56 PDT
Comment on attachment 438189 [details] [diff] [review]
patch 9: add support for calc() to font-size

>+++ b/layout/style/nsRuleNode.cpp
>+  struct ComputeData {
>+    // All of the parameters to CalcLengthWith except aValue.

That comment isn't quite right; there are some other parameters (the nsStyleContext*, the aUseUserFontSet) that you're hardcoding instead.

r=bzbarsky with that comment fixed up.
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 22:15:56 PDT
> They don't assume numbers are already normalized.

The assert that the unit is Number.  Per irc discussion, these asserts look bogus and the only question is why the tests don't hit them (or do they?).

Also per irc discussion, something like this: 1 * 2 * 1em, which should parse as times_L(times_L(1, 2), 1em) won't end up reducing to times_L(2, 1em) in the parser.  To make that happen, we need to reduce on aValue after the divide-by-zero check (and presumably only if storage != &aValue).
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2010-05-07 22:17:55 PDT
Comment on attachment 438182 [details] [diff] [review]
patch 2: templates for computing calc trees

r=bzbarsky modulo comment 32.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-05-09 10:07:01 PDT
(In reply to comment #23)
> Why do we need both?  Looking at your consumers, eCSSUnit_Calc_Times_L could 
> simply use MergeMultiplicativeR (which would then just be called
> MergeMultiplicative and have args named "value" and "number" or something) and
> reverse the order of the two arguments when calling it.  Is there a good reason
> to have the extra callback?

I think it's worth having the callback for callers that partially compute a calc() expression but want to preserve the value * number vs. number * value distinction.  The implementation can always use the same code for both.
Comment 35 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-05-10 16:18:16 PDT
Created attachment 444502 [details] [diff] [review]
interdiff on top of patch 3: support min()/max() on numbers

While writing additional tests to address your comments, I found one additional deviation from the spec that I hadn't meant to leave in.  This interdiff fixes it; requesting review on it.
Comment 36 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-05-10 16:20:38 PDT
Created attachment 444504 [details] [diff] [review]
patch 10: remove useless (void) in nsStyleCoord

This has been bothering me for years.  I decided to get rid of it before adding more types.

These mean the same thing in C++:
  int foo(void);
  int foo();
(though they're different in C, where the second means that the parameters are unknown or something like that).
Comment 37 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-05-10 16:21:36 PDT
Created attachment 444505 [details] [diff] [review]
patch 11: make CalcOps methods non-static and just instantiate ops rather than ComputeData

This simplifies the use of CalcOps a good bit, and adds more flexibility that will be needed for the calc() + nsStyleCoord changes.
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2010-05-10 19:56:09 PDT
Comment on attachment 444502 [details] [diff] [review]
interdiff on top of patch 3: support min()/max() on numbers

r=bzbarsky
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2010-05-10 20:22:24 PDT
Comment on attachment 444504 [details] [diff] [review]
patch 10: remove useless (void) in nsStyleCoord

r=bzbarsky
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2010-05-10 20:26:16 PDT
Comment on attachment 444505 [details] [diff] [review]
patch 11: make CalcOps methods non-static and just instantiate ops rather than ComputeData

r=bzbarsky
Comment 42 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-15 22:37:28 PDT
Created attachment 451482 [details] [diff] [review]
patch 12: add nsStyleContext::Alloc for allocations scoped to the lifetime of the style context

This lets me avoid giving nsStyleCoord a nontrivial destructor (which would in turn require style structs have nontrivial destructors), which would be a hit both in performance and code complexity.
Comment 43 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-15 22:38:09 PDT
Created attachment 451483 [details] [diff] [review]
patch 13: add nsStyleCoord::Array
Comment 44 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-15 22:38:50 PDT
Created attachment 451485 [details] [diff] [review]
patch 14: add storage for calc() expressions to nsStyleCoord
Comment 45 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-15 22:39:19 PDT
Created attachment 451486 [details] [diff] [review]
patch 15: fix namespace usage in nsRuleNode
Comment 46 William J. Edney 2010-06-24 04:32:54 PDT
Just wanted to let you all know that IE9 Preview #3 now implements calc() ;-).

Note that they went with the 'regular CSS spelling' and didn't put a vendor prefix on it.

Also, this bug for Webkit:

https://bugs.webkit.org/show_bug.cgi?id=16662

shows that the Webkit crowd will (if they move ahead), just use 'calc()' as well (without the vendor prefix).

Thanks!

Cheers,

- Bill
Comment 47 d 2010-06-24 04:52:15 PDT
But no vendor is allowed to ship a final version without the prefix, since the specification is still in Working Draft status, right?
Comment 48 David Smith 2010-06-24 08:00:32 PDT
(In reply to comment #46)
> Also, this bug for Webkit:
> 
> https://bugs.webkit.org/show_bug.cgi?id=16662
> 
> shows that the Webkit crowd will (if they move ahead), just use 'calc()' as
> well (without the vendor prefix).

The WebKit bug shows no such thing. My initial prototype patch implemented -webkit-calc() as is required.
Comment 49 William J. Edney 2010-06-24 08:15:47 PDT
Oops. Sorry - you're correct of course. Too little caffeine this morning ;-).
Comment 50 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-25 17:29:07 PDT
Created attachment 454182 [details] [diff] [review]
patch 16: switch nscoord math to saturating operations
Comment 51 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-25 17:33:10 PDT
Created attachment 454184 [details] [diff] [review]
patch 17: build computed-value calc() expressions and store them in nsStyleCoord
Comment 52 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-25 17:33:55 PDT
Created attachment 454185 [details] [diff] [review]
patch 18: make ComputeCalc more general so its input can be nsCSSValue or nsStyleCoord
Comment 53 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-25 17:35:10 PDT
Created attachment 454187 [details] [diff] [review]
patch 19: add API to compute computed-value calc() expressions to lengths
Comment 54 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-25 17:35:52 PDT
Created attachment 454188 [details] [diff] [review]
patch 20: add API to compute length, percentage, or calc() on nsStyleCoord
Comment 55 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-25 17:37:12 PDT
Created attachment 454189 [details] [diff] [review]
patch 21: make calc() serialization code a template so it can be used for nsStyleCoord or nsCSSValue

This converts the existing nsCSSValue user to the general mechanism so that I can add a new nsStyleCoord user in the next patch.
Comment 56 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-25 17:38:22 PDT
Created attachment 454190 [details] [diff] [review]
patch 22: handle computed-value calc() expressions in nsComputedDOMStyle
Comment 57 Boris Zbarsky [:bz] (still a bit busy) 2010-06-28 23:05:40 PDT
I'm not sure I buy the comments in patch 12 about why it's ok to use the per-context storage for structs.  What about structs that are cached in the ruletree and outlive the style context they're first computed for?
Comment 58 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-28 23:30:36 PDT
Right.  I guess I need to either (a) set canStoreInRuleTree to false when using nsStyleContext::Alloc or (b) change it to nsRuleNode::Alloc (and, in some cases, make the allocations last longer than needed).  I guess I'm inclined towards (a).
Comment 59 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-06-28 23:32:17 PDT
Or, (c), come up with a style-struct-based solution.
Comment 60 Boris Zbarsky [:bz] (still a bit busy) 2010-06-29 06:31:05 PDT
In the short run, (a) sounds best, I think.  At least it should be simplest to do.

The one drawback of (c) is that you'd need one pointer per struct that might have calc stuff, right?  Are there any other drawbacks?
Comment 61 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-07-01 09:28:31 PDT
I implemented (a) as modifications to three of the patches:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/531ac7b57d35
Comment 62 Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 14:34:20 PDT
Comment on attachment 451482 [details] [diff] [review]
patch 12: add nsStyleContext::Alloc for allocations scoped to the lifetime of the style context

s/this contexts/this context's/.  r=bzbarsky
Comment 63 Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 14:37:05 PDT
Comment on attachment 451483 [details] [diff] [review]
patch 13: add nsStyleCoord::Array

r=bzbarsky
Comment 64 Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 14:46:21 PDT
Comment on attachment 451485 [details] [diff] [review]
patch 14: add storage for calc() expressions to nsStyleCoord

Please document the ownership model for mPointer at least for array values (in this case, that the style context owns it)?
Comment 65 Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 15:01:17 PDT
Comment on attachment 454184 [details] [diff] [review]
patch 17: build computed-value calc() expressions and store them in nsStyleCoord

r=bzbarsky
Comment 66 Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 15:32:34 PDT
Comment on attachment 454189 [details] [diff] [review]
patch 21: make calc() serialization code a template so it can be used for nsStyleCoord or nsCSSValue

>+      // When we make recursive calls, we pass eCSSProperty_UNKNOWN as
>+      // the property so we can distinguish min() and max() at toplevel
>+      // (where we need to serialize with a -moz- prefix) from min() and
>+      // max() within calc() (where we don't).
>+      SerializeCalcInternal(array->Item(i), aOps);

This comment doesn't make sense.  We're no longer making recursive calls on AppendCSSValueToString for the calc-unit cssvalues, so the problem just doesn't arise, right?

r=bzbarsky with that comment removed or changed to make sense.
Comment 67 Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 15:38:53 PDT
Comment on attachment 454190 [details] [diff] [review]
patch 22: handle computed-value calc() expressions in nsComputedDOMStyle

This looks ok as long as we're still doing the whole "return used style if you can" thing for getComputedStyle....
Comment 68 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-07-01 15:42:07 PDT
(In reply to comment #67)
> (From update of attachment 454190 [details] [diff] [review])
> This looks ok as long as we're still doing the whole "return used style if you
> can" thing for getComputedStyle....

Yes; we only serialize when we can't get a basis for percentages, and we should only even hit this code for the layout-related properties when we don't have a frame.
Comment 69 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-07-01 15:50:01 PDT
(In reply to comment #66)
> This comment doesn't make sense.  We're no longer making recursive calls on
> AppendCSSValueToString for the calc-unit cssvalues, so the problem just doesn't
> arise, right?

Yeah, it's now handled by the distinction between SerializeCalc and SerializeCalcInternal.
Comment 70 Zack Weinberg (:zwol) 2010-07-01 16:13:17 PDT
... you just broke my patches that move AppendCSSValueToString, again, didn't you. ;-/
Comment 71 Masatoshi Kimura [:emk] 2010-07-02 17:06:00 PDT
(In reply to comment #47)
> But no vendor is allowed to ship a final version without the prefix, since the
> specification is still in Working Draft status, right?
CSS Text is still in Working Draft status. Why the text-shadow do not have a vendor prefix? (Answer: Acid3 tested the property)
CSS Color Module is still in Working Draft status. Why the opacity was removed a vendor prefix?
Comment 72 Patrick Dark 2010-07-02 17:22:56 PDT
(In reply to comment #71)
> CSS Text is still in Working Draft status. Why the text-shadow do not have a
> vendor prefix? (Answer: Acid3 tested the property)

The |text-shadow| property was also defined in CSS2, a W3C Recommendation since 2008. See <http://www.w3.org/TR/2008/REC-CSS2-20080411/text.html#propdef-text-shadow>.

> CSS Color Module is still in Working Draft status. Why the opacity was removed
> a vendor prefix?

CSS3 Color was a W3C Candidate Recommendation between 2003 and 2008. See <http://www.w3.org/TR/2003/CR-css3-color-20030514/>.

You don't need prefixes for either of those.

I'm not sure that it even matters since the spec that says you should have them is itself a W3C Working Draft that hasn't been touched for seven years (CSS3 Syntax: <http://www.w3.org/TR/2003/WD-css3-syntax-20030813/#vendor-specific>) and it doesn't mention property values at all.
Comment 73 Boris Zbarsky [:bz] (still a bit busy) 2010-07-02 17:26:53 PDT
> (Answer: Acid3 tested the property)

No, answer is that CSS3 Text was a Candidate Recommendation in 2003; see <http://www.w3.org/TR/2003/CR-css3-text-20030514/>.  Then text-shadow was implemented without prefixes.  Then CSS3 Text went back to working draft for various reasons, but the property was already implemented without prefixes by then.

> Why the opacity was removed a vendor prefix?

Because CSS3 Color was a Candidate Recommendation in 2003; see <http://www.w3.org/TR/2003/CR-css3-color-20030514/>.  Then opacity was implemented without prefixes.  Then CSS3 Color went back to working draft, but the property was already implemented without prefixes by then.

In general, Google has the answers to questions like that (e.g. those links above are the second hits on "CSS3 Text" and "CSS3 Color" respectively).
Comment 74 Boris Zbarsky [:bz] (still a bit busy) 2010-07-02 17:27:40 PDT
One more note.  Your Acid3 causality is backwards: Acid3 only tested things that were in REC, PR, or CR as of before the creation of the test.
Comment 75 Patrick Dark 2010-07-02 17:32:14 PDT
Sorry, a modern spec (CSS2.1 (CR)) does mention property values: <http://www.w3.org/TR/CSS2/syndata.html#vendor-keywords>. It only indicates that prefixes apply to keywords though (and the |calc| notation isn't a keyword per the previous paragraph).
Comment 76 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-07-02 22:12:33 PDT
I think if a spec only mentioned prefixing certain things, it was only lack of imagination, not intent that other things not be prefixed.

In any case, I landed the rest of the infrastructure for calc() that needs to be propagated to computed values (but no user-visible changes, which I'll probably put in a different bug):
http://hg.mozilla.org/mozilla-central/rev/fb5c5b8e14d9
http://hg.mozilla.org/mozilla-central/rev/f0cff020cf0b
http://hg.mozilla.org/mozilla-central/rev/caa87f845650
http://hg.mozilla.org/mozilla-central/rev/d9f9e02891fc
http://hg.mozilla.org/mozilla-central/rev/a2ac427a701e
http://hg.mozilla.org/mozilla-central/rev/c60f3194eae5
http://hg.mozilla.org/mozilla-central/rev/b8ebd422dd03
http://hg.mozilla.org/mozilla-central/rev/c70900796669
http://hg.mozilla.org/mozilla-central/rev/4a1e71092c02
http://hg.mozilla.org/mozilla-central/rev/1eb53bf4a941
http://hg.mozilla.org/mozilla-central/rev/f065d64675cc

Still one more big chunk of code needed before calc() on 'width', etc.
Comment 77 Mike Beltzner [:beltzner, not reading bugmail] 2010-08-30 08:48:15 PDT
Looks like this is slipping to beta6+, yes?
Comment 78 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-30 09:19:21 PDT
Fine with beta6, now that beta6 is feature freeze.
Comment 79 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-07 14:10:42 PDT
Created attachment 472765 [details] [diff] [review]
add outline-width and -moz-column-gap calc() support

It turns out I missed outline-width and -moz-column-gap.
Comment 80 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-07 14:11:25 PDT
Created attachment 472766 [details] [diff] [review]
fix calc() on border-*-width

... and I also missed half the code for border-*-width
Comment 81 Boris Zbarsky [:bz] (still a bit busy) 2010-09-08 13:32:36 PDT
Comment on attachment 472765 [details] [diff] [review]
add outline-width and -moz-column-gap calc() support

r=me
Comment 82 Boris Zbarsky [:bz] (still a bit busy) 2010-09-08 13:32:41 PDT
Comment on attachment 472766 [details] [diff] [review]
fix calc() on border-*-width

r=me
Comment 83 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-09 11:21:14 PDT
Created attachment 473628 [details] [diff] [review]
remove min() and max() parsing and storage

There's a *lot* of code simplification to come as followup, but this is the patch that removes parsing, storage, and serialization of min() and max().
Comment 84 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-09 12:26:10 PDT
Created attachment 473665 [details] [diff] [review]
change nsStyleCoord calc() storage as a result of removing min() and max()

... and remove a bunch of CalcOps classes as a result
Comment 85 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-09 14:08:22 PDT
outline-width/column-gap and border-*-width patches landed:
http://hg.mozilla.org/mozilla-central/rev/36bcecd7cd32
http://hg.mozilla.org/mozilla-central/rev/dd7bbf7febcb
Comment 86 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-10 10:27:05 PDT
Created attachment 474111 [details] [diff] [review]
change nsStyleCoord calc() storage as a result of removing min() and max()

revised to fix a few small errors caught by running tests
Comment 87 Boris Zbarsky [:bz] (still a bit busy) 2010-09-10 11:20:32 PDT
Comment on attachment 473628 [details] [diff] [review]
remove min() and max() parsing and storage

>+++ b/layout/style/nsCSSParser.cpp
>-#define VARIANT_CALC_NO_MIN_MAX 0x08000000 // no min() and max() for calc()
> #define VARIANT_ELEMENT       0x10000000  // eCSSUnit_Element

Change the value of VARIANT_ELEMENT?

r=me with that; I more or less skimmed the tests....
Comment 88 Boris Zbarsky [:bz] (still a bit busy) 2010-09-10 11:44:07 PDT
Is it worth trying to keep the more complicated computed value storage around for now for in case we add other stuff to calc() (people keep talking about adding other things there)?  Or do we figure we can restore it as needed?
Comment 89 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-10 11:46:33 PDT
I think we can add other stuff as needed.  Also, some of the other stuff we might want to add would be additional value types (like 'auto'), which wouldn't necessarily call for tree storage, just more things to multiply (and potentially a more flexible array).

(The other suggestion that came up was keeping min() and max() but disallowing percents; just allowing them on lengths or numbers.  That seems a bit confusing for authors, though.)
Comment 90 Zack Weinberg (:zwol) 2010-09-10 12:05:58 PDT
Out of morbid curiosity, where was dropping min/max discussed?  I don't see anything relevant on a quick skim of the last two months' archives of www-style and I didn't see it go by anywhere else either.
Comment 91 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-10 15:50:02 PDT
There is a thread in www-style in the last few days.
Comment 92 Boris Zbarsky [:bz] (still a bit busy) 2010-09-10 20:08:13 PDT
Comment on attachment 474111 [details] [diff] [review]
change nsStyleCoord calc() storage as a result of removing min() and max()

>+++ b/layout/style/nsRuleNode.cpp
>+      return result_type(0, aValue.GetPercentValue());
>+    } else {
>+      return result_type(CalcLength(aValue, mContext, mPresContext,

else after return here.

r=bzbarsky
Comment 93 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-11 10:13:09 PDT
min-max removal landed:
http://hg.mozilla.org/mozilla-central/rev/5ab6d6489a64
http://hg.mozilla.org/mozilla-central/rev/26bfc0860822

As I said in bug 585715 comment 87, I filed three followup bugs for remaining values to be done:
 * bug 594933 for properties that have numbers in them
 * bug 594934 for the background-position-like properties
 * bug 594935 for gradient stop positions
Based on author feedback about what was important, I may try to get to bug 594934 for this release if I have time, but the others are highly unlikely.

I think this bug, though, is FIXED at this point.
Comment 95 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-12 22:37:21 PDT
Created attachment 474621 [details] [diff] [review]
another piece of min/max code to remove

No point reducing tree depth when there's no tree.
Comment 96 Boris Zbarsky [:bz] (still a bit busy) 2010-09-15 15:32:34 PDT
Comment on attachment 474621 [details] [diff] [review]
another piece of min/max code to remove

r=me
Comment 97 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-17 12:49:48 PDT
additional removal landed:
http://hg.mozilla.org/mozilla-central/rev/e98ce9f4ddbb
Comment 98 Eric Shepherd [:sheppy] 2010-10-09 08:59:17 PDT
Now documented:

https://developer.mozilla.org/en/CSS/-moz-calc

Note You need to log in before you can comment on or make changes to this bug.