Closed Bug 363249 Opened 18 years ago Closed 14 years ago

implement css3-values calc()

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(28 files, 9 obsolete files)

7.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
17.48 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.72 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
24.15 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
19.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.65 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.60 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.32 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.07 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.71 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.13 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.65 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
86.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
43.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.36 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
QA Contact: ian → style-system
Assignee: dbaron → nobody
Flags: wanted1.9.2?
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Blocks: 520234
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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 on attachment 419838 [details] [diff] [review]
patch 1: add calc() types to nsCSSValue

r=bzbarsky
Attachment #419838 - Flags: review?(bzbarsky) → review+
Are the patches ready to be pushed, for early support, and, which properties are left to support?
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)
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)
Attachment #419840 - Attachment is obsolete: true
Attachment #438183 - Flags: review?(bzbarsky)
Attachment #419841 - Attachment is obsolete: true
Attachment #438184 - Flags: review?(bzbarsky)
Attachment #438186 - Attachment description: add mechanism for clamping nonnegative calc()s to SetCoord → patch 6: add mechanism for clamping nonnegative calc()s to SetCoord
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
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+
(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+
(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.
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)
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)
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+
Depends on: 565248
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)
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
But no vendor is allowed to ship a final version without the prefix, since the specification is still in Working Draft status, right?
(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.
Oops. Sorry - you're correct of course. Too little caffeine this morning ;-).
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)
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?
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).
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?
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+
(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.
(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?
(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.
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).
Blocks: 579683
Blocks: 571489
blocking2.0: --- → ?
blocking2.0: ? → beta5+
No longer blocks: 579683
Looks like this is slipping to beta6+, yes?
blocking2.0: beta5+ → beta6+
Fine with beta6, now that beta6 is feature freeze.
It turns out I missed outline-width and -moz-column-gap.
Attachment #472765 - Flags: review?(bzbarsky)
... 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+
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)
... and remove a bunch of CalcOps classes as a result
Attachment #473665 - Flags: review?(bzbarsky)
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?
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+
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
Closed: 14 years ago
Resolution: --- → FIXED
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+
Blocks: 594933
Blocks: css-values-3
Depends on: 776443
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: