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 660407 04ca21e7aa222496b82949c2004d420b0a338e62
parent 660406 08f5c5e4f3b1cc4dd50e1bdb8bb25b7af168f52b
child 660408 995328c9e946d6dbfab6f9f07e4091adefe22e89
push id78390
push userbmo:emilio@crisal.io
push dateWed, 06 Sep 2017 23:04:15 +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
servo/components/style/properties/shorthand/position.mako.rs
--- 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>
--- a/servo/components/style/properties/shorthand/position.mako.rs
+++ b/servo/components/style/properties/shorthand/position.mako.rs
@@ -443,26 +443,25 @@
             serialize_grid_template(self.grid_template_rows, self.grid_template_columns,
                                     self.grid_template_areas, dest)
         }
     }
 </%helpers:shorthand>
 
 <%helpers:shorthand name="grid"
                     sub_properties="grid-template-rows grid-template-columns grid-template-areas
-                                    grid-auto-rows grid-auto-columns grid-row-gap grid-column-gap
-                                    grid-auto-flow"
+                                    grid-auto-rows grid-auto-columns grid-auto-flow"
                     spec="https://drafts.csswg.org/css-grid/#propdef-grid"
                     products="gecko">
     use parser::Parse;
     use properties::longhands::{grid_auto_columns, grid_auto_rows, grid_auto_flow};
     use properties::longhands::grid_auto_flow::computed_value::{AutoFlow, T as SpecifiedAutoFlow};
     use values::{Either, None_};
     use values::generics::grid::{GridTemplateComponent, TrackListType};
-    use values::specified::{GenericGridTemplateComponent, NonNegativeLengthOrPercentage, TrackSize};
+    use values::specified::{GenericGridTemplateComponent, TrackSize};
 
     pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)
                                -> Result<Longhands, ParseError<'i>> {
         let mut temp_rows = GridTemplateComponent::None;
         let mut temp_cols = GridTemplateComponent::None;
         let mut temp_areas = Either::Second(None_);
         let mut auto_rows = TrackSize::default();
         let mut auto_cols = TrackSize::default();
@@ -512,42 +511,31 @@
 
         Ok(expanded! {
             grid_template_rows: temp_rows,
             grid_template_columns: temp_cols,
             grid_template_areas: temp_areas,
             grid_auto_rows: auto_rows,
             grid_auto_columns: auto_cols,
             grid_auto_flow: flow,
-            // This shorthand also resets grid gap
-            grid_row_gap: NonNegativeLengthOrPercentage::zero(),
-            grid_column_gap: NonNegativeLengthOrPercentage::zero(),
         })
     }
 
     impl<'a> LonghandsToSerialize<'a> {
         /// Returns true if other sub properties except template-{rows,columns} are initial.
         fn is_grid_template(&self) -> bool {
             *self.grid_template_areas == Either::Second(None_) &&
             *self.grid_auto_rows == TrackSize::default() &&
             *self.grid_auto_columns == TrackSize::default() &&
             *self.grid_auto_flow == grid_auto_flow::get_initial_value()
         }
     }
 
     impl<'a> ToCss for LonghandsToSerialize<'a> {
         fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-            // `grid` shorthand resets these properties. If they are not zero, that means they
-            // are changed by longhands and in that case we should fail serializing `grid`.
-            if *self.grid_row_gap != NonNegativeLengthOrPercentage::zero() ||
-               *self.grid_column_gap != NonNegativeLengthOrPercentage::zero() {
-                return Ok(());
-            }
-
-
             if *self.grid_template_areas != Either::Second(None_) ||
                (*self.grid_template_rows != GridTemplateComponent::None &&
                    *self.grid_template_columns != GridTemplateComponent::None) ||
                self.is_grid_template() {
                 return super::grid_template::serialize_grid_template(self.grid_template_rows,
                                                                      self.grid_template_columns,
                                                                      self.grid_template_areas, dest);
             }