Bug 1557063 - Better checks for invalid CSS when using gap, column-gap and row-gap. r=miker,fluent-reviewers,flod
authorRazvan Caliman <rcaliman@mozilla.com>
Fri, 07 Jun 2019 21:09:41 +0000
changeset 478104 ed1862121e9a7c6d4cdb377bc0c29a6b1b5b8067
parent 478103 a97a1b4916a1a806593d61aa8ee5e72390ab01b3
child 478105 bcac61c955a3ec82ca9f8c9febfe6346acf8652c
push id36136
push usernerli@mozilla.com
push dateTue, 11 Jun 2019 03:18:15 +0000
treeherdermozilla-central@bb62c9157f04 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmiker, fluent-reviewers, flod
bugs1557063
milestone69.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 1557063 - Better checks for invalid CSS when using gap, column-gap and row-gap. r=miker,fluent-reviewers,flod Splits the checks by appropriate container: - column-gap and gap for flex-container, grid-container and multi-column container - row-gap only for flex container and grid container See CSS Box Alignment: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Box_Alignment Differential Revision: https://phabricator.services.mozilla.com/D33814
devtools/client/locales/en-US/tooltips.ftl
devtools/server/actors/utils/inactive-property-helper.js
devtools/server/tests/browser/browser_inspector-inactive-property-helper.js
--- a/devtools/client/locales/en-US/tooltips.ftl
+++ b/devtools/client/locales/en-US/tooltips.ftl
@@ -10,16 +10,18 @@ learn-more = <span data-l10n-name="link"
 ## an icon. When this icon is hovered this message is displayed to explain why
 ## the property is not applied.
 ## Variables:
 ##   $property (string) - A CSS property name e.g. "color".
 ##   $display (string) - A CSS display value e.g. "inline-block".
 
 inactive-css-not-grid-or-flex-container = <strong>{ $property }</strong> has no effect on this element since it’s neither a flex container nor a grid container.
 
+inactive-css-not-grid-or-flex-container-or-multicol-container = <strong>{ $property }</strong> has no effect on this element since it’s not a flex container, a grid container, or a multi-column container.
+
 inactive-css-not-grid-or-flex-item = <strong>{ $property }</strong> has no effect on this element since it’s not a grid or flex item.
 
 inactive-css-not-grid-item = <strong>{ $property }</strong> has no effect on this element since it’s not a grid item.
 
 inactive-css-not-grid-container = <strong>{ $property }</strong> has no effect on this element since it’s not a grid container.
 
 inactive-css-not-flex-item = <strong>{ $property }</strong> has no effect on this element since it’s not a flex item.
 
@@ -30,16 +32,18 @@ inactive-css-not-inline-or-tablecell = <strong>{ $property }</strong> has no effect on this element since it’s not an inline or table-cell element.
 inactive-css-property-because-of-display = <strong>{ $property }</strong> has no effect on this element since it has a display of <strong>{ $display }</strong>.
 
 ## In the Rule View when a CSS property cannot be successfully applied we display
 ## an icon. When this icon is hovered this message is displayed to explain how
 ## the problem can be solved.
 
 inactive-css-not-grid-or-flex-container-fix = Try adding <strong>display:grid</strong> or <strong>display:flex</strong>. { learn-more }
 
+inactive-css-not-grid-or-flex-container-or-multicol-container-fix = Try adding either <strong>display:grid</strong>, <strong>display:flex</strong>, or <strong>columns:2</strong>. { learn-more }
+
 inactive-css-not-grid-or-flex-item-fix = Try adding <strong>display:grid</strong>, <strong>display:flex</strong>, <strong>display:inline-grid</strong> or <strong>display:inline-flex</strong>. { learn-more }
 
 inactive-css-not-grid-item-fix =Try adding <strong>display:grid</strong> or <strong>display:inline-grid</strong> to the item’s parent. { learn-more }
 
 inactive-css-not-grid-container-fix = Try adding <strong>display:grid</strong> or <strong>display:inline-grid</strong>. { learn-more }
 
 inactive-css-not-flex-item-fix = Try adding <strong>display:flex</strong> or <strong>display:inline-flex</strong> to the item’s parent. { learn-more }
 
--- a/devtools/server/actors/utils/inactive-property-helper.js
+++ b/devtools/server/actors/utils/inactive-property-helper.js
@@ -79,18 +79,16 @@ class InactivePropertyHelper {
       // Grid container property used on non-grid container.
       {
         invalidProperties: [
           "grid-auto-columns",
           "grid-auto-flow",
           "grid-auto-rows",
           "grid-template",
           "grid-gap",
-          "row-gap",
-          "column-gap",
           "justify-items",
         ],
         when: () => !this.gridContainer,
         fixId: "inactive-css-not-grid-container-fix",
         msgId: "inactive-css-not-grid-container",
         numFixProps: 2,
       },
       // Grid item property used on non-grid item.
@@ -121,22 +119,34 @@ class InactivePropertyHelper {
         numFixProps: 4,
       },
       // Grid and flex container properties used on non-grid or non-flex container.
       {
         invalidProperties: [
           "align-content",
           "align-items",
           "justify-content",
+          "row-gap",
         ],
         when: () => !this.gridContainer && !this.flexContainer,
         fixId: "inactive-css-not-grid-or-flex-container-fix",
         msgId: "inactive-css-not-grid-or-flex-container",
         numFixProps: 2,
       },
+      // column-gap and shorthand used on non-grid or non-flex or non-multi-col container.
+      {
+        invalidProperties: [
+          "column-gap",
+          "gap",
+        ],
+        when: () => !this.gridContainer && !this.flexContainer && !this.multiColContainer,
+        fixId: "inactive-css-not-grid-or-flex-container-or-multicol-container-fix",
+        msgId: "inactive-css-not-grid-or-flex-container-or-multicol-container",
+        numFixProps: 3,
+      },
       // Inline properties used on non-inline-level elements.
       {
         invalidProperties: [
           "vertical-align",
         ],
         when: () => {
           const { selectorText } = this.cssRule;
 
@@ -382,16 +392,27 @@ class InactivePropertyHelper {
   /**
    * Check if the current node is a grid item.
    */
   get gridItem() {
     return this.isGridItem(this.node);
   }
 
   /**
+   * Check if the current node is a multi-column container, i.e. a node element whose
+   * `column-width` or `column-count` property is not `auto`.
+   */
+  get multiColContainer() {
+    const autoColumnWidth = this.checkStyle("column-width", ["auto"]);
+    const autoColumnCount = this.checkStyle("column-count", ["auto"]);
+
+    return !autoColumnWidth || !autoColumnCount;
+  }
+
+  /**
    * Check if the current node is a table row.
    */
   get tableRow() {
     return this.style && this.style.display === "table-row";
   }
 
   /**
    * Check if the current node is a row group.
--- a/devtools/server/tests/browser/browser_inspector-inactive-property-helper.js
+++ b/devtools/server/tests/browser/browser_inspector-inactive-property-helper.js
@@ -261,16 +261,94 @@ add_task(async function() {
     rules: ["div { min-height: 500px; }"],
     isActive: true,
   }, {
     info: "max-height is active on a block element",
     property: "max-height",
     tagName: "div",
     rules: ["div { max-height: 500px; }"],
     isActive: true,
+  }, {
+    info: "column-gap is inactive on non-grid and non-flex container",
+    property: "column-gap",
+    tagName: "div",
+    rules: ["div { column-gap: 10px; display: block; }"],
+    isActive: false,
+  }, {
+    info: "column-gap is active on grid container",
+    property: "column-gap",
+    tagName: "div",
+    rules: ["div { column-gap: 10px; display: grid; }"],
+    isActive: true,
+  }, {
+    info: "column-gap is active on flex container",
+    property: "column-gap",
+    tagName: "div",
+    rules: ["div { column-gap: 10px; display: flex; }"],
+    isActive: true,
+  }, {
+    info: "column-gap is inactive on non-multi-col container",
+    property: "column-gap",
+    tagName: "div",
+    rules: ["div { column-gap: 10px; column-count: auto; }"],
+    isActive: false,
+  }, {
+    info: "column-gap is active on multi-column container",
+    property: "column-gap",
+    tagName: "div",
+    rules: ["div { column-gap: 10px; column-count: 2; }"],
+    isActive: true,
+  }, {
+    info: "row-gap is inactive on non-grid and non-flex container",
+    property: "row-gap",
+    tagName: "div",
+    rules: ["div { row-gap: 10px; display: block; }"],
+    isActive: false,
+  }, {
+    info: "row-gap is active on grid container",
+    property: "row-gap",
+    tagName: "div",
+    rules: ["div { row-gap: 10px; display: grid; }"],
+    isActive: true,
+  }, {
+    info: "row-gap is active on flex container",
+    property: "row-gap",
+    tagName: "div",
+    rules: ["div { row-gap: 10px; display: flex; }"],
+    isActive: true,
+  }, {
+    info: "gap is inactive on non-grid and non-flex container",
+    property: "gap",
+    tagName: "div",
+    rules: ["div { gap: 10px; display: block; }"],
+    isActive: false,
+  }, {
+    info: "gap is active on flex container",
+    property: "gap",
+    tagName: "div",
+    rules: ["div { gap: 10px; display: flex; }"],
+    isActive: true,
+  }, {
+    info: "gap is active on grid container",
+    property: "gap",
+    tagName: "div",
+    rules: ["div { gap: 10px; display: grid; }"],
+    isActive: true,
+  }, {
+    info: "gap is inactive on non-multi-col container",
+    property: "gap",
+    tagName: "div",
+    rules: ["div { gap: 10px; column-count: auto; }"],
+    isActive: false,
+  }, {
+    info: "gap is active on multi-col container",
+    property: "gap",
+    tagName: "div",
+    rules: ["div { gap: 10px; column-count: 2; }"],
+    isActive: true,
   }];
 
   for (const {info: summary, property, tagName, rules, ruleIndex, isActive} of tests) {
     info(summary);
 
     // Create an element which will contain the test elements.
     const main = document.createElement("main");
     document.firstElementChild.appendChild(main);