Bug 1387410: Stop the grid shorthand from resetting grid-gap properties. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 06 Sep 2017 13:32:04 +0200
changeset 661284 3fc4a13d41ca0d90edd112a210102564c4cd6545
parent 661283 95430ac5c638f16cb20bca4abcffd02f9092b81c
child 661285 d9db66be23fbd58a69862dd76e25e3e892d8cc82
push id78700
push userbmo:emilio@crisal.io
push dateFri, 08 Sep 2017 08:10:44 +0000
reviewersheycam
bugs1387410
milestone57.0a1
Bug 1387410: Stop the grid shorthand from resetting grid-gap properties. r?heycam MozReview-Commit-ID: GxU9YuAUc00
layout/reftests/css-grid/grid-item-button-001.html
layout/reftests/css-grid/grid-item-canvas-001.html
layout/reftests/css-grid/grid-item-overflow-stretch-003-ref.html
layout/style/Declaration.cpp
layout/style/nsCSSParser.cpp
layout/style/nsCSSProps.cpp
layout/style/test/property_database.js
layout/style/test/test_grid_shorthand_serialization.html
--- a/layout/reftests/css-grid/grid-item-button-001.html
+++ b/layout/reftests/css-grid/grid-item-button-001.html
@@ -90,31 +90,31 @@ button:nth-child(4n) { background: silve
 <button class="jend">AB</button>
 <button class="jend">AB</button>
 <button class="aend">AB</button>
 <button class="aend">AB</button>
 <button class="end">AB</button>
 <button class="end">AB</button>
 </div>
 
-<div class="grid" style="grid:auto/auto auto">
+<div class="grid" style="grid: auto/auto auto; grid-gap: 0;">
 <button>AB</button>
 <button class="jend">AB</button>
 <button class="aend">AB</button>
 <button class="end">AB</button>
 </div>
 
-<div class="grid" style="grid:50px 50px/50px 50px">
+<div class="grid" style="grid: 50px 50px/50px 50px; grid-gap: 0;">
 <button>AB</button>
 <button class="jend">&nbsp;&nbsp;</button>
 <button class="aend">AB</button>
 <button class="end">&nbsp;&nbsp;</button>
 </div>
 
-<div class="grid" style="grid:minmax(auto,20px) 20px/auto auto">
+<div class="grid" style="grid: minmax(auto,20px) 20px/auto auto; grid-gap: 0;">
 <button>AB</button>
 <button class="jend">AB</button>
 <button class="aend">AB</button>
 <button class="end">AB</button>
 </div>
 
 <div class="grid sz t2 mw">
 <button>AB</button>
@@ -137,24 +137,24 @@ button:nth-child(4n) { background: silve
 <button class="jend">AB</button>
 <button class="jend">AB</button>
 <button class="aend">AB</button>
 <button class="aend">AB</button>
 <button class="end">AB</button>
 <button class="end">AB</button>
 </div>
 
-<div class="grid max40" style="grid:50px 50px/50px 50px">
+<div class="grid max40" style="grid:50px 50px/50px 50px; grid-gap: 0;">
 <button>AB</button>
 <button class="jend">&nbsp;&nbsp;</button>
 <button class="aend">AB</button>
 <button class="end">&nbsp;&nbsp;</button>
 </div>
 
-<div class="grid max40" style="grid:50px 50px/50px 50px; place-items:stretch">
+<div class="grid max40" style="grid:50px 50px/50px 50px; place-items:stretch; grid-gap: 0;">
 <button>AB</button>
 <button class="jend">&nbsp;&nbsp;</button>
 <button class="aend">AB</button>
 <button class="end">&nbsp;&nbsp;</button>
 </div>
 
 </body>
 </html>
--- a/layout/reftests/css-grid/grid-item-canvas-001.html
+++ b/layout/reftests/css-grid/grid-item-canvas-001.html
@@ -73,24 +73,24 @@ canvas:nth-child(4n) { background: black
 <canvas class="jend"></canvas>
 <canvas class="jend"></canvas>
 <canvas class="aend"></canvas>
 <canvas class="aend"></canvas>
 <canvas class="end"></canvas>
 <canvas class="end"></canvas>
 </div>
 
-<div class="grid" style="grid:auto/auto auto">
+<div class="grid" style="grid: auto/auto auto; grid-gap: 0;">
 <canvas></canvas>
 <canvas class="jend"></canvas>
 <canvas class="aend"></canvas>
 <canvas class="end"></canvas>
 </div>
 
-<div class="grid" style="grid:minmax(auto,30px) 30px/auto auto">
+<div class="grid" style="grid: minmax(auto,30px) 30px/auto auto; grid-gap: 0">
 <canvas></canvas>
 <canvas class="jend"></canvas>
 <canvas class="aend"></canvas>
 <canvas class="end"></canvas>
 </div>
 
 </body>
 </html>
--- a/layout/reftests/css-grid/grid-item-overflow-stretch-003-ref.html
+++ b/layout/reftests/css-grid/grid-item-overflow-stretch-003-ref.html
@@ -14,17 +14,17 @@ body,html { color:black; background:whit
   display: inline-grid;
   width: 100px;
   height: 50px;
   grid: 7px 30px 3px / 7px 112px 3px;
   grid-gap: 5px;
   border:1px solid;
 }
 .c2 { grid-template-columns: 7px 122px 3px; }
-.h > .grid { grid: 7px 112px 3px / 7px 80px 3px; }
+.h > .grid { grid: 7px 112px 3px / 7px 80px 3px; grid-gap: 0; }
 
 .grid > * {
   grid-area: 2/2;
   border:1px solid;
   margin: 0 auto;
   justify-self:start;
   align-self:start;
   height:28px;
--- a/layout/style/Declaration.cpp
+++ b/layout/style/Declaration.cpp
@@ -1214,28 +1214,16 @@ Declaration::GetPropertyValueInternal(
       break;
     }
 
     // The 'grid' shorthand has 3 different possibilities for syntax:
     // #1 <'grid-template'>
     // #2 <'grid-template-rows'> / [ auto-flow && dense? ] <'grid-auto-columns'>?
     // #3 [ auto-flow && dense? ] <'grid-auto-rows'>? / <'grid-template-columns'>
     case eCSSProperty_grid: {
-      const nsCSSValue& columnGapValue =
-        *data->ValueFor(eCSSProperty_grid_column_gap);
-      if (columnGapValue.GetUnit() != eCSSUnit_Pixel ||
-          columnGapValue.GetFloatValue() != 0.0f) {
-        return; // Not serializable, bail.
-      }
-      const nsCSSValue& rowGapValue =
-        *data->ValueFor(eCSSProperty_grid_row_gap);
-      if (rowGapValue.GetUnit() != eCSSUnit_Pixel ||
-          rowGapValue.GetFloatValue() != 0.0f) {
-        return; // Not serializable, bail.
-      }
       const nsCSSValue& areasValue =
         *data->ValueFor(eCSSProperty_grid_template_areas);
       const nsCSSValue& columnsValue =
         *data->ValueFor(eCSSProperty_grid_template_columns);
       const nsCSSValue& rowsValue =
         *data->ValueFor(eCSSProperty_grid_template_rows);
 
       const nsCSSValue& autoFlowValue =
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -9446,23 +9446,16 @@ CSSParserImpl::ParseGrid()
     for (const nsCSSPropertyID* subprops =
            nsCSSProps::SubpropertyEntryFor(eCSSProperty_grid);
          *subprops != eCSSProperty_UNKNOWN; ++subprops) {
       AppendValue(*subprops, value);
     }
     return true;
   }
 
-  // https://drafts.csswg.org/css-grid/#grid-shorthand
-  // "Also, the gutter properties are reset by this shorthand,
-  //  even though they can't be set by it."
-  value.SetFloatValue(0.0f, eCSSUnit_Pixel);
-  AppendValue(eCSSProperty_grid_row_gap, value);
-  AppendValue(eCSSProperty_grid_column_gap, value);
-
   // [ auto-flow && dense? ] <'grid-auto-rows'>? / <'grid-template-columns'>
   auto res = ParseGridShorthandAutoProps(NS_STYLE_GRID_AUTO_FLOW_ROW);
   if (res == CSSParseResult::Error) {
     return false;
   }
   if (res == CSSParseResult::Ok) {
     value.SetAutoValue();
     AppendValue(eCSSProperty_grid_auto_columns, value);
--- a/layout/style/nsCSSProps.cpp
+++ b/layout/style/nsCSSProps.cpp
@@ -2872,18 +2872,16 @@ static const nsCSSPropertyID gGridTempla
 
 static const nsCSSPropertyID gGridSubpropTable[] = {
   eCSSProperty_grid_template_areas,
   eCSSProperty_grid_template_rows,
   eCSSProperty_grid_template_columns,
   eCSSProperty_grid_auto_flow,
   eCSSProperty_grid_auto_rows,
   eCSSProperty_grid_auto_columns,
-  eCSSProperty_grid_row_gap, // can only be reset, not get/set
-  eCSSProperty_grid_column_gap, // can only be reset, not get/set
   eCSSProperty_UNKNOWN
 };
 
 static const nsCSSPropertyID gGridColumnSubpropTable[] = {
   eCSSProperty_grid_column_start,
   eCSSProperty_grid_column_end,
   eCSSProperty_UNKNOWN
 };
--- a/layout/style/test/property_database.js
+++ b/layout/style/test/property_database.js
@@ -6739,18 +6739,16 @@ if (IsCSSPropertyPrefEnabled("layout.css
     type: CSS_TYPE_TRUE_SHORTHAND,
     subproperties: [
       "grid-template-areas",
       "grid-template-rows",
       "grid-template-columns",
       "grid-auto-flow",
       "grid-auto-rows",
       "grid-auto-columns",
-      "grid-column-gap",
-      "grid-row-gap",
     ],
     initial_values: [
       "none",
       "none / none",
     ],
     other_values: [
       "auto-flow 40px / none",
       "auto-flow / 40px",
--- a/layout/style/test/test_grid_shorthand_serialization.html
+++ b/layout/style/test/test_grid_shorthand_serialization.html
@@ -157,40 +157,32 @@ grid_test_cases = grid_template_test_cas
     {
         gridAutoFlow: "row",
         gridAutoRows: "40px",
         gridTemplateColumns: "100px",
         shorthand: "auto-flow 40px / 100px",
     },
     {
         gridAutoFlow: "row",
-        gridRowGap: "0px",
+        shorthand: "none / none",
+    },
+    // Test that grid-gap properties don't affect serialization.
+    {
+        gridRowGap: "1px",
         shorthand: "none / none",
     },
     {
-        gridAutoFlow: "row",
-        gridRowGap: "1px",
-        shorthand: "",
-    },
-    {
-        gridAutoFlow: "row",
         gridColumnGap: "1px",
-        shorthand: "",
+        shorthand: "none / none",
     },
 ]);
 
 var grid_important_test_cases = [
     {
         "grid-auto-flow": "row",
-        "grid-row-gap": "0px",
-        shorthand: "",
-    },
-    {
-        "grid-auto-flow": "row",
-        "grid-column-gap": "1px",
         shorthand: "",
     },
 ];
 
 
 function run_tests(test_cases, shorthand, subproperties) {
     test_cases.forEach(function(test_case) {
         test(function() {
@@ -220,17 +212,17 @@ function run_important_tests(test_cases,
     });
 }
 
 run_tests(grid_template_test_cases, "gridTemplate", [
     "gridTemplateAreas", "gridTemplateColumns", "gridTemplateRows"]);
 
 run_tests(grid_test_cases, "grid", [
     "gridTemplateAreas", "gridTemplateColumns", "gridTemplateRows",
-    "gridAutoFlow", "gridAutoColumns", "gridAutoRows", "gridColumnGap", "gridRowGap"]);
+    "gridAutoFlow", "gridAutoColumns", "gridAutoRows"]);
 
 run_important_tests(grid_important_test_cases, "grid", [
     "grid-template-areas", "grid-template-columns", "grid-template-rows",
-    "grid-auto-flow", "grid-auto-columns", "grid-auto-rows", "grid-column-gap", "grid-row-gap"]);
+    "grid-auto-flow", "grid-auto-columns", "grid-auto-rows"]);
 
 </script>
 </body>
 </html>