Bug 1339394 - Don't serialize transparent color to transparent keyword when not necessary. r=heycam
☠☠ backed out by 036b32fc9bc4 ☠ ☠
authorXidorn Quan <me@upsuper.org>
Tue, 14 Feb 2017 22:43:32 +1100
changeset 389521 ae26b4e4d59b93c33bf06701d5bf9d4a97a2b1f8
parent 389520 8d030984b6a9dcabdd8ec4ea83ee1337c194dd45
child 389522 036b32fc9bc49cb1ac912ca450685cda787a7a35
push id7198
push userjlorenzo@mozilla.com
push dateTue, 18 Apr 2017 12:07:49 +0000
treeherdermozilla-beta@d57aa49c3948 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1339394
milestone54.0a1
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
Bug 1339394 - Don't serialize transparent color to transparent keyword when not necessary. r=heycam MozReview-Commit-ID: 59cmaCoFJMR
layout/style/nsCSSValue.cpp
layout/style/nsComputedDOMStyle.cpp
layout/style/test/chrome/hover_helper.html
layout/style/test/property_database.js
layout/style/test/test_additional_sheets.html
layout/style/test/test_bug229915.html
layout/style/test/test_bug372770.html
layout/style/test/test_bug635286.html
layout/style/test/test_computed_style.html
layout/style/test/test_default_computed_style.html
--- a/layout/style/nsCSSValue.cpp
+++ b/layout/style/nsCSSValue.cpp
@@ -1655,48 +1655,41 @@ nsCSSValue::AppendToString(nsCSSProperty
       MOZ_ASSERT(false, "bad color value");
     }
   }
   else if (IsNumericColorUnit(unit)) {
     if (aSerialization == eNormalized ||
         unit == eCSSUnit_RGBColor ||
         unit == eCSSUnit_RGBAColor) {
       nscolor color = GetColorValue();
-      if (aSerialization == eNormalized &&
-          color == NS_RGBA(0, 0, 0, 0)) {
-        // Use the strictest match for 'transparent' so we do correct
-        // round-tripping of all other rgba() values.
-        aResult.AppendLiteral("transparent");
+      // For brevity, we omit the alpha component if it's equal to 255 (full
+      // opaque). Also, we try to preserve the author-specified function name,
+      // unless it's rgba() and we're omitting the alpha component - then we
+      // use rgb().
+      uint8_t a = NS_GET_A(color);
+      bool showAlpha = (a != 255);
+
+      if (unit == eCSSUnit_RGBAColor && showAlpha) {
+        aResult.AppendLiteral("rgba(");
       } else {
-        // For brevity, we omit the alpha component if it's equal to 255 (full
-        // opaque). Also, we try to preserve the author-specified function name,
-        // unless it's rgba() and we're omitting the alpha component - then we
-        // use rgb().
-        uint8_t a = NS_GET_A(color);
-        bool showAlpha = (a != 255);
-
-        if (unit == eCSSUnit_RGBAColor && showAlpha) {
-          aResult.AppendLiteral("rgba(");
-        } else {
-          aResult.AppendLiteral("rgb(");
-        }
-
-        NS_NAMED_LITERAL_STRING(comma, ", ");
-
-        aResult.AppendInt(NS_GET_R(color), 10);
+        aResult.AppendLiteral("rgb(");
+      }
+
+      NS_NAMED_LITERAL_STRING(comma, ", ");
+
+      aResult.AppendInt(NS_GET_R(color), 10);
+      aResult.Append(comma);
+      aResult.AppendInt(NS_GET_G(color), 10);
+      aResult.Append(comma);
+      aResult.AppendInt(NS_GET_B(color), 10);
+      if (showAlpha) {
         aResult.Append(comma);
-        aResult.AppendInt(NS_GET_G(color), 10);
-        aResult.Append(comma);
-        aResult.AppendInt(NS_GET_B(color), 10);
-        if (showAlpha) {
-          aResult.Append(comma);
-          aResult.AppendFloat(nsStyleUtil::ColorComponentToFloat(a));
-        }
-        aResult.Append(char16_t(')'));
+        aResult.AppendFloat(nsStyleUtil::ColorComponentToFloat(a));
       }
+      aResult.Append(char16_t(')'));
     } else if (eCSSUnit_HexColor == unit ||
                eCSSUnit_HexColorAlpha == unit) {
       nscolor color = GetColorValue();
       aResult.Append('#');
       aResult.AppendPrintf("%02x", NS_GET_R(color));
       aResult.AppendPrintf("%02x", NS_GET_G(color));
       aResult.AppendPrintf("%02x", NS_GET_B(color));
       if (eCSSUnit_HexColorAlpha == unit) {
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -1110,21 +1110,16 @@ nsComputedDOMStyle::DoGetStackSizing()
                 eCSSKeyword_ignore);
   return val.forget();
 }
 
 void
 nsComputedDOMStyle::SetToRGBAColor(nsROCSSPrimitiveValue* aValue,
                                    nscolor aColor)
 {
-  if (NS_GET_A(aColor) == 0) {
-    aValue->SetIdent(eCSSKeyword_transparent);
-    return;
-  }
-
   nsROCSSPrimitiveValue *red   = new nsROCSSPrimitiveValue;
   nsROCSSPrimitiveValue *green = new nsROCSSPrimitiveValue;
   nsROCSSPrimitiveValue *blue  = new nsROCSSPrimitiveValue;
   nsROCSSPrimitiveValue *alpha  = new nsROCSSPrimitiveValue;
 
   uint8_t a = NS_GET_A(aColor);
   nsDOMCSSRGBColor *rgbColor =
     new nsDOMCSSRGBColor(red, green, blue, alpha, a < 255);
--- a/layout/style/test/chrome/hover_helper.html
+++ b/layout/style/test/chrome/hover_helper.html
@@ -60,27 +60,27 @@ function setResize(str) {
 }
 
 function step1() {
     /** test basic hover **/
     var divone = document.getElementById("one");
     synthesizeMouse(divone, 5, 7, moveEvent, window);
     is(getComputedStyle(divone, "").backgroundColor, "rgb(0, 0, 255)",
        ":hover applies");
-    is(getComputedStyle(divone.firstChild, "").backgroundColor, "transparent",
+    is(getComputedStyle(divone.firstChild, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     synthesizeMouse(divone, 5, 2, moveEvent, window);
     is(getComputedStyle(divone, "").backgroundColor, "rgb(0, 0, 255)",
        ":hover applies hierarchically");
     is(getComputedStyle(divone.firstChild, "").backgroundColor, "rgb(255, 0, 0)",
        ":hover applies");
     synthesizeMouse(divone, 15, 7, moveEvent, window);
-    is(getComputedStyle(divone, "").backgroundColor, "transparent",
+    is(getComputedStyle(divone, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
-    is(getComputedStyle(divone.firstChild, "").backgroundColor, "transparent",
+    is(getComputedStyle(divone.firstChild, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     synthesizeMouse(divone, 15, 2, moveEvent, window);
     is(getComputedStyle(divone, "").backgroundColor, "rgb(0, 0, 255)",
        ":hover applies hierarchically");
     is(getComputedStyle(divone.firstChild, "").backgroundColor, "rgb(255, 0, 0)",
        ":hover applies");
 
     /** Test for Bug 302561 **/
@@ -110,17 +110,17 @@ var step3called = false;
 function step3() {
     is(step3called, false, "step3 called only once");
     step3called = true;
     if (getComputedStyle(iframe, "").width == "100px") {
         // The two resize events may be coalesced into a single one.
         step4();
         return;
     }
-    is(getComputedStyle(divtwo, "").backgroundColor, "transparent",
+    is(getComputedStyle(divtwo, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     setResize("step4()");
     /* expect to get a second resize from the oscillation */
 }
 
 var step4called = false;
 function step4() {
     is(step4called, false, "step4 called only once (more than two cycles of oscillation)");
@@ -139,29 +139,29 @@ function step5() {
     setResize("step6()");
     synthesizeMouse(divtwoparent, 25, 5, moveEvent, window);
 }
 
 var step6called = false;
 function step6() {
     is(step6called, false, "step6 called only once");
     step6called = true;
-    is(getComputedStyle(divtwo, "").backgroundColor, "transparent",
+    is(getComputedStyle(divtwo, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     synthesizeMouse(divtwoparent, 2, 5, moveEvent, window);
     setTimeout(step7, 500); // time to detect oscillations if they exist
 }
 
 var step7called = false;
 function step7() {
     is(step7called, false, "step7 called only once (more than two cycles of oscillation)");
     if (step7called)
         return;
     step7called = true;
-    is(getComputedStyle(divtwo, "").backgroundColor, "transparent",
+    is(getComputedStyle(divtwo, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     setTimeout(step8, 500); // time to detect oscillations if they exist
 }
 
 /* test the same case with scrolltop */
 
 var step8called = false;
 function step8() {
@@ -190,17 +190,17 @@ var step10called = false;
 function step10() {
     is(step10called, false, "step10 called only once");
     step10called = true;
     if (getComputedStyle(iframe, "").width == "100px") {
         // The two resize events may be coalesced into a single one.
         step11();
         return;
     }
-    is(getComputedStyle(divtwo, "").backgroundColor, "transparent",
+    is(getComputedStyle(divtwo, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     setResize("step11()");
     /* expect to get a second resize from the oscillation */
 }
 
 var step11called = false;
 function step11() {
     is(step11called, false, "step11 called only once (more than two cycles of oscillation)");
@@ -219,17 +219,17 @@ function step12() {
     setResize("step13()");
     divtwoparent.scrollLeft = 25; /* mouse now over 27,5 */
 }
 
 var step13called = false;
 function step13() {
     is(step13called, false, "step13 called only once");
     step13called = true;
-    is(getComputedStyle(divtwo, "").backgroundColor, "transparent",
+    is(getComputedStyle(divtwo, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     setResize("step14()");
     divtwoparent.scrollLeft = 0; /* mouse now over 2,5 */
 }
 
 var step14called = false;
 function step14() {
     is(step14called, false, "step14 called only once");
@@ -246,17 +246,17 @@ function step14() {
 }
 
 var step15called = false;
 function step15() {
     is(step15called, false, "step15 called only once (more than two cycles of oscillation)");
     if (step15called)
         return;
     step15called = true;
-    is(getComputedStyle(divtwo, "").backgroundColor, "transparent",
+    is(getComputedStyle(divtwo, "").backgroundColor, "rgba(0, 0, 0, 0)",
        ":hover does not apply");
     setTimeout(finish, 500); // time to detect oscillations if they exist
 }
 
 function finish() {
     document.getElementById("display").style.display = "none";
 
     var tester = window.SimpleTest;
--- a/layout/style/test/property_database.js
+++ b/layout/style/test/property_database.js
@@ -2230,18 +2230,18 @@ var gCSSProperties = {
     initial_values: [ "border-box" ],
     other_values: [ "content-box", "padding-box", "border-box, padding-box", "padding-box, padding-box, padding-box", "border-box, border-box" ],
     invalid_values: [ "margin-box", "border-box border-box", "fill-box", "stroke-box", "view-box", "no-clip" ]
   },
   "background-color": {
     domProp: "backgroundColor",
     inherited: false,
     type: CSS_TYPE_LONGHAND,
-    initial_values: [ "transparent", "rgba(255, 127, 15, 0)", "hsla(240, 97%, 50%, 0.0)", "rgba(0, 0, 0, 0)", "rgba(255,255,255,-3.7)" ],
-    other_values: [ "green", "rgb(255, 0, 128)", "#fc2", "#96ed2a", "black", "rgba(255,255,0,3)", "hsl(240, 50%, 50%)", "rgb(50%, 50%, 50%)", "-moz-default-background-color", "rgb(100, 100.0, 100)" ],
+    initial_values: [ "transparent", "rgba(0, 0, 0, 0)" ],
+    other_values: [ "green", "rgb(255, 0, 128)", "#fc2", "#96ed2a", "black", "rgba(255,255,0,3)", "hsl(240, 50%, 50%)", "rgb(50%, 50%, 50%)", "-moz-default-background-color", "rgb(100, 100.0, 100)", "rgba(255, 127, 15, 0)", "hsla(240, 97%, 50%, 0.0)", "rgba(255,255,255,-3.7)" ],
     invalid_values: [ "#0", "#00", "#00000", "#0000000", "#000000000", "rgb(100, 100%, 100)" ],
     quirks_values: { "000000": "#000000", "96ed2a": "#96ed2a" },
   },
   "background-image": {
     domProp: "backgroundImage",
     inherited: false,
     type: CSS_TYPE_LONGHAND,
     initial_values: [ "none" ],
--- a/layout/style/test/test_additional_sheets.html
+++ b/layout/style/test/test_additional_sheets.html
@@ -200,17 +200,17 @@ function loadAndCheck(win, firstType, se
     firstType.type + "(normal)" + " vs " + secondType.type + (swap ? "(important)" : "(normal)" ) + " 1");
   is(cs.getPropertyValue('background-color'), result2,
     firstType.type + "(important)" + " vs " + secondType.type + (swap ? "(normal)" : "(important)" ) + " 2");
 
   firstType.removeRules(win, firstStyle);
   secondType.removeRules(win, secondStyle);
 
   is(cs.getPropertyValue('color'), 'rgb(0, 0, 0)', firstType.type + " vs " + secondType.type + " 3");
-  is(cs.getPropertyValue('background-color'), 'transparent', firstType.type + " vs " + secondType.type + " 4");
+  is(cs.getPropertyValue('background-color'), 'rgba(0, 0, 0, 0)', firstType.type + " vs " + secondType.type + " 4");
 }
 
 // There are 8 cases. Regular against regular, regular against important, important
 // against regular, important against important. We can load style from typeA first
 // then typeB or the other way around so that's 4*2=8 cases.
 
 function testStyleVsStyle(win, typeA, typeB, results)
 {
--- a/layout/style/test/test_bug229915.html
+++ b/layout/style/test/test_bug229915.html
@@ -40,17 +40,17 @@ https://bugzilla.mozilla.org/show_bug.cg
 </div>
 <pre id="test">
 <script class="testbody" type="text/javascript">
 
 /** Test for Bug 229915 **/
 
 const GREEN = "rgb(0, 128, 0)";
 const BLACK = "rgb(0, 0, 0)";
-const TRANSPARENT = "transparent";
+const TRANSPARENT = "rgba(0, 0, 0, 0)";
 const WHITE = "rgb(255, 255, 255)";
 
 function make_prev() {
   var result = document.createElement("p");
   result.setAttribute("class", "prev");
   var t = document.createTextNode("Dynamically created previous paragraph.");
   result.appendChild(t);
   return result;
--- a/layout/style/test/test_bug372770.html
+++ b/layout/style/test/test_bug372770.html
@@ -30,28 +30,22 @@ var i;
 for (i = 0; i < colors.length; ++i) {
   var color = colors[i];
   style1.color = color;
   style2.color = color;
   is(style1.color, color, "Inline style color roundtripping failed at color " + i);
   is(style2.color, color, "Rule style color roundtripping failed at color " + i);
 }
 
-// This code is only here because of bug 372783.  Once that's fixed, this test
-// for "rgba(0, 0, 0, 0)" will fail.
 style1.color = "rgba(0, 0, 0, 0)";
 style2.color = "rgba(0, 0, 0, 0)";
-is(style1.color, "transparent",
-   "Inline style should give transparent for rgba(0,0,0,0)");
-is(style2.color, "transparent",
-   "Rule style should give transparent for rgba(0,0,0,0)");
-todo(style1.color == "rgba(0, 0, 0, 0)",
-     "Inline style should round-trip black transparent color correctly");
-todo(style2.color == "rgba(0, 0, 0, 0)",
-     "Rule style should round-trip black transparent color correctly");
+is(style1.color, "rgba(0, 0, 0, 0)",
+   "Inline style should round-trip black transparent color correctly");
+is(style2.color, "rgba(0, 0, 0, 0)",
+   "Rule style should round-trip black transparent color correctly");
 
 for (var i = 0; i <= 100; ++i) {
   if (i == 70 || i == 90) {
     // Tinderbox unhappy for some reason... just skip these for now?
     continue;
   }
   var color1 = "rgba(128, 128, 128, " + i/100 + ")";
   var color2 = "rgba(175, 63, 27, " + i/100 + ")";
--- a/layout/style/test/test_bug635286.html
+++ b/layout/style/test/test_bug635286.html
@@ -31,17 +31,17 @@ window.addEventListener("load", function
   var cases = Array.slice(document.getElementsByTagName("div"));
   cases.forEach(function(aCase, aIndex) {
     aCase.className = "after";
   });
   window.setTimeout(function() {
     cases.forEach(function(aCase, aIndex) {
       is(window.getComputedStyle(aCase)
            .getPropertyValue("background-color"),
-         "transparent",
+         "rgba(0, 0, 0, 0)",
          aCase.textContent);
     });
     SimpleTest.finish();
   }, 1);
 });
 
 SimpleTest.waitForExplicitFinish();
 SimpleTest.requestFlakyTimeout("untriaged");
--- a/layout/style/test/test_computed_style.html
+++ b/layout/style/test/test_computed_style.html
@@ -356,18 +356,18 @@ var noframe_container = document.getElem
 
   var testStyles = {
     "mask" : "",
     "markerStart" : "",
     "markerMid" : "",
     "markerEnd" : "",
     "clipPath" : "",
     "filter" : "",
-    "fill" : " transparent",
-    "stroke" : " transparent",
+    "fill" : " rgba(0, 0, 0, 0)",
+    "stroke" : " rgba(0, 0, 0, 0)",
   };
 
   for (var prop in testStyles) {
     p.style[prop] = localURL;
     is(cs[prop], localURL + testStyles[prop], "computed value of " + prop);
     p.style[prop] = nonLocalURL;
     is(cs[prop], resolvedNonLocalURL + testStyles[prop], "computed value of " + prop);
   }
--- a/layout/style/test/test_default_computed_style.html
+++ b/layout/style/test/test_default_computed_style.html
@@ -40,17 +40,17 @@ is(cs.marginTop, "0px", "We have 0 margi
 is(cs.backgroundColor, "rgb(255, 255, 0)", "We have yellow background");
 is(cs.color, "rgb(0, 0, 255)", "We have blue text");
 is(cs_pseudo.content, '"Visible"', "We have some content");
 is(cs_pseudo.display, "block", "Our ::before is block");
 
 // And now our actual tests
 is(cs_default.display, "block", "We have block display by default");
 is(cs_default.marginTop, "16px", "We have 16px margin by default");
-is(cs_default.backgroundColor, "transparent",
+is(cs_default.backgroundColor, "rgba(0, 0, 0, 0)",
    "We have transparent background by default");
 is(cs_default.color, "rgb(0, 0, 0)", "We have black text by default");
 is(cs_default_pseudo.content, "none", "We have no content by default");
 is(cs_default_pseudo.display, "inline", "Our ::before is inline by default");
 
 
 </script>
 </pre>