Bug 1551586 - Display a warning when min-width, min-height etc. are used incorrectly. r=miker,fluent-reviewers,flod.
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Tue, 04 Jun 2019 20:33:58 +0000
changeset 476911 31cec3975dd0988d798cb63711477e9745339064
parent 476910 9c513f0d9c67fc9545de85e471a5fe0f1ccb4643
child 476921 155a7e2117e575ff6de6caa3dfe5b076cb455ae1
child 476922 d80312b41cd46b99ba638c548cde716502b45868
push id36110
push usermalexandru@mozilla.com
push dateWed, 05 Jun 2019 09:49:10 +0000
treeherdermozilla-central@31cec3975dd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmiker, fluent-reviewers, flod
bugs1551586
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 1551586 - Display a warning when min-width, min-height etc. are used incorrectly. r=miker,fluent-reviewers,flod. This introduce a new form of invalid messages. Until now, all the messages were like "X has no effect on this since it's not Y". But in this case, the width/height properties applies to all but a few cases, which means we can't really keep the same shape of message (or it would be "since it's not A, B, C, D, ...). So we're switching to a message that prints the display property of the element ("X has no effect on this element since it has a display of Y"). In order to do that, we need to pipe the element computed display into the inactive tooltip. Differential Revision: https://phabricator.services.mozilla.com/D32805
devtools/client/locales/en-US/tooltips.ftl
devtools/client/shared/widgets/tooltip/inactive-css-tooltip-helper.js
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
@@ -6,31 +6,34 @@
 
 learn-more = <span data-l10n-name="link">Learn more</span>
 
 ## 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 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-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.
 
 inactive-css-not-flex-container = <strong>{ $property }</strong> has no effect on this element since it’s not a flex container.
 
 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-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 }
 
@@ -38,8 +41,12 @@ 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 }
 
 inactive-css-not-flex-container-fix = Try adding <strong>display:flex</strong> or <strong>display:inline-flex</strong>. { learn-more }
 
 inactive-css-not-inline-or-tablecell-fix = Try adding <strong>display:inline</strong> or <strong>display:table-cell</strong>. { learn-more }
+
+inactive-css-non-replaced-inline-or-table-row-or-row-group-fix = Try adding <strong>display:inline-block</strong> or <strong>display:block</strong>. { learn-more }
+
+inactive-css-non-replaced-inline-or-table-column-or-column-group-fix = Try adding <strong>display:inline-block</strong>. { learn-more }
--- a/devtools/client/shared/widgets/tooltip/inactive-css-tooltip-helper.js
+++ b/devtools/client/shared/widgets/tooltip/inactive-css-tooltip-helper.js
@@ -71,30 +71,30 @@ class InactiveCssTooltipHelper {
    *                          // tooltip.
    *          property: "color", // Property name
    *        }
    * @param {HTMLTooltip} tooltip
    *        The tooltip we are targetting.
    */
   getTemplate(data, tooltip) {
     const XHTML_NS = "http://www.w3.org/1999/xhtml";
-    const { fixId, msgId, numFixProps, property } = data;
+    const { fixId, msgId, numFixProps, property, display } = data;
     const { doc } = tooltip;
 
     this._currentTooltip = tooltip;
     this._currentUrl = `https://developer.mozilla.org/docs/Web/CSS/${property}` +
                        `?utm_source=devtools&utm_medium=inspector-inactive-css`;
 
     const templateNode = doc.createElementNS(XHTML_NS, "template");
 
     // eslint-disable-next-line
     templateNode.innerHTML = `
     <div class="devtools-tooltip-inactive-css">
       <p data-l10n-id="${msgId}"
-         data-l10n-args='${JSON.stringify({property})}'>
+         data-l10n-args='${JSON.stringify({property, display})}'>
         <strong></strong>
       </p>
       <p data-l10n-id="${fixId}">
         ${"<strong></strong>".repeat(numFixProps)}
         <span data-l10n-name="link" class="link"></span>
       </p>
     </div>`;
 
--- a/devtools/server/actors/utils/inactive-property-helper.js
+++ b/devtools/server/actors/utils/inactive-property-helper.js
@@ -144,16 +144,44 @@ class InactivePropertyHelper {
           const isFirstLine = selectorText && selectorText.includes("::first-line");
 
           return !this.isInlineLevel() && !isFirstLetter && !isFirstLine;
         },
         fixId: "inactive-css-not-inline-or-tablecell-fix",
         msgId: "inactive-css-not-inline-or-tablecell",
         numFixProps: 2,
       },
+      // (max-|min-)width used on inline elements, table rows, or row groups.
+      {
+        invalidProperties: [
+          "max-width",
+          "min-width",
+          "width",
+        ],
+        when: () => this.nonReplacedInlineBox
+          || this.tableRow
+          || this.rowGroup,
+        fixId: "inactive-css-non-replaced-inline-or-table-row-or-row-group-fix",
+        msgId: "inactive-css-property-because-of-display",
+        numFixProps: 2,
+      },
+      // (max-|min-)height used on inline elements, table columns, or column groups.
+      {
+        invalidProperties: [
+          "max-height",
+          "min-height",
+          "height",
+        ],
+        when: () => this.nonReplacedInlineBox
+          || this.tableColumn
+          || this.columnGroup,
+        fixId: "inactive-css-non-replaced-inline-or-table-column-or-column-group-fix",
+        msgId: "inactive-css-property-because-of-display",
+        numFixProps: 1,
+      },
     ];
   }
 
   get unusedCssEnabled() {
     if (!this._unusedCssEnabled) {
       this._unusedCssEnabled = Services.prefs.getBoolPref(PREF_UNUSED_CSS_ENABLED, false);
     }
     return this._unusedCssEnabled;
@@ -167,25 +195,27 @@ class InactivePropertyHelper {
    * @param {Style} elStyle
    *        The computed style for this DOMNode.
    * @param {DOMRule} cssRule
    *        The CSS rule the property is defined in.
    * @param {String} property
    *        The CSS property name.
    *
    * @return {Object} object
-   * @return {Boolean} object.fixId
+   * @return {String} object.display
+   *         The element computed display value.
+   * @return {String} object.fixId
    *         A Fluent id containing a suggested solution to the problem that is
    *         causing a property to be inactive.
-   * @return {Boolean} object.msgId
+   * @return {String} object.msgId
    *         A Fluent id containing an error message explaining why a property
    *         is inactive in this situation.
-   * @return {Boolean} object.numFixProps
+   * @return {Integer} object.numFixProps
    *         The number of properties we suggest in the fixId string.
-   * @return {Boolean} object.property
+   * @return {String} object.property
    *         The inactive property name.
    * @return {Boolean} object.used
    *         true if the property is used.
    */
   isPropertyUsed(el, elStyle, cssRule, property) {
     if (!this.unusedCssEnabled) {
       return {used: true};
     }
@@ -221,17 +251,25 @@ class InactivePropertyHelper {
         used = false;
 
         return true;
       }
 
       return false;
     });
 
+    // Accessing this.style might throws, we wrap it in a try/catch block to avoid test
+    // failures.
+    let display;
+    try {
+      display = this.style ? this.style.display : null;
+    } catch (e) {}
+
     return {
+      display,
       fixId,
       msgId,
       numFixProps,
       property,
       used,
     };
   }
 
@@ -289,16 +327,19 @@ class InactivePropertyHelper {
    * @param {DOMNode} node
    *        The node to check.
    * @param {String} propName
    *        Property name to check.
    * @param {Array} values
    *        Values to compare against.
    */
   checkStyleForNode(node, propName, values) {
+    if (!this.style) {
+      return false;
+    }
     return values.some(value => this.style[propName] === value);
   }
 
   /**
    *  Check if the current node is an inline-level box.
    */
   isInlineLevel() {
     return this.checkStyle("display", [
@@ -341,16 +382,112 @@ 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 table row.
+   */
+  get tableRow() {
+    return this.style && this.style.display === "table-row";
+  }
+
+  /**
+   * Check if the current node is a row group.
+   */
+  get rowGroup() {
+    return this.style && (
+      this.style.display === "table-row-group" ||
+      this.style.display === "table-header-group" ||
+      this.style.display === "table-footer-group"
+    );
+  }
+
+  /**
+   * Check if the current node is a table column.
+   */
+  get tableColumn() {
+    return this.style && this.style.display === "table-column";
+  }
+
+  /**
+   * Check if the current node is a table column group.
+   */
+  get columnGroup() {
+    return this.style && this.style.display === "table-column-group";
+  }
+
+  /**
+   * Check if the current node is a non-replaced inline box.
+   */
+  get nonReplacedInlineBox() {
+    return this.nonReplaced && this.style && this.style.display === "inline";
+  }
+
+  /**
+   * Check if the current node is a non-replaced element. See `replaced()` for
+   * a description of what a replaced element is.
+   */
+  get nonReplaced() {
+    return !this.replaced;
+  }
+
+  /**
+   * Check if the current node is a replaced element i.e. an element with
+   * content that will be replaced e.g. <img>, <audio>, <video> or <object>
+   * elements.
+   */
+  get replaced() {
+    // The <applet> element was removed in Gecko 56 so we can ignore them.
+    // These are always treated as replaced elements:
+    if (this.nodeNameOneOf([
+      "br", "button", "canvas", "embed", "hr", "iframe", "math",
+      "object", "picture", "svg", "video",
+    ])) {
+      return true;
+    }
+
+    // audio – Treated as a replaced element only when it's "exposing a user
+    // interface element" i.e. has a "controls" attribute.
+    if (this.nodeName === "audio" && this.node.getAttribute("controls")) {
+      return true;
+    }
+
+    // img tags are replaced elements only when the image has finished loading.
+    if (this.nodeName === "img" && this.node.complete) {
+      return true;
+    }
+
+    return false;
+  }
+
+  /**
+   * Return the current node's nodeName.
+   *
+   * @returns {String}
+   */
+  get nodeName() {
+    return this.node.nodeName;
+  }
+
+  /**
+   * Check if the current node's nodeName matches a value inside the value array.
+   *
+   * @param {Array} values
+   *        Array of values to compare against.
+   * @returns {Boolean}
+   */
+  nodeNameOneOf(values) {
+    return values.includes(this.nodeName);
+  }
+
+  /**
    * Check if a node is a flex item.
    *
    * @param {DOMNode} node
    *        The node to check.
    */
   isFlexItem(node) {
     return !!node.parentFlexElement;
   }
@@ -382,26 +519,27 @@ class InactivePropertyHelper {
    *        The node to check.
    */
   isGridItem(node) {
     return !!this.getParentGridElement(this.node);
   }
 
   getParentGridElement(node) {
     if (node.nodeType === node.ELEMENT_NODE) {
-      const display = this.style.display;
+      const display = this.style ? this.style.display : null;
 
       if (!display || display === "none" || display === "contents") {
         // Doesn't generate a box, not a grid item.
         return null;
       }
-      const position = this.style.position;
+      const position = this.style ? this.style.position : null;
+      const cssFloat = this.style ? this.style.cssFloat : null;
       if (position === "absolute" ||
           position === "fixed" ||
-          this.style.cssFloat !== "none") {
+          cssFloat !== "none") {
         // Out of flow, not a grid item.
         return null;
       }
     } else if (node.nodeType !== node.TEXT_NODE) {
       return null;
     }
 
     for (let p = node.flattenedTreeParentNode; p; p = p.flattenedTreeParentNode) {
--- a/devtools/server/tests/browser/browser_inspector-inactive-property-helper.js
+++ b/devtools/server/tests/browser/browser_inspector-inactive-property-helper.js
@@ -63,16 +63,214 @@ add_task(async function() {
     rules: ["div::first-line { vertical-align: top; }"],
     isActive: true,
   }, {
     info: "vertical-align is active on an inline element",
     property: "vertical-align",
     tagName: "span",
     rules: ["span { vertical-align: top; }"],
     isActive: true,
+  }, {
+    info: "width is inactive on a non-replaced inline element",
+    property: "width",
+    tagName: "span",
+    rules: ["span { width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "min-width is inactive on a non-replaced inline element",
+    property: "min-width",
+    tagName: "span",
+    rules: ["span { min-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "max-width is inactive on a non-replaced inline element",
+    property: "max-width",
+    tagName: "span",
+    rules: ["span { max-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "width is inactive on an tr element",
+    property: "width",
+    tagName: "tr",
+    rules: ["tr { width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "min-width is inactive on an tr element",
+    property: "min-width",
+    tagName: "tr",
+    rules: ["tr { min-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "max-width is inactive on an tr element",
+    property: "max-width",
+    tagName: "tr",
+    rules: ["tr { max-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "width is inactive on an thead element",
+    property: "width",
+    tagName: "thead",
+    rules: ["thead { width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "min-width is inactive on an thead element",
+    property: "min-width",
+    tagName: "thead",
+    rules: ["thead { min-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "max-width is inactive on an thead element",
+    property: "max-width",
+    tagName: "thead",
+    rules: ["thead { max-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "width is inactive on an tfoot element",
+    property: "width",
+    tagName: "tfoot",
+    rules: ["tfoot { width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "min-width is inactive on an tfoot element",
+    property: "min-width",
+    tagName: "tfoot",
+    rules: ["tfoot { min-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "max-width is inactive on an tfoot element",
+    property: "max-width",
+    tagName: "tfoot",
+    rules: ["tfoot { max-width: 500px; }"],
+    isActive: false,
+  }, {
+    info: "width is active on a replaced inline element",
+    property: "width",
+    tagName: "img",
+    rules: ["img { width: 500px; }"],
+    isActive: true,
+  }, {
+    info: "min-width is active on a replaced inline element",
+    property: "min-width",
+    tagName: "img",
+    rules: ["img { min-width: 500px; }"],
+    isActive: true,
+  }, {
+    info: "max-width is active on a replaced inline element",
+    property: "max-width",
+    tagName: "img",
+    rules: ["img { max-width: 500px; }"],
+    isActive: true,
+  }, {
+    info: "width is active on a block element",
+    property: "width",
+    tagName: "div",
+    rules: ["div { width: 500px; }"],
+    isActive: true,
+  }, {
+    info: "min-width is active on a block element",
+    property: "min-width",
+    tagName: "div",
+    rules: ["div { min-width: 500px; }"],
+    isActive: true,
+  }, {
+    info: "max-width is active on a block element",
+    property: "max-width",
+    tagName: "div",
+    rules: ["div { max-width: 500px; }"],
+    isActive: true,
+  }, {
+    info: "height is inactive on a non-replaced inline element",
+    property: "height",
+    tagName: "span",
+    rules: ["span { height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "min-height is inactive on a non-replaced inline element",
+    property: "min-height",
+    tagName: "span",
+    rules: ["span { min-height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "max-height is inactive on a non-replaced inline element",
+    property: "max-height",
+    tagName: "span",
+    rules: ["span { max-height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "height is inactive on colgroup element",
+    property: "height",
+    tagName: "colgroup",
+    rules: ["colgroup { height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "min-height is inactive on colgroup element",
+    property: "min-height",
+    tagName: "colgroup",
+    rules: ["colgroup { min-height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "max-height is inactive on colgroup element",
+    property: "max-height",
+    tagName: "colgroup",
+    rules: ["colgroup { max-height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "height is inactive on col element",
+    property: "height",
+    tagName: "col",
+    rules: ["col { height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "min-height is inactive on col element",
+    property: "min-height",
+    tagName: "col",
+    rules: ["col { min-height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "max-height is inactive on col element",
+    property: "max-height",
+    tagName: "col",
+    rules: ["col { max-height: 500px; }"],
+    isActive: false,
+  }, {
+    info: "height is active on a replaced inline element",
+    property: "height",
+    tagName: "img",
+    rules: ["img { height: 500px; }"],
+    isActive: true,
+  }, {
+    info: "min-height is active on a replaced inline element",
+    property: "min-height",
+    tagName: "img",
+    rules: ["img { min-height: 500px; }"],
+    isActive: true,
+  }, {
+    info: "max-height is active on a replaced inline element",
+    property: "max-height",
+    tagName: "img",
+    rules: ["img { max-height: 500px; }"],
+    isActive: true,
+  }, {
+    info: "height is active on a block element",
+    property: "height",
+    tagName: "div",
+    rules: ["div { height: 500px; }"],
+    isActive: true,
+  }, {
+    info: "min-height is active on a block element",
+    property: "min-height",
+    tagName: "div",
+    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,
   }];
 
   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);