Bug 1717161 - Clamp perspective() values to a minimum of 1px. r=emilio

Differential Revision: https://phabricator.services.mozilla.com/D118250

layout/painting/nsDisplayList.cpp

@@ -7645,10 +7645,9 @@ bool nsDisplayTransform::ComputePerspectiveMatrix(const nsIFrame* aFrame,
   }
 
   MOZ_ASSERT(perspectiveDisplay->mChildPerspective.IsLength());
-  // TODO(emilio): Seems quite silly to go through app units just to convert to
-  // float pixels below.
-  nscoord perspective =
-      perspectiveDisplay->mChildPerspective.length._0.ToAppUnits();
+  float perspective =
+      perspectiveDisplay->mChildPerspective.length._0.ToCSSPixels();
+  perspective = std::max(1.0f, perspective);
   if (perspective < std::numeric_limits<Float>::epsilon()) {
     return true;
   }
@@ -7675,7 +7674,8 @@ bool nsDisplayTransform::ComputePerspectiveMatrix(const nsIFrame* aFrame,
   perspectiveOrigin += frameToPerspectiveGfxOffset;
 
   aOutMatrix._34 =
-      -1.0 / NSAppUnitsToFloatPixels(perspective, aAppUnitsPerPixel);
+      -1.0 / NSAppUnitsToFloatPixels(CSSPixel::ToAppUnits(perspective),
+                                     aAppUnitsPerPixel);
 
   aOutMatrix.ChangeBasis(Point3D(perspectiveOrigin.x, perspectiveOrigin.y, 0));
   return true;

layout/style/nsStyleTransformMatrix.h

@@ -39,9 +39,7 @@ enum class MatrixTransformOperator : uint8_t { Interpolate, Accumulate };
 // follows CSSWG's resolution on perspective(0). See bug 1316236.
 inline void ApplyPerspectiveToMatrix(mozilla::gfx::Matrix4x4& aMatrix,
                                      float aDepth) {
-  if (aDepth >= std::numeric_limits<float>::epsilon()) {
-    aMatrix.Perspective(aDepth);
-  }
+  aMatrix.Perspective(std::max(aDepth, 1.0f));
 }
 
 /**

servo/components/style/values/animated/transform.rs

@@ -1204,8 +1204,8 @@ impl ComputeSquaredDistance for ComputedTransformOperation {
                 // FIXME(emilio): Is this right? Why interpolating this with
                 // Perspective but not with anything else?
                 let mut p_matrix = Matrix3D::identity();
-                if p.px() > 0. {
-                    p_matrix.m34 = -1. / p.px();
+                if p.px() >= 0. {
+                    p_matrix.m34 = -1. / p.px().max(1.);
                 }
                 p_matrix.compute_squared_distance(&m)
             },

servo/components/style/values/generics/transform.rs

@@ -582,17 +582,10 @@ impl<T: ToMatrix> Transform<T> {
 /// Return the transform matrix from a perspective length.
 #[inline]
 pub fn create_perspective_matrix(d: CSSFloat) -> Transform3D<CSSFloat> {
-    // TODO(gw): The transforms spec says that perspective length must
-    // be positive. However, there is some confusion between the spec
-    // and browser implementations as to handling the case of 0 for the
-    // perspective value. Until the spec bug is resolved, at least ensure
-    // that a provided perspective value of <= 0.0 doesn't cause panics
-    // and behaves as it does in other browsers.
-    // See https://lists.w3.org/Archives/Public/www-style/2016Jan/0020.html for more details.
-    if d <= 0.0 {
+    if d < 0.0 {
         Transform3D::identity()
     } else {
-        Transform3D::perspective(d)
+        Transform3D::perspective(d.max(1.))
     }
 }
 

testing/web-platform/meta/css/css-transforms/perspective-zero-2.html.ini

@@ -1,2 +0,0 @@
-[perspective-zero-2.html]
-  expected: FAIL

testing/web-platform/meta/css/css-transforms/perspective-zero-3.html.ini

@@ -1,2 +0,0 @@
-[perspective-zero-3.html]
-  expected: FAIL

testing/web-platform/meta/css/css-transforms/perspective-zero.html.ini

@@ -1,2 +0,0 @@
-[perspective-zero.html]
-  expected: FAIL

testing/web-platform/meta/css/css-transforms/transform3d-perspective-005.html.ini

@@ -1,2 +0,0 @@
-[transform3d-perspective-005.html]
-  expected: FAIL

testing/web-platform/meta/web-animations/animation-model/animation-types/interpolation-per-property-002.html.ini

@@ -1,4 +0,0 @@
-[interpolation-per-property-002.html]
-  [transform: perspective]
-    expected: FAIL
-

testing/web-platform/tests/css/css-transforms/animation/transform-perspective-composition.html

@@ -13,7 +13,7 @@
 // Addition and accumulation of perspective values are very similar, but not
 // identical. We can test the difference by constructing a scenario where a
 // perspective parameter would go negative in one case (and thus be clamped
-// to 0), and would not go negative in the other case.
+// to 1), and would not go negative in the other case.
 //
 // In the test below, the values differ at 1.5 progress. The reason for this
 // is that at 1.5 progress, the addition case (which uses concatenation)
@@ -23,7 +23,7 @@
 //
 // Since perspective cannot go negative, this is clamped to:
 //
-//   perspective(10px) identity
+//   perspective(10px) perspective(1px) 
 //
 // The accumulation case, on the other hand, combines the components
 // and so ends up blending from perspective(5px) to perspective(8.33...px) at
@@ -44,7 +44,7 @@ test_composition({
   {at: 0.5, expect: 'perspective(6.15px)'},
   {at: 0.75, expect: 'perspective(7.06px)'},
   {at: 1, expect: 'perspective(8.33px)'},
-  {at: 1.5, expect: 'perspective(10px)'},
+  {at: 1.5, expect: 'matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, -1.1, 0, 0, 0, 1)'},
 ]);
 
 // ------------ Accumulation tests --------------