Animate skew in angle space rather than tangent space for interop and to avoid issues with infinite tangents. (Bug 606722) r=bzbarsky a2.0=blocking
authorL. David Baron <dbaron@dbaron.org>
Tue, 16 Nov 2010 09:56:03 -0800
changeset 57583 860d55aec420a96d0535d39c9a68b50099fb697c
parent 57582 0b5df68704ad59ace22682b40b1b30843af9e05d
child 57584 f0458767cf4b2d8f37eabb8feb9475fbccf43f18
push id16993
push userdbaron@mozilla.com
push dateTue, 16 Nov 2010 17:56:35 +0000
treeherdermozilla-central@860d55aec420 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs606722
milestone2.0b8pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Animate skew in angle space rather than tangent space for interop and to avoid issues with infinite tangents. (Bug 606722) r=bzbarsky a2.0=blocking
layout/style/nsStyleAnimation.cpp
layout/style/test/test_transitions_per_property.html
--- a/layout/style/nsStyleAnimation.cpp
+++ b/layout/style/nsStyleAnimation.cpp
@@ -618,25 +618,17 @@ nsStyleAnimation::ComputeDistance(nsCSSP
             CalcValue c1 = ExtractCalcValue(v1),
                       c2 = ExtractCalcValue(v2);
             double diff = c1.mLength - c2.mLength;
             squareDistance += diff * diff;
             diff = c1.mPercent - c2.mPercent;
             squareDistance += diff * diff;
           } else {
             NS_ABORT_IF_FALSE(v1.GetUnit() == v2.GetUnit(), "unit mismatch");
-            double diff;
-            if (tfunc == eCSSKeyword_skewx ||
-                tfunc == eCSSKeyword_skewy ||
-                tfunc == eCSSKeyword_skew) {
-              NS_ABORT_IF_FALSE(v1.GetUnit() == eCSSUnit_Radian, "unexpected unit");
-              diff = tan(v2.GetFloatValue()) - tan(v1.GetFloatValue());
-            } else {
-              diff = v2.GetFloatValue() - v1.GetFloatValue();
-            }
+            double diff = v2.GetFloatValue() - v1.GetFloatValue();
             squareDistance += diff * diff;
           }
         }
       }
       NS_ABORT_IF_FALSE(!list1 && !list2,
                         "list lengths should match after AddWeighted");
 
       aDistance = sqrt(squareDistance);
@@ -876,31 +868,16 @@ AddTransformScale(const nsCSSValue &aVal
   NS_ABORT_IF_FALSE(aValue2.GetUnit() == eCSSUnit_Number, "unexpected unit");
 
   float v1 = aValue1.GetFloatValue() - 1.0f,
         v2 = aValue2.GetFloatValue() - 1.0f;
   float result = v1 * aCoeff1 + v2 * aCoeff2;
   aResult.SetFloatValue(result + 1.0f, eCSSUnit_Number);
 }
 
-// FIXME: The spec still says skew should animate in angle space,
-// although I think we at least sort of agreed that it should animate
-// in tangent space.  So here I animate in in tangent space.
-// Animating in angle space would mean just using AddCSSValueAngle.
-static void
-AddTransformSkew(const nsCSSValue &aValue1, double aCoeff1,
-                 const nsCSSValue &aValue2, double aCoeff2,
-                 nsCSSValue &aResult)
-{
-  aResult.SetFloatValue(atan(aCoeff1 * tan(aValue1.GetAngleValueInRadians()) +
-                             aCoeff2 * tan(aValue2.GetAngleValueInRadians())),
-                        eCSSUnit_Radian);
-}
-
-
 static already_AddRefed<nsCSSValue::Array>
 AppendTransformFunction(nsCSSKeyword aTransformFunction,
                         nsCSSValueList**& aListTail)
 {
   PRUint32 nargs;
   if (aTransformFunction == eCSSKeyword_matrix) {
     nargs = 6;
   } else if (aTransformFunction == eCSSKeyword_translate ||
@@ -1151,20 +1128,17 @@ AddTransformMatrix(const nsStyleTransfor
 
   float rotate1, XYshear1, scaleX1, scaleY1;
   DecomposeMatrix(aMatrix1, rotate1, XYshear1, scaleX1, scaleY1);
   float rotate2, XYshear2, scaleX2, scaleY2;
   DecomposeMatrix(aMatrix2, rotate2, XYshear2, scaleX2, scaleY2);
 
   float rotate = rotate1 * aCoeff1 + rotate2 * aCoeff2;
 
-  // FIXME: The spec still says skew should animate in angle space,
-  // although I think we at least sort of agreed that it should animate
-  // in tangent space.  So here I animate in in tangent space.
-  float skewX = atanf(XYshear1 * aCoeff1 + XYshear2 * aCoeff2);
+  float skewX = atanf(XYshear1) * aCoeff1 + atanf(XYshear2) * aCoeff2;
 
   // Handle scale, and the two matrix components where identity is 1, by
   // subtracting 1, multiplying by the coefficients, and then adding 1
   // back.  This gets the right AddWeighted behavior and gets us the
   // interpolation-against-identity behavior for free.
   float scaleX =
     ((scaleX1 - 1.0f) * aCoeff1 + (scaleX2 - 1.0f) * aCoeff2) + 1.0f;
   float scaleY =
@@ -1275,47 +1249,44 @@ AddTransformLists(const nsCSSValueList* 
         NS_ABORT_IF_FALSE(a1->Count() == 2, "unexpected count");
         NS_ABORT_IF_FALSE(a2->Count() == 2, "unexpected count");
 
         AddTransformScale(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
                           arr->Item(1));
 
         break;
       }
+      // It would probably be nicer to animate skew in tangent space
+      // rather than angle space.  However, it's easy to specify
+      // skews with infinite tangents, and behavior changes pretty
+      // drastically when crossing such skews (since the direction of
+      // animation flips), so interop is probably more important here.
       case eCSSKeyword_skew: {
         NS_ABORT_IF_FALSE(a1->Count() == 2 || a1->Count() == 3,
                           "unexpected count");
         NS_ABORT_IF_FALSE(a2->Count() == 2 || a2->Count() == 3,
                           "unexpected count");
 
         nsCSSValue zero(0.0f, eCSSUnit_Radian);
         // Add Y component of skew.
-        AddTransformSkew(a1->Count() == 3 ? a1->Item(2) : zero,
+        AddCSSValueAngle(a1->Count() == 3 ? a1->Item(2) : zero,
                          aCoeff1,
                          a2->Count() == 3 ? a2->Item(2) : zero,
                          aCoeff2,
                          arr->Item(2));
 
         // Add X component of skew (which can be merged with case below
         // in non-DEBUG).
-        AddTransformSkew(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
+        AddCSSValueAngle(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
                          arr->Item(1));
 
         break;
       }
       case eCSSKeyword_skewx:
-      case eCSSKeyword_skewy: {
-        NS_ABORT_IF_FALSE(a1->Count() == 2, "unexpected count");
-        NS_ABORT_IF_FALSE(a2->Count() == 2, "unexpected count");
-
-        AddTransformSkew(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
-                         arr->Item(1));
-
-        break;
-      }
+      case eCSSKeyword_skewy:
       case eCSSKeyword_rotate: {
         NS_ABORT_IF_FALSE(a1->Count() == 2, "unexpected count");
         NS_ABORT_IF_FALSE(a2->Count() == 2, "unexpected count");
 
         AddCSSValueAngle(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
                          arr->Item(1));
 
         break;
--- a/layout/style/test/test_transitions_per_property.html
+++ b/layout/style/test/test_transitions_per_property.html
@@ -954,44 +954,45 @@ function test_transform_transition(prop)
       expected_uncomputed: 'scaleX(2.5)',
       expected: 'matrix(2.5, 0, 0, 1, 0px, 0px)' },
     { start: 'scaleY(5)', end: 'none',
       expected_uncomputed: 'scaleY(4)',
       expected: 'matrix(1, 0, 0, 4, 0px, 0px)' },
 
     // skew
     { start: 'skewX(45deg)', end: 'none',
-      expected: 'matrix(1, 0, 0.75, 1, 0px, 0px)' },
+      expected_uncomputed: 'skewX(33.75deg)' },
     { start: 'skewY(45deg)', end: 'none',
-      expected: 'matrix(1, 0.75, 0, 1, 0px, 0px)' },
+      expected_uncomputed: 'skewY(33.75deg)' },
     { start: 'skew(45deg)', end: 'none',
-      expected: 'matrix(1, 0, 0.75, 1, 0px, 0px)' },
+      expected_uncomputed: 'skew(33.75deg)' },
     { start: 'skew(45deg, 45deg)', end: 'none',
-      expected: 'matrix(1, 0.75, 0.75, 1, 0px, 0px)' },
+      expected_uncomputed: 'skew(33.75deg, 33.75deg)' },
     { start: 'skewX(45deg)', end: 'skewX(-45deg)',
-      expected: 'matrix(1, 0, 0.5, 1, 0px, 0px)' },
+      expected_uncomputed: 'skewX(22.5deg)' },
     { start: 'skewX(0)', end: 'skewX(-45deg)',
-      expected: 'matrix(1, 0, -0.25, 1, 0px, 0px)' },
+      expected_uncomputed: 'skewX(-11.25deg)' },
     { start: 'skewY(45deg)', end: 'skewY(-45deg)',
-      expected: 'matrix(1, 0.5, 0, 1, 0px, 0px)' },
+      expected_uncomputed: 'skewY(22.5deg)' },
 
     // matrix : skewX
     { start: 'matrix(1, 0, 3, 1, 0px, 0px)', end: 'none',
-      expected_uncomputed: 'matrix(1, 0, 2.25, 1, 0px, 0px)' },
+      force_compute: true,
+      expected_uncomputed: 'matrix(1, 0, ' + Math.tan(Math.atan(3) * 0.75) + ', 1, 0px, 0px)' },
     { start: 'skewX(0)', end: 'skewX(-45deg) translate(0)',
-      expected: 'matrix(1, 0, -0.25, 1, 0px, 0px)' },
+      expected_uncomputed: 'skewX(-11.25deg)' },
     // matrix : rotate
     { start: 'rotate(-30deg)', end: 'matrix(0, 1, -1, 0, 0px, 0px)',
       expected_uncomputed: 'matrix(1, 0, 0, 1, 0px, 0px)' },
     { start: 'rotate(-30deg) translateX(0)',
       end: 'translateX(0) rotate(-90deg)',
       expected: c('rotate(-45deg)') },
     // matrix decomposition of skewY
     { start: 'skewY(60deg)', end: 'skewY(-60deg) translateX(0)',
-      expected: c('rotate(30deg) skewX(' + Math.atan(Math.tan(Math.PI / 3) / 2) + 'rad) scale(2, 0.5)') },
+      expected: c('rotate(30deg) skewX(30deg) scale(2, 0.5)') },
 
     // matrix decomposition
 
     // Four pairs of the same matrix expressed different ways.
     { start: 'matrix(-1, 0, 0, -1, 0pt, 0pt)', /* rotate(180deg) */
       end: 'matrix(1, 0, 0, 1, 0, 0)',
       expected: c('rotate(135deg)') },
     { start: 'scale(-1)', end: 'none',
@@ -1036,74 +1037,72 @@ function test_transform_transition(prop)
     // the animations in
     // http://dbaron.org/css/test/2010/transition-negative-determinant
     // don't flip when they finish, and then wrote tests corresponding
     // to the current code's behavior.
     // ... start with four with positive determinants
     { start: 'none',
       end: 'matrix(1, 0, 1.5, 1, 0pt, 0pt)',
               /* skewX(atan(1.5)) */
-      expected: c('matrix(1, 0, 0.375, 1, 0, 0)') },
+      expected_uncomputed: 'matrix(1, 0, ' + Math.tan(Math.atan(1.5) * 0.25).toFixed(6) + ', 1, 0px, 0px)' },
     { start: 'none',
       end: 'matrix(-1, 0, 2, -1, 0pt, 0pt)',
               /* rotate(180deg) skewX(atan(-2)) */
-      expected: c('rotate(45deg) matrix(1, 0, -0.5, 1, 0, 0)') },
+      expected: c('rotate(45deg) matrix(1, 0, ' + Math.tan(Math.atan(-2) * 0.25) + ', 1, 0px, 0px)') },
     { start: 'none',
       end: 'matrix(0, -1, 1, -3, 0pt, 0pt)',
               /* rotate(-90deg) skewX(atan(3)) */
-      expected: c('rotate(-22.5deg) matrix(1, 0, 0.75, 1, 0, 0)') },
+      expected: c('rotate(-22.5deg) matrix(1, 0, ' + Math.tan(Math.atan(3) * 0.25) + ', 1, 0px, 0px)') },
     { start: 'none',
       end: 'matrix(0, 1, -1, 4, 0pt, 0pt)',
               /* rotate(90deg) skewX(atan(4)) */
-      expected: c('rotate(22.5deg) matrix(1, 0, 1, 1, 0, 0)') },
+      expected: c('rotate(22.5deg) matrix(1, 0, ' + Math.tan(Math.atan(4) * 0.25) + ', 1, 0px, 0px)') },
     // and then four with negative determinants
     { start: 'none',
       end: 'matrix(1, 0, 1, -1, 0pt, 0pt)',
               /* rotate(-180deg) skewX(atan(-1)) scaleX(-1) */
-      expected: c('rotate(-45deg) matrix(1, 0, -0.25, 1, 0, 0) scaleX(0.5)'),
-      round_error_ok: true },
+      expected: c('rotate(-45deg) matrix(1, 0, ' + Math.tan(Math.atan(-1) * 0.25) + ', 1, 0px, 0px) scaleX(0.5)') },
     { start: 'none',
       end: 'matrix(-1, 0, -1, 1, 0pt, 0pt)',
               /* skewX(atan(-1)) scaleX(-1) */
-      expected: c('matrix(1, 0, -0.25, 1, 0, 0) scaleX(0.5)') },
+      expected: c('matrix(1, 0, ' + Math.tan(Math.atan(-1) * 0.25) + ', 1, 0px, 0px) scaleX(0.5)') },
     { start: 'none',
       end: 'matrix(0, 1, 1, -2, 0pt, 0pt)',
               /* rotate(-90deg) skewX(atan(2)) scaleX(-1) */
-      expected: c('rotate(-22.5deg) matrix(1, 0, 0.5, 1, 0, 0) scaleX(0.5)') },
+      expected: c('rotate(-22.5deg) matrix(1, 0, ' + Math.tan(Math.atan(2) * 0.25) + ', 1, 0px, 0px) scaleX(0.5)') },
     { start: 'none',
       end: 'matrix(0, -1, -1, 0.5, 0pt, 0pt)',
               /* rotate(90deg) skewX(atan(0.5)) scaleX(-1) */
-      expected: c('rotate(22.5deg) matrix(1, 0, 0.125, 1, 0, 0) scaleX(0.5)') },
+      expected: c('rotate(22.5deg) matrix(1, 0, ' + Math.tan(Math.atan(0.5) * 0.25) + ', 1, 0px, 0px) scaleX(0.5)') },
 
     // lists vs. matrix decomposition
     { start: 'translate(10px) skewY(45deg)',
       end: 'translate(30px) skewY(-45deg)',
-      expected: 'matrix(1, 0.5, 0, 1, 15px, 0px)' },
+      expected_uncomputed: 'translate(15px) skewY(22.5deg)' },
     { start: 'skewY(45deg) rotate(90deg)',
       end: 'skewY(-45deg) rotate(90deg)',
-      expected: c('matrix(1, 0.5, 0, 1, 0px, 0px) rotate(90deg)') },
+      expected_uncomputed: 'skewY(22.5deg) rotate(90deg)' },
     { start: 'skewY(45deg) rotate(90deg) translate(0)',
       end: 'skewY(-45deg) rotate(90deg)',
-      expected: c('matrix(1, 0.5, 0, 1, 0px, 0px) rotate(90deg)') },
+      expected: c('rotate(90deg) skewX(-22.5deg)') },
     { start: 'skewX(45deg) rotate(90deg)',
       end: 'skewX(-45deg) rotate(90deg)',
-      expected: c('matrix(1, 0, 0.5, 1, 0px, 0px) rotate(90deg)') },
+      expected_uncomputed: 'skewX(22.5deg) rotate(90deg)' },
     { start: 'skewX(-60deg) rotate(90deg) translate(0)',
       end: 'skewX(60deg) rotate(90deg)',
-      expected: c('rotate(120deg) skewX(' +
-                   Math.atan(Math.tan(Math.PI / 3) / 2) +
-                  'rad) scale(2, 0.5)') },
+      expected: c('rotate(120deg) skewX(30deg) scale(2, 0.5)'),
+      round_error_ok: true },
   ];
 
   var matrix_re = /^matrix\(([^,]*), ([^,]*), ([^,]*), ([^,]*), ([^,]*)px, ([^,]*)px\)$/;
   for (var i in tests) {
     var test = tests[i];
     if (!("expected" in test)) {
       var v = test.expected_uncomputed;
-      if (v.match(matrix_re)) {
+      if (v.match(matrix_re) && !test.force_compute) {
         test.expected = v;
       } else {
         test.expected = c(v);
       }
     }
   }
 
   for (var i in tests) {