Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()). r?xidorn draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Sun, 23 Apr 2017 16:11:10 -0700
changeset 566827 9c7184db1cb990016f9c39da7c0736a152216385
parent 566810 06c7c56497ad4e34729d6e54da8408ad868ec8de
child 625437 67dad16ecbf9da0e8204c5832c5d297f04cf7978
push id55349
push userdholbert@mozilla.com
push dateSun, 23 Apr 2017 23:11:20 +0000
reviewersxidorn
bugs1241623
milestone55.0a1
Bug 1241623: Represent legacy -webkit-gradient(linear,...) expressions as an approximately-equivalent linear-gradient() (instead of -moz-linear-gradient()). r?xidorn Note: Instead of exactly honoring the legacy -webkit-gradient(linear,...) syntax, we aim to simply parse it into something that's approximately equivalent for common use cases. In the legacy -webkit-gradient(linear,...) syntax, authors provide *two arbitrary points* to establish the direction of the gradient, whereas modern linear-gradient() is simpler: it just takes a single "<side-or-corner>" point and fills the box with a gradient in the direction of that side or corner. Before this changeset, we parsed -webkit-gradient(linear,...) into a slightly-less-legacy "-moz-linear-gradient" representation, so that we could honor at least one of the author's specified points (since -moz-linear-gradient accepts a single arbitrary point as the start of the gradient). But that prevents us from deprecating -moz-linear-gradient, and it makes it impossible to spec our emulation behavior in terms of modern standards. So, this changeset is just changing our approximate representation so that it can always be serializeable as a modern linear-gradient() expression. In addition, I'm removing the reftest "webkit-gradient-approx-linear-1.html" (whose behavior this patch is changing slightly) and I'm replacing it with a new chunk in test_computed_style.html to more directly test this parsing behaivor. MozReview-Commit-ID: 6N1oKaGeOuE
layout/reftests/webkit-gradient/reftest-stylo.list
layout/reftests/webkit-gradient/reftest.list
layout/reftests/webkit-gradient/webkit-gradient-approx-linear-1-ref.html
layout/reftests/webkit-gradient/webkit-gradient-approx-linear-1.html
layout/style/nsCSSParser.cpp
layout/style/test/test_computed_style.html
--- a/layout/reftests/webkit-gradient/reftest-stylo.list
+++ b/layout/reftests/webkit-gradient/reftest-stylo.list
@@ -1,17 +1,16 @@
 # DO NOT EDIT! This is a auto-generated temporary list for Stylo testing
 # This directory contains tests for -webkit-gradient() expressions.
 # They require webkit prefix support to be enabled.
 default-preferences pref(layout.css.prefixes.webkit,true)
 
-# Tests where we don't render a "-webkit-gradient" exactly correctly.
-# (These just ensure that our approximate/do-something rendering does not
+# In this test, we don't render a "-webkit-gradient" exactly correctly.
+# (It's just here to ensure that our approximate/do-something rendering doesn't
 # change unexpectedly.)
-fails == webkit-gradient-approx-linear-1.html webkit-gradient-approx-linear-1.html
 fails == webkit-gradient-approx-radial-1.html webkit-gradient-approx-radial-1.html
 
 # Tests for -webkit-gradient(linear, ...)
 fails == webkit-gradient-linear-1a.html webkit-gradient-linear-1a.html
 fails == webkit-gradient-linear-1b.html webkit-gradient-linear-1b.html
 fails == webkit-gradient-linear-1c.html webkit-gradient-linear-1c.html
 fails == webkit-gradient-linear-1d.html webkit-gradient-linear-1d.html
 fails == webkit-gradient-linear-2.html webkit-gradient-linear-2.html
--- a/layout/reftests/webkit-gradient/reftest.list
+++ b/layout/reftests/webkit-gradient/reftest.list
@@ -1,16 +1,15 @@
 # This directory contains tests for -webkit-gradient() expressions.
 # They require webkit prefix support to be enabled.
 default-preferences pref(layout.css.prefixes.webkit,true)
 
-# Tests where we don't render a "-webkit-gradient" exactly correctly.
-# (These just ensure that our approximate/do-something rendering does not
+# In this test, we don't render a "-webkit-gradient" exactly correctly.
+# (It's just here to ensure that our approximate/do-something rendering doesn't
 # change unexpectedly.)
-fuzzy-if(cocoaWidget,3,3369) == webkit-gradient-approx-linear-1.html webkit-gradient-approx-linear-1-ref.html # bug 1225372
 == webkit-gradient-approx-radial-1.html webkit-gradient-approx-radial-1-ref.html
 
 # Tests for -webkit-gradient(linear, ...)
 == webkit-gradient-linear-1a.html webkit-gradient-linear-1-ref.html
 == webkit-gradient-linear-1b.html webkit-gradient-linear-1-ref.html
 == webkit-gradient-linear-1c.html webkit-gradient-linear-1-ref.html
 == webkit-gradient-linear-1d.html webkit-gradient-linear-1-ref.html
 == webkit-gradient-linear-2.html webkit-gradient-linear-2-ref.html
deleted file mode 100644
--- a/layout/reftests/webkit-gradient/webkit-gradient-approx-linear-1-ref.html
+++ /dev/null
@@ -1,68 +0,0 @@
-<!DOCTYPE html>
-<!--
-     Any copyright is dedicated to the Public Domain.
-     http://creativecommons.org/publicdomain/zero/1.0/
--->
-<html>
-<head>
-  <title>CSS Reference</title>
-  <style>
-    div {
-      border: 1px solid black;
-      width:  50px;
-      height: 30px;
-      margin: 1px;
-      float: left;
-    }
-    br { clear: both; }
-  </style>
-</head>
-<body>
-  <!-- Start point and end point are the same, in testcase:
-       We end up detecting that the start & end have the same "y" value,
-       so we render a gradient going from left to right horizontally. -->
-  <div style="background: linear-gradient(to right,
-                                          blue, yellow)"></div>
-  <div style="background: linear-gradient(to right,
-                                          blue, yellow)"></div>
-  <div style="background: linear-gradient(to right,
-                                          blue, yellow)"></div>
-  <br>
-
-  <!-- For the rest of the gradients here, we end up rendering a "legacy"
-       gradient (-moz-linear-gradient) with the gradient line starting at the
-       testcase's specified "start" point, and we ignore the "end" point. -->
-  <div style="background: -moz-linear-gradient(center center,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(left top,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(10px 15px,
-                                               blue, yellow)"></div>
-  <br>
-
-  <div style="background: -moz-linear-gradient(-10px -15px,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(-100px 10px,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(10px -100px,
-                                               blue, yellow)"></div>
-  <br>
-
-  <div style="background: -moz-linear-gradient(center top,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(left center,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(right center,
-                                               blue, yellow)"></div>
-  <br>
-
-  <div style="background: -moz-linear-gradient(right top,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(left top,
-                                               blue, yellow)"></div>
-  <div style="background: -moz-linear-gradient(left bottom,
-                                               blue, yellow)"></div>
-  <br>
-
-</body>
-</html>
deleted file mode 100644
--- a/layout/reftests/webkit-gradient/webkit-gradient-approx-linear-1.html
+++ /dev/null
@@ -1,73 +0,0 @@
-<!DOCTYPE html>
-<!--
-     Any copyright is dedicated to the Public Domain.
-     http://creativecommons.org/publicdomain/zero/1.0/
--->
-<html>
-<head>
-  <title>
-    CSS Test: -webkit-gradient(linear, ...) expressions which we don't render
-    quite correctly because they can't easily be represented with modern syntax.
-  </title>
-  <style>
-    div {
-      border: 1px solid black;
-      width:  50px;
-      height: 30px;
-      margin: 1px;
-      float: left;
-
-      /* We include a fallback background, to easily distinguish failures at
-       * parse time (which fall back to this value) vs. something later on. */
-      background: red;
-    }
-    br { clear: both; }
-  </style>
-</head>
-<body>
-  <!-- Start point and end point are the same: -->
-  <div style="background: -webkit-gradient(linear, left top, left top,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, 40 40, 40 40,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, center center, center center,
-                                           from(blue), to(yellow))"></div>
-  <br>
-
-  <!-- Gradient starting and/or ending somewhere in the middle (not an edge): -->
-  <div style="background: -webkit-gradient(linear, center center, right top,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, left top, center center,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, 10 15, 5 20,
-                                           from(blue), to(yellow))"></div>
-  <br>
-
-  <!-- Gradient starting and/or ending somewhere arbitrary outside: -->
-  <div style="background: -webkit-gradient(linear, -10 -15, left top,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, -100 10, 20 30,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, 10 -100, 100 10,
-                                           from(blue), to(yellow))"></div>
-  <br>
-
-  <!-- Slanted gradient between sides/corners: -->
-  <div style="background: -webkit-gradient(linear, center top, left center,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, left center, center top,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, right center, center top,
-                                           from(blue), to(yellow))"></div>
-  <br>
-
-  <div style="background: -webkit-gradient(linear, right top, center bottom,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, left top, right bottom,
-                                           from(blue), to(yellow))"></div>
-  <div style="background: -webkit-gradient(linear, left bottom, right top,
-                                           from(blue), to(yellow))"></div>
-  <br>
-
-</body>
-</html>
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -10795,75 +10795,81 @@ IsWebkitGradientCoordLarger(const nsCSSV
 }
 
 // Finalize our internal representation of a -webkit-gradient(linear, ...)
 // expression, given the parsed points.  (The parsed color stops
 // should already be hanging off of the passed-in nsCSSValueGradient.)
 //
 // Note: linear gradients progress along a line between two points.  The
 // -webkit-gradient(linear, ...) syntax lets the author precisely specify the
-// starting and ending point. However, our internal gradient structures
-// only store one point, and the other point is implicitly its reflection
-// across the painted area's center. (The legacy -moz-linear-gradient syntax
-// also lets us store an angle.)
+// starting and ending point. However, our internal gradient structures only
+// store ONE point (which for modern linear-gradient() expressions is a side or
+// corner & represents the ending point of the gradient -- and the starting
+// point is implicitly the opposite side/corner).
 //
-// In this function, we try to go from the two-point representation to an
-// equivalent or approximately-equivalent one-point representation.
+// In this function, we analyze the start & end points from a
+// -webkit-gradient(linear, ...) expression, and we choose an appropriate
+// target point to produce a modern linear-gradient() representation that has
+// the same rough trajectory.  If we can't determine a target point for some
+// reason, we just fall back to the default top-to-bottom linear-gradient()
+// directionality.
 void
 CSSParserImpl::FinalizeLinearWebkitGradient(nsCSSValueGradient* aGradient,
                                             const nsCSSValuePair& aStartPoint,
                                             const nsCSSValuePair& aEndPoint)
 {
   MOZ_ASSERT(!aGradient->mIsRadial, "passed-in gradient must be linear");
 
-  // If the start & end points have the same Y-coordinate, then we can treat
-  // this as a horizontal gradient progressing towards the center of the left
-  // or right side.
-  if (aStartPoint.mYValue == aEndPoint.mYValue) {
-    aGradient->mBgPos.mYValue.SetIntValue(NS_STYLE_IMAGELAYER_POSITION_CENTER,
+  if (aStartPoint == aEndPoint ||
+      aStartPoint.mXValue.GetUnit() != aEndPoint.mXValue.GetUnit() ||
+      aStartPoint.mYValue.GetUnit() != aEndPoint.mYValue.GetUnit()) {
+    // Start point & end point are the same, OR they use different units for at
+    // least one coordinate.  Either way, this isn't something we can represent
+    // using an unprefixed linear-gradient expression, so instead we'll just
+    // arbitrarily pretend this is a top-to-bottom gradient (which is the
+    // default for modern linear-gradient if a direction is unspecified).
+    aGradient->mBgPos.mYValue.SetIntValue(NS_STYLE_IMAGELAYER_POSITION_BOTTOM,
                                           eCSSUnit_Enumerated);
-    if (IsWebkitGradientCoordLarger(aStartPoint.mXValue, aEndPoint.mXValue)) {
-      aGradient->mBgPos.mXValue.SetIntValue(NS_STYLE_IMAGELAYER_POSITION_LEFT,
-                                            eCSSUnit_Enumerated);
-    } else {
-      aGradient->mBgPos.mXValue.SetIntValue(NS_STYLE_IMAGELAYER_POSITION_RIGHT,
-                                            eCSSUnit_Enumerated);
-    }
-    return;
-  }
-
-  // If the start & end points have the same X-coordinate, then we can treat
-  // this as a horizontal gradient progressing towards the center of the top
-  // or bottom side.
-  if (aStartPoint.mXValue == aEndPoint.mXValue) {
     aGradient->mBgPos.mXValue.SetIntValue(NS_STYLE_IMAGELAYER_POSITION_CENTER,
                                           eCSSUnit_Enumerated);
-    if (IsWebkitGradientCoordLarger(aStartPoint.mYValue, aEndPoint.mYValue)) {
-      aGradient->mBgPos.mYValue.SetIntValue(NS_STYLE_IMAGELAYER_POSITION_TOP,
-                                            eCSSUnit_Enumerated);
-    } else {
-      aGradient->mBgPos.mYValue.SetIntValue(NS_STYLE_IMAGELAYER_POSITION_BOTTOM,
-                                            eCSSUnit_Enumerated);
-    }
     return;
   }
 
-  // OK, the gradient is angled, which means we likely can't represent it
-  // exactly in |aGradient|, without doing analysis on the two points to
-  // extract an angle (which we might not be able to do depending on the units
-  // used).  For now, we'll just do something really basic -- just use the
-  // first point as if it were the starting point in a legacy
-  // -moz-linear-gradient() expression. That way, the rendered gradient will
-  // progress from this first point, towards the center of the covered element,
-  // to a reflected end point on the far side. Note that we have to use
-  // mIsLegacySyntax=true for this to work, because standardized (non-legacy)
-  // gradients place some restrictions on the reference point [namely, that it
-  // use percent units & be on the border of the element].
-  aGradient->mIsLegacySyntax = true;
-  aGradient->mBgPos = aStartPoint;
+  // Set the target point to one of the box's corners (or the center of an
+  // edge), by comparing aStartPoint and aEndPoint to extract a general
+  // direction.
+
+  int32_t targetX;
+  if (aStartPoint.mXValue == aEndPoint.mXValue) {
+    targetX = NS_STYLE_IMAGELAYER_POSITION_CENTER;
+  } else if (IsWebkitGradientCoordLarger(aStartPoint.mXValue,
+                                         aEndPoint.mXValue)) {
+    targetX = NS_STYLE_IMAGELAYER_POSITION_LEFT;
+  } else {
+    MOZ_ASSERT(IsWebkitGradientCoordLarger(aEndPoint.mXValue,
+                                           aStartPoint.mXValue),
+               "IsWebkitGradientCoordLarger returning inconsistent results?");
+    targetX = NS_STYLE_IMAGELAYER_POSITION_RIGHT;
+  }
+
+  int32_t targetY;
+  if (aStartPoint.mYValue == aEndPoint.mYValue) {
+    targetY = NS_STYLE_IMAGELAYER_POSITION_CENTER;
+  } else if (IsWebkitGradientCoordLarger(aStartPoint.mYValue,
+                                         aEndPoint.mYValue)) {
+    targetY = NS_STYLE_IMAGELAYER_POSITION_TOP;
+  } else {
+    MOZ_ASSERT(IsWebkitGradientCoordLarger(aEndPoint.mYValue,
+                                           aStartPoint.mYValue),
+               "IsWebkitGradientCoordLarger returning inconsistent results?");
+    targetY = NS_STYLE_IMAGELAYER_POSITION_BOTTOM;
+  }
+
+  aGradient->mBgPos.mXValue.SetIntValue(targetX, eCSSUnit_Enumerated);
+  aGradient->mBgPos.mYValue.SetIntValue(targetY, eCSSUnit_Enumerated);
 }
 
 // Finalize our internal representation of a -webkit-gradient(radial, ...)
 // expression, given the parsed points & radii.  (The parsed color-stops
 // should already be hanging off of the passed-in nsCSSValueGradient).
 void
 CSSParserImpl::FinalizeRadialWebkitGradient(nsCSSValueGradient* aGradient,
                                             const nsCSSValuePair& aFirstCenter,
--- a/layout/style/test/test_computed_style.html
+++ b/layout/style/test/test_computed_style.html
@@ -337,16 +337,132 @@ var noframe_container = document.getElem
     for (var i = 0; i < subProp.length; i++) {
       p.style.mask = subProp[i];
       isnot(cs.mask, "", "computed value of " + subProp[i] + " mask");
     }
   }
   p.remove();
 })();
 
+(function test_bug_1241623() {
+  // Test that -webkit-gradient() styles are approximated the way we expect:
+
+  // For compactness, we'll pull out the common prefix & suffix from all of the
+  // specified & expected styles, and construct the full expression on the fly:
+  const specPrefix = "-webkit-gradient(linear, ";
+  const specSuffix = ", from(blue), to(lime))";
+
+  const expPrefix = "linear-gradient(";
+  const expSuffix = "rgb(0, 0, 255) 0%, rgb(0, 255, 0) 100%)";
+
+  let testcases = [
+    //
+    // [ legacyDirection,
+    //   modernDirection, // (empty string means use default direction)
+    //   descriptionOfTestcase ],
+
+    // If start & end are at same point, we just produce a gradient with
+    // the default direction.
+    [ "left top, left top",
+      "",
+      "start & end point are the same" ],
+    [ "40 40, 40 40",
+      "",
+      "start & end point are the same" ],
+    [ "center center, center center",
+      "",
+      "start & end point are the same" ],
+
+    // If start & end use different units in the same coordinate, we generally
+    // can't extract a direction (because we can't know whether arbitrary
+    // percent values are larger or smaller than arbitrary pixel values). So
+    // we produce a gradient in the default direction.
+    [ "left top, 30 100%",  // (Note: keywords like "left" are really % vals.)
+      "",
+      "start & end point have different units" ],
+    [ "100% 15, right bottom",
+      "",
+      "start & end point have different units" ],
+    [ "0 0%, 20 20",
+      "",
+      "start & end point have different units" ],
+    [ "0 0, 100% 20",
+      "",
+      "start & end point have different units" ],
+    [ "5% 30, 20 50%",
+      "",
+      "start & end point have different units" ],
+    [ "5% 6%, 20 30",
+      "",
+      "start & end point have different units" ],
+
+    // Gradient starting/ending somewhere arbitrary in middle:
+    [ "center center, right top",
+      "to top right",
+      "from center to top right" ],
+    [ "left top, center center",
+      "to bottom right",
+      "from left top to center" ],
+    [ "10 15, 5 20",
+      "to bottom left",
+      "from arbitrary point to another point in lower-left direction" ],
+
+    // Gradient using negative coordinates:
+    [ "-10 -15, 0 0",
+      "to bottom right",
+      "from negative point to origin" ],
+    [ "-100 10, 20 30",
+      "to bottom right",
+      "from negative-x point to another point in lower-right direction" ],
+    [ "10 -100, 5 10",
+      "to bottom left",
+      "from negative-y point to another point in lower-left direction" ],
+
+    // Diagonal gradient between sides/corners:
+    [ "center top, left center",
+      "to bottom left",
+      "left/bottom-wards, using edge keywords" ],
+    [ "left center, center top",
+      "to top right",
+      "top/right-wards, using edge keywords" ],
+    [ "right center, center top",
+      "to top left",
+      "top/left-wards, using edge keywords" ],
+    [ "right top, center bottom",
+      "to bottom left",
+      "bottom/left-wards, using edge keywords" ],
+    [ "left top, right bottom",
+      "to bottom right",
+      "bottom/right-wards, using edge keywords" ],
+    [ "left bottom, right top",
+      "to top right",
+      "top/right-wards, using edge keywords" ],
+  ];
+
+  let p = document.createElement("p");
+  let cs = getComputedStyle(p, "");
+  frame_container.appendChild(p);
+
+  for (let test of testcases) {
+    let specifiedStyle = specPrefix + test[0] + specSuffix;
+    let expectedStyle = expPrefix;
+    if (test[1] != "") {
+      expectedStyle += test[1] + ", ";
+    }
+    expectedStyle += expSuffix;
+
+    p.style.backgroundImage = specifiedStyle;
+    is(cs.backgroundImage, expectedStyle,
+       "computed value of -webkit-gradient expression (" + test[2] + ")");
+    p.style.backgroundImage = "";
+  }
+
+  p.remove();
+})();
+
 (function test_bug_1293164() {
 
   var p = document.createElement("p");
   var cs = getComputedStyle(p, "");
   frame_container.appendChild(p);
 
   var docPath = document.URL.substring(0, document.URL.lastIndexOf("/") + 1);