Closed
Bug 363249
Opened 18 years ago
Closed 14 years ago
implement css3-values calc()
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
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)
We should implement calc() from css3-values: http://www.w3.org/TR/css3-values/#calc
Assignee | ||
Updated•17 years ago
|
QA Contact: ian → style-system
Assignee | ||
Updated•16 years ago
|
Assignee: dbaron → nobody
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 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•14 years ago
|
||
Attachment #419838 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #419839 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #419840 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #419841 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #419842 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #419843 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #419844 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #419845 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #419846 -
Flags: review?(bzbarsky)
Comment 11•14 years ago
|
||
Comment on attachment 419838 [details] [diff] [review] patch 1: add calc() types to nsCSSValue r=bzbarsky
Attachment #419838 -
Flags: review?(bzbarsky) → review+
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 12•14 years ago
|
||
Are the patches ready to be pushed, for early support, and, which properties are left to support?
Assignee | ||
Comment 13•14 years ago
|
||
No. I need to update a bunch per CSS WG discussion (and also write up a spec proposal to reflect that discussion).
Updated•14 years ago
|
Attachment #419839 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #419840 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #419841 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #419842 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #419843 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #419844 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #419845 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #419846 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•14 years ago
|
||
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•14 years ago
|
||
Attachment #419840 -
Attachment is obsolete: true
Attachment #438183 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #419841 -
Attachment is obsolete: true
Attachment #438184 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #419842 -
Attachment is obsolete: true
Attachment #438185 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #419843 -
Attachment is obsolete: true
Attachment #438186 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 19•14 years ago
|
||
Attachment #419844 -
Attachment is obsolete: true
Attachment #438187 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #419845 -
Attachment is obsolete: true
Attachment #438188 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 21•14 years ago
|
||
Attachment #419846 -
Attachment is obsolete: true
Attachment #438189 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 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•14 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•14 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 23•14 years ago
|
||
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•14 years ago
|
||
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 25•14 years ago
|
||
Comment on attachment 438184 [details] [diff] [review] patch 4: calc() serialization r=bzbarsky
Attachment #438184 -
Flags: review?(bzbarsky) → review+
Comment 26•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #438186 -
Flags: review?(bzbarsky) → review+
Comment 27•14 years ago
|
||
Comment on attachment 438186 [details] [diff] [review] patch 6: add mechanism for clamping nonnegative calc()s to SetCoord r=bzbarsky
Comment 28•14 years ago
|
||
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•14 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 30•14 years ago
|
||
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 31•14 years ago
|
||
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+
Comment 32•14 years ago
|
||
> 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•14 years ago
|
||
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•14 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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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 38•14 years ago
|
||
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 39•14 years ago
|
||
Comment on attachment 444504 [details] [diff] [review] patch 10: remove useless (void) in nsStyleCoord r=bzbarsky
Attachment #444504 -
Flags: review?(bzbarsky) → review+
Comment 40•14 years ago
|
||
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•14 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.
Assignee | ||
Comment 42•14 years ago
|
||
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•14 years ago
|
||
Attachment #451483 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 44•14 years ago
|
||
Attachment #451485 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 45•14 years ago
|
||
Attachment #451486 -
Flags: review?(bzbarsky)
Comment 46•14 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•14 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•14 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•14 years ago
|
||
Oops. Sorry - you're correct of course. Too little caffeine this morning ;-).
Assignee | ||
Comment 50•14 years ago
|
||
Attachment #454182 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 51•14 years ago
|
||
Attachment #454184 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 52•14 years ago
|
||
Attachment #454185 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 53•14 years ago
|
||
Attachment #454187 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 54•14 years ago
|
||
Attachment #454188 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 55•14 years ago
|
||
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•14 years ago
|
||
Attachment #454190 -
Flags: review?(bzbarsky)
Comment 57•14 years ago
|
||
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•14 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•14 years ago
|
||
Or, (c), come up with a style-struct-based solution.
Comment 60•14 years ago
|
||
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•14 years ago
|
||
I implemented (a) as modifications to three of the patches: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/531ac7b57d35
Comment 62•14 years ago
|
||
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 63•14 years ago
|
||
Comment on attachment 451483 [details] [diff] [review] patch 13: add nsStyleCoord::Array r=bzbarsky
Attachment #451483 -
Flags: review?(bzbarsky) → review+
Comment 64•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #451486 -
Flags: review?(bzbarsky) → review+
Updated•14 years ago
|
Attachment #454182 -
Flags: review?(bzbarsky) → review+
Comment 65•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #454185 -
Flags: review?(bzbarsky) → review+
Updated•14 years ago
|
Attachment #454187 -
Flags: review?(bzbarsky) → review+
Updated•14 years ago
|
Attachment #454188 -
Flags: review?(bzbarsky) → review+
Comment 66•14 years ago
|
||
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 67•14 years ago
|
||
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•14 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•14 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.
Comment 70•14 years ago
|
||
... you just broke my patches that move AppendCSSValueToString, again, didn't you. ;-/
Comment 71•14 years ago
|
||
(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•14 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.
Comment 73•14 years ago
|
||
> (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•14 years ago
|
||
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•14 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•14 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.
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → beta5+
Assignee | ||
Comment 78•14 years ago
|
||
Fine with beta6, now that beta6 is feature freeze.
Assignee | ||
Comment 79•14 years ago
|
||
It turns out I missed outline-width and -moz-column-gap.
Attachment #472765 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 80•14 years ago
|
||
... and I also missed half the code for border-*-width
Attachment #472766 -
Flags: review?(bzbarsky)
Comment 81•14 years ago
|
||
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 82•14 years ago
|
||
Comment on attachment 472766 [details] [diff] [review] fix calc() on border-*-width r=me
Attachment #472766 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 83•14 years ago
|
||
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•14 years ago
|
||
... and remove a bunch of CalcOps classes as a result
Attachment #473665 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 85•14 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•14 years ago
|
||
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 87•14 years ago
|
||
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+
Comment 88•14 years ago
|
||
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•14 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.)
Comment 90•14 years ago
|
||
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 92•14 years ago
|
||
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•14 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
Closed: 14 years ago
Resolution: --- → FIXED
Comment 94•14 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•14 years ago
|
||
No point reducing tree depth when there's no tree.
Attachment #474621 -
Flags: review?(bzbarsky)
Comment 96•14 years ago
|
||
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•14 years ago
|
||
additional removal landed: http://hg.mozilla.org/mozilla-central/rev/e98ce9f4ddbb
Comment 98•14 years ago
|
||
Now documented: https://developer.mozilla.org/en/CSS/-moz-calc
Keywords: dev-doc-needed → dev-doc-complete
Blocks: css-values-3
You need to log in
before you can comment on or make changes to this bug.
Description
•