implement css3-values calc()

RESOLVED FIXED

Status

()

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

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 -

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(URL)

Attachments

(28 attachments, 9 obsolete attachments)

7.40 KB, patch
Details | Diff | Splinter Review
8.42 KB, patch
Details | Diff | Splinter Review
17.48 KB, patch
Details | Diff | Splinter Review
5.95 KB, patch
Details | Diff | Splinter Review
10.72 KB, patch
Details | Diff | Splinter Review
2.38 KB, patch
Details | Diff | Splinter Review
24.15 KB, patch
Details | Diff | Splinter Review
9.37 KB, patch
Details | Diff | Splinter Review
7.58 KB, patch
Details | Diff | Splinter Review
6.95 KB, patch
Details | Diff | Splinter Review
9.54 KB, patch
Details | Diff | Splinter Review
19.18 KB, patch
Details | Diff | Splinter Review
5.88 KB, patch
Details | Diff | Splinter Review
5.78 KB, patch
Details | Diff | Splinter Review
6.65 KB, patch
Details | Diff | Splinter Review
4.21 KB, patch
Details | Diff | Splinter Review
3.60 KB, patch
Details | Diff | Splinter Review
9.32 KB, patch
Details | Diff | Splinter Review
7.07 KB, patch
Details | Diff | Splinter Review
3.71 KB, patch
Details | Diff | Splinter Review
3.19 KB, patch
Details | Diff | Splinter Review
12.13 KB, patch
Details | Diff | Splinter Review
5.65 KB, patch
Details | Diff | Splinter Review
7.53 KB, patch
Details | Diff | Splinter Review
10.43 KB, patch
Details | Diff | Splinter Review
86.19 KB, patch
Details | Diff | Splinter Review
43.19 KB, patch
Details | Diff | Splinter Review
1.36 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
We should implement calc() from css3-values:
http://www.w3.org/TR/css3-values/#calc
(Assignee)

Updated

10 years ago
QA Contact: ian → style-system
(Assignee)

Updated

9 years ago
Assignee: dbaron → nobody

Updated

8 years ago
Flags: wanted1.9.2?
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Blocks: 520234
(Assignee)

Updated

8 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Blocks: 536539
(Assignee)

Comment 1

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

Comment 2

8 years ago
Created attachment 419838 [details] [diff] [review]
patch 1: add calc() types to nsCSSValue
Attachment #419838 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

8 years ago
Created attachment 419839 [details] [diff] [review]
patch 2: templates for computing calc trees
Attachment #419839 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

8 years ago
Created attachment 419840 [details] [diff] [review]
patch 3: calc() parsing
Attachment #419840 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

8 years ago
Created attachment 419841 [details] [diff] [review]
patch 4: calc() serialization
Attachment #419841 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

8 years ago
Created attachment 419842 [details] [diff] [review]
patch 5: add mechanism for length-only calc() to SetCoord
Attachment #419842 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

8 years ago
Created attachment 419843 [details] [diff] [review]
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord
Attachment #419843 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

8 years ago
Created attachment 419844 [details] [diff] [review]
patch 7: add support for calc() to the easiest properties
Attachment #419844 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

8 years ago
Created attachment 419845 [details] [diff] [review]
patch 8: add support for calc() to text-shadow and -moz-box-shadow
Attachment #419845 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

8 years ago
Created attachment 419846 [details] [diff] [review]
patch 9: add support for calc() to font-size
Attachment #419846 - Flags: review?(bzbarsky)
Comment on attachment 419838 [details] [diff] [review]
patch 1: add calc() types to nsCSSValue

r=bzbarsky
Attachment #419838 - Flags: review?(bzbarsky) → review+
Keywords: dev-doc-needed

Comment 12

7 years ago
Are the patches ready to be pushed, for early support, and, which properties are left to support?
(Assignee)

Comment 13

7 years ago
No.  I need to update a bunch per CSS WG discussion (and also write up a spec proposal to reflect that discussion).
Attachment #419839 - Flags: review?(bzbarsky)
Attachment #419840 - Flags: review?(bzbarsky)
Attachment #419841 - Flags: review?(bzbarsky)
Attachment #419842 - Flags: review?(bzbarsky)
Attachment #419843 - Flags: review?(bzbarsky)
Attachment #419844 - Flags: review?(bzbarsky)
Attachment #419845 - Flags: review?(bzbarsky)
Attachment #419846 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

7 years ago
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
Attachment #419839 - Attachment is obsolete: true
Attachment #438182 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

7 years ago
Created attachment 438183 [details] [diff] [review]
patch 3: calc() parsing
Attachment #419840 - Attachment is obsolete: true
Attachment #438183 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

7 years ago
Created attachment 438184 [details] [diff] [review]
patch 4: calc() serialization
Attachment #419841 - Attachment is obsolete: true
Attachment #438184 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

7 years ago
Created attachment 438185 [details] [diff] [review]
patch 5: add mechanism for length-only calc() to SetCoord
Attachment #419842 - Attachment is obsolete: true
Attachment #438185 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

7 years ago
Created attachment 438186 [details] [diff] [review]
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord
Attachment #419843 - Attachment is obsolete: true
Attachment #438186 - Flags: review?(bzbarsky)
(Assignee)

Comment 19

7 years ago
Created attachment 438187 [details] [diff] [review]
patch 7: add support for calc() to the easiest properties
Attachment #419844 - Attachment is obsolete: true
Attachment #438187 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

7 years ago
Created attachment 438188 [details] [diff] [review]
patch 8: add support for calc() to text-shadow and -moz-box-shadow
Attachment #419845 - Attachment is obsolete: true
Attachment #438188 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

7 years ago
Created attachment 438189 [details] [diff] [review]
patch 9: add support for calc() to font-size
Attachment #419846 - Attachment is obsolete: true
Attachment #438189 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #438186 - Attachment description: add mechanism for clamping nonnegative calc()s to SetCoord → patch 6: add mechanism for clamping nonnegative calc()s to SetCoord
(Assignee)

Updated

7 years ago
Attachment #438188 - Attachment description: add support for calc() to text-shadow and -moz-box-shadow → patch 8: add support for calc() to text-shadow and -moz-box-shadow
(Assignee)

Comment 22

7 years ago
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 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 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).
Attachment #438183 - Flags: review?(bzbarsky) → review+
Comment on attachment 438184 [details] [diff] [review]
patch 4: calc() serialization

r=bzbarsky
Attachment #438184 - Flags: review?(bzbarsky) → review+
Comment on attachment 438185 [details] [diff] [review]
patch 5: add mechanism for length-only calc() to SetCoord

r=bzbarsky
Attachment #438185 - Flags: review?(bzbarsky) → review+
Attachment #438186 - Flags: review?(bzbarsky) → review+
Comment on attachment 438186 [details] [diff] [review]
patch 6: add mechanism for clamping nonnegative calc()s to SetCoord

r=bzbarsky
Comment on attachment 438187 [details] [diff] [review]
patch 7: add support for calc() to the easiest properties

r=bzbarsky
Attachment #438187 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 29

7 years ago
(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 on attachment 438188 [details] [diff] [review]
patch 8: add support for calc() to text-shadow and -moz-box-shadow

r=bzbarsky
Attachment #438188 - Flags: review?(bzbarsky) → review+
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.
Attachment #438189 - Flags: review?(bzbarsky) → review+
> 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 on attachment 438182 [details] [diff] [review]
patch 2: templates for computing calc trees

r=bzbarsky modulo comment 32.
Attachment #438182 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 34

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

Comment 35

7 years ago
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.
Attachment #444502 - Flags: review?(bzbarsky)
(Assignee)

Comment 36

7 years ago
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).
Attachment #444504 - Flags: review?(bzbarsky)
(Assignee)

Comment 37

7 years ago
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.
Attachment #444505 - Flags: review?(bzbarsky)
Comment on attachment 444502 [details] [diff] [review]
interdiff on top of patch 3: support min()/max() on numbers

r=bzbarsky
Attachment #444502 - Flags: review?(bzbarsky) → review+
Comment on attachment 444504 [details] [diff] [review]
patch 10: remove useless (void) in nsStyleCoord

r=bzbarsky
Attachment #444504 - Flags: review?(bzbarsky) → review+
Comment on attachment 444505 [details] [diff] [review]
patch 11: make CalcOps methods non-static and just instantiate ops rather than ComputeData

r=bzbarsky
Attachment #444505 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 41

7 years ago
I landed the following 12 patches:
http://hg.mozilla.org/mozilla-central/rev/46655a6b532e
http://hg.mozilla.org/mozilla-central/rev/155737744a68
http://hg.mozilla.org/mozilla-central/rev/564caae3da4b
http://hg.mozilla.org/mozilla-central/rev/bd771ce9a597
http://hg.mozilla.org/mozilla-central/rev/ccfcfde74733
http://hg.mozilla.org/mozilla-central/rev/bf73d1295b5e
http://hg.mozilla.org/mozilla-central/rev/c5da51a04d80
http://hg.mozilla.org/mozilla-central/rev/006565bfdc46
http://hg.mozilla.org/mozilla-central/rev/858463a7c089
http://hg.mozilla.org/mozilla-central/rev/f4b1e12bf450
http://hg.mozilla.org/mozilla-central/rev/84e571efb1d2
http://hg.mozilla.org/mozilla-central/rev/05f3c68e73c9

This means -moz-calc() now (i.e., starting in tomorrow's nightly mozilla-central builds) works for a small set of properties, but it doesn't work for any of the actually interesting cases yet (cases where % depends on layout).  That's still to come.

Updated

7 years ago
Depends on: 565248
Blocks: 569993
(Assignee)

Comment 42

7 years ago
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.
Attachment #451482 - Flags: review?(bzbarsky)
(Assignee)

Comment 43

7 years ago
Created attachment 451483 [details] [diff] [review]
patch 13: add nsStyleCoord::Array
Attachment #451483 - Flags: review?(bzbarsky)
(Assignee)

Comment 44

7 years ago
Created attachment 451485 [details] [diff] [review]
patch 14: add storage for calc() expressions to nsStyleCoord
Attachment #451485 - Flags: review?(bzbarsky)
(Assignee)

Comment 45

7 years ago
Created attachment 451486 [details] [diff] [review]
patch 15: fix namespace usage in nsRuleNode
Attachment #451486 - Flags: review?(bzbarsky)

Comment 46

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

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

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

7 years ago
Oops. Sorry - you're correct of course. Too little caffeine this morning ;-).
(Assignee)

Comment 50

7 years ago
Created attachment 454182 [details] [diff] [review]
patch 16: switch nscoord math to saturating operations
Attachment #454182 - Flags: review?(bzbarsky)
(Assignee)

Comment 51

7 years ago
Created attachment 454184 [details] [diff] [review]
patch 17: build computed-value calc() expressions and store them in nsStyleCoord
Attachment #454184 - Flags: review?(bzbarsky)
(Assignee)

Comment 52

7 years ago
Created attachment 454185 [details] [diff] [review]
patch 18: make ComputeCalc more general so its input can be nsCSSValue or nsStyleCoord
Attachment #454185 - Flags: review?(bzbarsky)
(Assignee)

Comment 53

7 years ago
Created attachment 454187 [details] [diff] [review]
patch 19: add API to compute computed-value calc() expressions to lengths
Attachment #454187 - Flags: review?(bzbarsky)
(Assignee)

Comment 54

7 years ago
Created attachment 454188 [details] [diff] [review]
patch 20: add API to compute length, percentage, or calc() on nsStyleCoord
Attachment #454188 - Flags: review?(bzbarsky)
(Assignee)

Comment 55

7 years ago
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.
Attachment #454189 - Flags: review?(bzbarsky)
(Assignee)

Comment 56

7 years ago
Created attachment 454190 [details] [diff] [review]
patch 22: handle computed-value calc() expressions in nsComputedDOMStyle
Attachment #454190 - Flags: review?(bzbarsky)
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?
(Assignee)

Comment 58

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

Comment 59

7 years ago
Or, (c), come up with a style-struct-based solution.
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?
(Assignee)

Comment 61

7 years ago
I implemented (a) as modifications to three of the patches:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/531ac7b57d35
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
Attachment #451482 - Flags: review?(bzbarsky) → review+
Comment on attachment 451483 [details] [diff] [review]
patch 13: add nsStyleCoord::Array

r=bzbarsky
Attachment #451483 - Flags: review?(bzbarsky) → review+
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)?
Attachment #451485 - Flags: review?(bzbarsky) → review+
Attachment #451486 - Flags: review?(bzbarsky) → review+
Attachment #454182 - Flags: review?(bzbarsky) → review+
Comment on attachment 454184 [details] [diff] [review]
patch 17: build computed-value calc() expressions and store them in nsStyleCoord

r=bzbarsky
Attachment #454184 - Flags: review?(bzbarsky) → review+
Attachment #454185 - Flags: review?(bzbarsky) → review+
Attachment #454187 - Flags: review?(bzbarsky) → review+
Attachment #454188 - Flags: review?(bzbarsky) → review+
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.
Attachment #454189 - Flags: review?(bzbarsky) → review+
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....
Attachment #454190 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 68

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

Comment 69

7 years ago
(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.
... you just broke my patches that move AppendCSSValueToString, again, didn't you. ;-/
(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

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

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

Comment 76

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

Updated

7 years ago
Blocks: 579683
Blocks: 571489
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
blocking2.0: ? → beta5+
(Assignee)

Updated

7 years ago
Depends on: 585715

Updated

7 years ago
No longer blocks: 579683
Looks like this is slipping to beta6+, yes?
blocking2.0: beta5+ → beta6+
(Assignee)

Comment 78

7 years ago
Fine with beta6, now that beta6 is feature freeze.
(Assignee)

Comment 79

7 years ago
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.
Attachment #472765 - Flags: review?(bzbarsky)
(Assignee)

Comment 80

7 years ago
Created attachment 472766 [details] [diff] [review]
fix calc() on border-*-width

... and I also missed half the code for border-*-width
Attachment #472766 - Flags: review?(bzbarsky)
Comment on attachment 472765 [details] [diff] [review]
add outline-width and -moz-column-gap calc() support

r=me
Attachment #472765 - Flags: review?(bzbarsky) → review+
Comment on attachment 472766 [details] [diff] [review]
fix calc() on border-*-width

r=me
Attachment #472766 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 83

7 years ago
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().
Attachment #473628 - Flags: review?(bzbarsky)
(Assignee)

Comment 84

7 years ago
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
Attachment #473665 - Flags: review?(bzbarsky)
(Assignee)

Comment 85

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

Comment 86

7 years ago
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
Attachment #473665 - Attachment is obsolete: true
Attachment #474111 - Flags: review?(bzbarsky)
Attachment #473665 - Flags: review?(bzbarsky)
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....
Attachment #473628 - Flags: review?(bzbarsky) → review+
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?
(Assignee)

Comment 89

7 years ago
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.)
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.
There is a thread in www-style in the last few days.
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
Attachment #474111 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 93

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 94

7 years ago
@Comment 90: The discussions were at:

1. http://lists.w3.org/Archives/Public/www-style/2010Sep/0003.html (two places)
2. http://lists.w3.org/Archives/Public/www-style/2010Sep/0233.html
(Assignee)

Comment 95

7 years ago
Created attachment 474621 [details] [diff] [review]
another piece of min/max code to remove

No point reducing tree depth when there's no tree.
Attachment #474621 - Flags: review?(bzbarsky)
Comment on attachment 474621 [details] [diff] [review]
another piece of min/max code to remove

r=me
Attachment #474621 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 97

7 years ago
additional removal landed:
http://hg.mozilla.org/mozilla-central/rev/e98ce9f4ddbb
Now documented:

https://developer.mozilla.org/en/CSS/-moz-calc
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 594933

Updated

5 years ago
Blocks: 741643

Updated

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