This bug is for implementing animation of CSS transforms in nsStyleAnimation (i.e., support CSS transitions of CSS 2D transforms). The relevant section of the transitions specification: http://dev.w3.org/csswg/css3-transitions/#animation-of-property-types- defers all of the details to the 2-D and 3-D transforms specifications. For the 2-D transforms specification (all that's relevant for us, right now), the relevant section is: http://dev.w3.org/csswg/css3-2d-transforms/#animation This, in turn, refers to the unmatrix program in Graphics Gems, available from http://tog.acm.org/resources/GraphicsGems/ , and in particular as the file GraphicsGems/gemsii/unmatrix.c in http://tog.acm.org/resources/GraphicsGems/AllGems.tar.gz The unmatrix reference is for general 3-D transform matrices (any of the 16 components can have any value). For CSS 2-D transforms, we have a 2-D matrix with the bottom row constant: [ A C E ] [ B D F ] [ 0 0 1 ] For that case, I believe the algorithm in unmatrix reduces to: (1) If A * D - B * C == 0, the matrix is singular. Fail. (2) Set translation components (Tx and Ty) to the translation parts of the matrix (E and F) and then ignore them for the rest of the time. (For us, E and F each actually consist of three constants: a length, a multiplier for the width, and a multiplier for the height. This actually requires its own decomposition, but I'll keep that separate.) (3) Let the X scale factor (Sx) be sqrt(A^2 + B^2). Then divide both A and B by it. (4) Let the XY shear (K) be A * C + B * D. From C, subtract A times the XY shear. From D, subtract B times the XY shear. (5) Let the Y scale (Sy) be sqrt(C^2 + D^2). Divide C, D, and the XY shear (K) by it. (6) At this point, A * D - B * C is either 1 or -1. If it is -1, negate all of A, B, C, D, the X scale (Sx), and the Y scale (Sy). (7) Let the rotation be R = atan2(B, A). Then the resulting decomposed transformation is: translate(Tx, Ty) rotate(R) skewX(atan(K)) scale(Sx, Sy) An interesting result of this is that all of the simple transform functions (i.e., all functions other than matrix()), in isolation, decompose back to themselves except for: 'skewY(φ)', which is 'matrix(1, tan(φ), 0, 1, 0, 0)', which decomposes to 'rotate(φ) skewX(φ) scale(sec(φ), cos(φ))' since (ignoring the alternate sign possibilities that would get fixed in step 6): In step 3, the X scale factor is sqrt(1+tan²(φ)) = sqrt(sec²(φ)) = sec(φ). Thus, after step 3, A = 1/sec(φ) = cos(φ) and B = tan(φ) / sec(φ) = sin(φ). In step 4, the XY shear is sin(φ). Thus, after step 4, C = -cos(φ)sin(φ) and D = 1 - sin²(φ) = cos²(φ). Thus, in step 5, the Y scale is sqrt(cos²(φ)(sin²(φ) + cos²(φ)) = cos(φ). Thus, after step 5, C = -sin(φ), D = cos(φ), and the XY shear is tan(φ). Thus, in step 6, A * D - B * C = cos²(φ) + sin²(φ) = 1. In step 7, the rotation is thus φ. skew(θ, φ), which is matrix(1, tan(φ), tan(θ), 1, 0, 0), which decomposes to 'rotate(φ) skewX(θ + φ) scale(sec(φ), cos(φ))' since (ignoring the alternate sign possibilities that would get fixed in step 6): In step 3, the X scale factor is sqrt(1+tan²(φ)) = sqrt(sec²(φ)) = sec(φ). Thus, after step 3, A = 1/sec(φ) = cos(φ) and B = tan(φ) / sec(φ) = sin(φ). In step 4, the XY shear is cos(φ)tan(θ) + sin(φ). Thus, after step 4, C = tan(θ) - cos(φ)(cos(φ)tan(θ) + sin(φ)) = tan(θ)sin²(φ) - cos(φ)sin(φ) D = 1 - sin(φ)(cos(φ)tan(θ) + sin(φ)) = cos²(φ) - sin(φ)cos(φ)tan(θ) Thus, in step 5, the Y scale is sqrt(C² + D²) = sqrt(tan²(θ)(sin⁴(φ) + sin²(φ)cos²(φ)) - 2 tan(θ)(sin³(φ)cos(φ) + sin(φ)cos³(φ)) + (sin²(φ)cos²(φ) + cos⁴(φ))) = sqrt(tan²(θ)sin²(φ) - 2 tan(θ)sin(φ)cos(φ) + cos²(φ)) = cos(φ) - tan(θ)sin(φ) (taking the negative of the obvious solution so we avoid flipping in step 6). After step 5, C = -sin(φ) and D = cos(φ), and the XY shear is (cos(φ)tan(θ) + sin(φ)) / (cos(φ) - tan(θ)sin(φ)) = (dividing both numerator and denominator by cos(φ)) (tan(θ) + tan(φ)) / (1 - tan(θ)tan(φ)) = tan(θ + φ). (See http://en.wikipedia.org/wiki/List_of_trigonometric_identities .) Thus, in step 6, A * D - B * C = cos²(φ) + sin²(φ) = 1. In step 7, the rotation is thus φ. To check this result, we can multiply things back together: [ cos(φ) -sin(φ) ] [ 1 tan(θ + φ) ] [ sec(φ) 0 ] [ sin(φ) cos(φ) ] [ 0 1 ] [ 0 cos(φ) ] [ cos(φ) cos(φ)tan(θ + φ) - sin(φ) ] [ sec(φ) 0 ] [ sin(φ) sin(φ)tan(θ + φ) + cos(φ) ] [ 0 cos(φ) ] but since tan(θ + φ) = (tan(θ) + tan(φ)) / (1 - tan(θ)tan(φ)), cos(φ)tan(θ + φ) - sin(φ) = cos(φ)(tan(θ) + tan(φ)) - sin(φ) + sin(φ)tan(θ)tan(φ) = cos(φ)tan(θ) + sin(φ) - sin(φ) + sin(φ)tan(θ)tan(φ) = cos(φ)tan(θ) + sin(φ)tan(θ)tan(φ) = tan(θ) (cos(φ) + sin(φ)tan(φ)) = tan(θ) sec(φ) (cos²(φ) + sin²(φ)) = tan(θ) sec(φ) and sin(φ)tan(θ + φ) + cos(φ) = sin(φ)(tan(θ) + tan(φ)) + cos(φ) - cos(φ)tan(θ)tan(φ) = tan(θ) (sin(φ) - sin(φ)) + sin(φ)tan(φ) + cos(φ) = sec(φ) (sin²(φ) + cos²(φ)) = sec(φ) so the above is: [ cos(φ) tan(θ) sec(φ) ] [ sec(φ) 0 ] [ sin(φ) sec(φ) ] [ 0 cos(φ) ] [ 1 tan(θ) ] [ tan(φ) 1 ] So the algorithms in the spec seem largely reasonable to me. I think it is valuable to avoid the matrix decomposition for transform lists that match, since it will produce better results in a number of cases, I think. One question I wonder, though, is whether animation of skew should animate over angle or over number (i.e., the animation function transitions between two tangents rather than between two angles); I tend to think it should actually be the latter.

*** Bug 554602 has been marked as a duplicate of this bug. ***

Created attachment 454191 [details] [diff] [review] patch 1: store the specified transform list in nsStyleDisplay This patch series applies on top of the patches in bug 363249 up through patch 22 (though it doesn't need patch 19 or 20 there).

Created attachment 454192 [details] [diff] [review] patch 2: add helper functions for some common nsCSSValue interpolations

Created attachment 454193 [details] [diff] [review] patch 3: add common function for setting pieces of transform matrix to a translation

Created attachment 454194 [details] [diff] [review] patch 4: allow CSS parser to forbid min()/max() within certain calc() expressions For transforms, I'm going to implement calc() without min() and max(); that makes it straightforward to convert to the matrix form that we use; without that restriction we'd have to do the full transform computation after we learned the width and height.

Created attachment 454195 [details] [diff] [review] patch 5: fix an odd function signature in nsStyleTransformMatrix

Created attachment 454197 [details] [diff] [review] patch 6: add support for calc() expressions in translate functions of -moz-transform

Created attachment 454198 [details] [diff] [review] patch 7: move ReadTransforms from nsRuleNode to nsStyleTransformMatrix

Created attachment 454199 [details] [diff] [review] patch 8 (main patch): implement animation of -moz-transform property

Comment on attachment 454195 [details] [diff] [review] patch 5: fix an odd function signature in nsStyleTransformMatrix Glancing at patch 5 first, since it looks smallest. :) >+static nscoord CalcLength(const nsCSSValue &aValue, >+ nsStyleContext* aContext, >+ nsPresContext* aPresContext, >+ PRBool &aCanStoreInRuleTree) > { >- aOut = nsRuleNode::CalcLength(aValue, aContext, aPresContext, >+ return nsRuleNode::CalcLength(aValue, aContext, aPresContext, > aCanStoreInRuleTree); > } Why do we need this method at all? Wouldn't it be simpler for the clients of this method to just call nsRuleNode::CalcLength directly?

We don't, really, though it saves an "nsRuleNode::". I should probably make it inline if we keep it, though.

Created attachment 454333 [details] [diff] [review] patch 8 (main patch): implement animation of -moz-transform property Here's a revised version of the main patch. The revision is to allow a small amount of floating point error to accumulate in two of the tests (one, which was previously commented out, is needed on my machine, and for the other test it's needed for Mac try). With that change, the patches in this bug and the calc() patches they depend on are green on try server.

Comment on attachment 454191 [details] [diff] [review] patch 1: store the specified transform list in nsStyleDisplay Notes for patch 1: > /* If we need to inherit, do so by making a full deep-copy. */ > else if (head->mValue.GetUnit() == eCSSUnit_Inherit) { >- display->mTransformPresent = parentDisplay->mTransformPresent; >- if (parentDisplay->mTransformPresent) >+ display->mSpecifiedTransform = parentDisplay->mSpecifiedTransform; >+ if (parentDisplay->mSpecifiedTransform) As we just spoke about, I think this comment needs updating (sine the code doesn't actually do a deep-copy). Otherwise, looks good; r=dholbert

Comment on attachment 454192 [details] [diff] [review] patch 2: add helper functions for some common nsCSSValue interpolations r=dholbert on patch 2

Comment on attachment 454193 [details] [diff] [review] patch 3: add common function for setting pieces of transform matrix to a translation r=dholbert on patch 3

Comment on attachment 454195 [details] [diff] [review] patch 5: fix an odd function signature in nsStyleTransformMatrix (In reply to comment #11) > We don't, really, though it saves an "nsRuleNode::". I should probably make it > inline if we keep it, though. r=dholbert if you make the CalcLength helper-function inline. You also should probably update the comment above it to mention something like "Convenience wrapper for RuleNode::CalcLength". (or if you prefer, this patch could just use direct calls to nsRuleNode::CalcLength everywhere -- r=dholbert either way)

(In reply to comment #16) > r=dholbert if you make the CalcLength helper-function inline. You also should > probably update the comment above it to mention something like "Convenience > wrapper for RuleNode::CalcLength". > > (or if you prefer, this patch could just use direct calls to > nsRuleNode::CalcLength everywhere -- r=dholbert either way) Actually, the reason it's not inline is that it becomes more complicated in the main patch in this bug. (I just explained that to dholbert verbally, and he was ok with it.)

Comment on attachment 454198 [details] [diff] [review] patch 7: move ReadTransforms from nsRuleNode to nsStyleTransformMatrix r=dholbert on patch 7

Comment on attachment 454333 [details] [diff] [review] patch 8 (main patch): implement animation of -moz-transform property >diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp >--- a/layout/style/nsStyleAnimation.cpp >+++ b/layout/style/nsStyleAnimation.cpp >+ double squareDistance = 0.0f; Nit: I don't think we need/want the "f" suffix there, since it's getting stored in a double. (applies to a few other places in this file, too) >+AppendTransformFunction(nsCSSKeyword aTransformFunction, Could you add an assertion to check that aTransformFunction is a valid transform function? (and not some arbitrary nsCSSKeyword) >+ nsRefPtr<nsCSSValue::Array> arr = nsCSSValue::Array::Create(nargs + 1); >+ if (!arr) { >+ return nsnull; >+ } No need to null-check 'arr' here -- nsCSSValue::Array::Create uses infallible-new under the hood. (or, if you'd prefer to keep the null-check to allow for ::Create to be fallible in the future, then there are a few not-null-checked ::Create calls elsewhere in this patch that would need checking.) >+ nsCSSValueList *item = new nsCSSValueList; >+ if (!item) { >+ return nsnull; >+ } No need to null-check 'item'. >+ * (3) Let the X scale factor (Sx) be sqrt(A^2 + B^2). Then divide both A >+ * and B by it. [...] >+ * (5) Let the Y scale (Sy) be sqrt(C^2 + D^2). Divide C, D, and the XY >+ * shear (K) by it. Nit: s/Y scale/Y scale factor/, for consistency with "X scale factor" in (3). (or, remove "factor" from (3)) >+ * (6) At this point, A * D - B * C is either 1 or -1. If it is -1, >+ * negate the XY shear (K) and the Y scale (Sy). [...] >+ if (A * D < B * C) { // A*D - B*C should be 1 or -1 >+ A = -A; >+ B = -B; >+ C = -C; >+ D = -D; >+ XYshear = -XYshear; >+ scaleX = -scaleX; >+ } The code here doesn't match the documentation (quoted above). In particular: - The documentation says to negate the *Y* scale, but the code negate the *X* scale instead. - The documentation doesn't mention negating A/B/C/D, and yet the code negates them (which I think has no effect in the case of C & D, since those variables aren't used after this block). Can you fix the code & documentation so they're in sync on the above points? Also, could you add an assertion or an NS_ABORT_IF_FALSE just above this block to verify that your "A*D - B*C should be 1 or -1" claim holds? (within some epsilon of float error) >+ arr = AppendTransformFunction(tfunc, resultTail); >+ if (!arr) { >+ return nsnull; >+ } Given the null-check-removals that I suggested above, I think AppendTransformFunction as a whole is infallible. So I don't think you need to null-check its return value here. (There are other places in the patch where you already don't null-check its return value.) >+AddTransformLists(const nsCSSValueList* aList1, double aCoeff1, [...] >+ nsCSSValue zero(0.0f, eCSSUnit_Pixel); [...] >+ nsCSSValue zero(0.0f, eCSSUnit_Radian); Looks like these |zero| values are never modified -- can we be declare them as |static const| so that we don't have to re-initialize them every time we traverse this code? >+ AddTransformTranslate(a1->Count() == 3 ? a1->Item(2) : zero, >+ aCoeff1, >+ a2->Count() == 3 ? a2->Item(2) : zero, >+ aCoeff2, >+ arr->Item(2)); >+ AddTransformTranslate(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2, >+ arr->Item(1)); The ordering here (handling 'y' before 'x') seems a bit counterintuitive -- seems to me like it would make more sense to handle 'x' before 'y'. Regardless of the ordering, though, could you add comments above each of the AddTransformTranslate calls to say: // add x component of translation and // add y component of translation (With the 1/2/3/zero/a1/a2/etc values and no mention of 'x' or 'y', it takes a bit of work to parse what's going on right now, at first glance.) Both of these points apply to the "scale" and "skew" cases in the same switch statement. >@@ -872,16 +1447,86 @@ nsStyleAnimation::AddWeighted(nsCSSPrope [...] >+ result = new nsCSSValueList; >+ if (result) { >+ result->mValue.SetNoneValue(); >+ } No need to null-check |result| here. >+ if (!result) { >+ return PR_FALSE; >+ } (This is at the end of the Transform clause inside of "AddWeighted") Do we actually need this check? That is, can |result| actually be null here? It looks to me like it gets set to something non-null in every single case. But I might be missing something. >+SubstitutePixelValues(nsStyleContext* aStyleContext, [...] >+ PRBool canStoreInRuleTree = PR_TRUE; >+ nscoord len = nsRuleNode::CalcLength(aInput, aStyleContext, >+ aStyleContext->PresContext(), >+ canStoreInRuleTree); I'd get rid of the "PR_TRUE" initialization there. |canStoreInRuleTree| is an outparam (which you're ignoring), so it's a bit misleading to initialize it to something. (Mostly for consistency -- elsewhere in this patch, you pass a boolean named "dummy" as an |aCanStoreInRuleTree| outparam, and you don't initialize |dummy| to anything.) >@@ -1666,16 +2342,51 @@ nsStyleAnimation::ExtractComputedValue(n >+ nsCSSValueList *clone = new nsCSSValueList; >+ if (!clone) { >+ result = nsnull; >+ break; >+ } No need to null-check |clone| here. >+ result = new nsCSSValueList(); >+ if (result) { >+ result->mValue.SetNoneValue(); No need to null-check |result| here. >+ if (!result) { >+ NS_WARNING("out of memory"); >+ return PR_FALSE; >+ } I don't think we could ever hit this OOM clause (right?), so it could probably be removed or converted into an NS_ABORT_IF_FALSE. >+++ b/layout/style/nsStyleTransformMatrix.cpp [...] > /* Helper function to fill in an nscoord with the specified nsCSSValue. */ > static nscoord CalcLength(const nsCSSValue &aValue, [...] >+ // fine) so that callers aare allowed to pass a null style context s/aare/are/ >+ /** >+ * Return the transform function, as an nsCSSKeyword, for the given >+ * nsCSSValue::Array from a transform list. >+ */ >+/* static */ nsCSSKeyword >+nsStyleTransformMatrix::TransformFunctionOf(const nsCSSValue::Array* aData) >+{ That header comment is indented 2 spaces here (probably from being copied from .h-->.cpp file) -- nix the indentation here. >+++ b/layout/style/nsStyleTransformMatrix.h >+ * aContext and aPresContext may be null if all of the length values in >+ * aData are already known to have been converted to eCSSUnit_Pixel (as >+ * they are in an nsStyleAnimation::Value) > */ > void SetToTransformFunction(const nsCSSValue::Array* aData, To avoid ambiguity, it's probably worth indicating in the comment above that percent values don't need to have been converted. Perhaps replace "length values" with "non-percent length values", or with "length values (not percents)"? You do have comments elsewhere about percent-values being OK, but those comments are both inside the .cpp file (in ExtractComputedValue and in your static CalcLength() function, to be specific), and it seems like this is something worth documenting in the header file. r=dholbert with the above fixed/addressed.

Comment on attachment 454194 [details] [diff] [review] patch 4: allow CSS parser to forbid min()/max() within certain calc() expressions r=bzbarsky

Comment on attachment 454197 [details] [diff] [review] patch 6: add support for calc() expressions in translate functions of -moz-transform r=bzbarsky

(In reply to comment #19) > >+AddTransformLists(const nsCSSValueList* aList1, double aCoeff1, > [...] > >+ nsCSSValue zero(0.0f, eCSSUnit_Pixel); > [...] > >+ nsCSSValue zero(0.0f, eCSSUnit_Radian); > > Looks like these |zero| values are never modified -- can we be declare them as > |static const| so that we don't have to re-initialize them every time we > traverse this code? I'd actually prefer not to prematurely optimize in this case, especially for an optimization that has an even odds chance of being slower rather than faster. > >+ AddTransformTranslate(a1->Count() == 3 ? a1->Item(2) : zero, > >+ aCoeff1, > >+ a2->Count() == 3 ? a2->Item(2) : zero, > >+ aCoeff2, > >+ arr->Item(2)); > >+ AddTransformTranslate(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2, > >+ arr->Item(1)); > > The ordering here (handling 'y' before 'x') seems a bit counterintuitive -- > seems to me like it would make more sense to handle 'x' before 'y'. I did it because the compiler is probably more likely to merge with the case below (which can be done non-DEBUG only) if the code is in that order, I think. > Regardless of the ordering, though, could you add comments above each of the > AddTransformTranslate calls to say: > // add x component of translation > and > // add y component of translation > (With the 1/2/3/zero/a1/a2/etc values and no mention of 'x' or 'y', it takes a > bit of work to parse what's going on right now, at first glance.) Yeah, I'll add comments, and mention the reason for the order. > I'd get rid of the "PR_TRUE" initialization there. |canStoreInRuleTree| is an > outparam (which you're ignoring), so it's a bit misleading to initialize it to > something. (Mostly for consistency -- elsewhere in this patch, you pass a > boolean named "dummy" as an |aCanStoreInRuleTree| outparam, and you don't > initialize |dummy| to anything.) It's not a true out-param; it's a param that gets false written to it but never gets true written to it. I'd sort of prefer to leave the initialization. I have everything else addressed locally.

Thanks, that sounds good. RE the pseudo-out-param -- maybe consider whether that logic applies to |dummy|, too (which currently isn't initialized to anything, and gets passed into nsStyleTransformMatrix::ReadTransforms). Maybe that one's more of a 'true' outparam, though, and that's the difference? Anyway, I trust your judgement on that, of course. :)

I think the difference is that I don't care what value a variable called |dummy| has, but if it has a sensible name it ought to have a sensible value, and the code is a little clearer when it has a sensible name.

http://hg.mozilla.org/mozilla-central/rev/c88af82d34cc http://hg.mozilla.org/mozilla-central/rev/fd83cf94ce08 http://hg.mozilla.org/mozilla-central/rev/b2fea2529876 http://hg.mozilla.org/mozilla-central/rev/4125a0ac5ec5 http://hg.mozilla.org/mozilla-central/rev/15e9cd490443 http://hg.mozilla.org/mozilla-central/rev/aa23d71ab230 http://hg.mozilla.org/mozilla-central/rev/e419e47c09bf http://hg.mozilla.org/mozilla-central/rev/f2b02ba56bdd That should put it in tomorrow's mozilla-central nightly (July 3), and also in Firefox 4.0 beta 2 (but not beta 1).

And if you notice it crashing, that's probably bug 576761; I'll try to get that fix in for tomorrow's nightly.

(In reply to comment #19) > >+ * (6) At this point, A * D - B * C is either 1 or -1. If it is -1, > >+ * negate the XY shear (K) and the Y scale (Sy). > [...] > >+ if (A * D < B * C) { // A*D - B*C should be 1 or -1 > >+ A = -A; > >+ B = -B; > >+ C = -C; > >+ D = -D; > >+ XYshear = -XYshear; > >+ scaleX = -scaleX; > >+ } > > The code here doesn't match the documentation (quoted above). In particular: > - The documentation says to negate the *Y* scale, but the code negate the *X* > scale instead. > - The documentation doesn't mention negating A/B/C/D, and yet the code negates > them (which I think has no effect in the case of C & D, since those variables > aren't used after this block). > > Can you fix the code & documentation so they're in sync on the above points? Looks like this point wasn't fixed in the final push [1] (the first half seeming more important, about Sx vs Sy) -- mind investigating & fixing whichever of the code vs. documentation is incorrect? [1] http://hg.mozilla.org/mozilla-central/rev/f2b02ba56bdd

The final push had: 2.247 + * (6) At this point, A * D - B * C is either 1 or -1. If it is -1, 2.248 + * negate the XY shear (K), the X scale (Sy), and A, B, C, and D. 2.249 + * (Alternatively, we could negate the XY shear (K) and the Y scale 2.250 + * (Sy).) In other words, we have a choice between: (1) negating K, Sx, A, B, (C, D) (2) negating K, Sy Since the spec isn't clear (due to the problems described in http://lists.w3.org/Archives/Public/www-style/2010Jun/0602.html ) I just picked one. The final push does have a single typo, though: "X scale (Sy)" instead of "X scale (Sx)".

Ah, sorry for overstating it -- I'd just glanced at the comment and saw the Sy, and then saw that the code part hadn't changed & still had scaleX. (but I missed that the rest of the comment had changed). So, yeah, just a s/Sy/Sx/ is all that's needed, looks like. Thanks!

Comment fixed: http://hg.mozilla.org/mozilla-central/rev/70967e44b56c