Bug 1358479 - Check if a grid fragment is valid before highlighting it; r=jdescottes
authorPatrick Brosset <pbrosset@mozilla.com>
Tue, 25 Apr 2017 14:16:57 +0200
changeset 356460 73696a2f5ec643bd4d503b1b858f1bbfc60b51d8
parent 356459 a86584f1364d236034e7860e7f6b269cc724f3c7
child 356461 01424585603f033bead0b84198f48fe79800a442
push id31767
push usercbook@mozilla.com
push dateFri, 05 May 2017 13:15:58 +0000
treeherdermozilla-central@8872ad4d52b6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes
bugs1358479
milestone55.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 1358479 - Check if a grid fragment is valid before highlighting it; r=jdescottes Grid fragments returned by the chrome-only el.getGridFragments() API may sometimes in fact be empty. When they are, the returned object still looks like a fragment, but has either no rows or no columns. So this commit simply adds a quick check on this in the css grid highlighter before attempting to highlight a grid fragment. MozReview-Commit-ID: GjsGu9hpU5G
devtools/client/inspector/test/browser.ini
devtools/client/inspector/test/browser_inspector_highlighter-cssgrid_02.js
devtools/server/actors/highlighters/css-grid.js
--- a/devtools/client/inspector/test/browser.ini
+++ b/devtools/client/inspector/test/browser.ini
@@ -67,16 +67,17 @@ skip-if = os == "mac" # Full keyboard na
 [browser_inspector_highlighter-01.js]
 [browser_inspector_highlighter-02.js]
 [browser_inspector_highlighter-03.js]
 [browser_inspector_highlighter-04.js]
 [browser_inspector_highlighter-by-type.js]
 [browser_inspector_highlighter-cancel.js]
 [browser_inspector_highlighter-comments.js]
 [browser_inspector_highlighter-cssgrid_01.js]
+[browser_inspector_highlighter-cssgrid_02.js]
 [browser_inspector_highlighter-csstransform_01.js]
 [browser_inspector_highlighter-csstransform_02.js]
 [browser_inspector_highlighter-embed.js]
 [browser_inspector_highlighter-eyedropper-clipboard.js]
 subsuite = clipboard
 skip-if = (os == 'linux' && bits == 32 && debug) # bug 1328915, disable linux32 debug devtools for timeouts
 [browser_inspector_highlighter-eyedropper-csp.js]
 [browser_inspector_highlighter-eyedropper-events.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/test/browser_inspector_highlighter-cssgrid_02.js
@@ -0,0 +1,44 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+// Test that grid layouts without items don't cause grid highlighter errors.
+
+const TEST_URL = `
+  <style type='text/css'>
+    .grid {
+      display: grid;
+      grid-template-columns: 20px 20px;
+      grid-gap: 15px;
+    }
+  </style>
+  <div class="grid"></div>
+`;
+
+const HIGHLIGHTER_TYPE = "CssGridHighlighter";
+
+add_task(function* () {
+  let {inspector, testActor} = yield openInspectorForURL(
+    "data:text/html;charset=utf-8," + encodeURIComponent(TEST_URL));
+  let front = inspector.inspector;
+  let highlighter = yield front.getHighlighterByType(HIGHLIGHTER_TYPE);
+
+  info("Try to show the highlighter on the grid container");
+  let node = yield getNodeFront(".grid", inspector);
+  yield highlighter.show(node);
+
+  let hidden = yield testActor.getHighlighterNodeAttribute(
+    "css-grid-canvas", "hidden", highlighter);
+  ok(!hidden, "The highlighter is visible");
+
+  info("Hiding the highlighter");
+  yield highlighter.hide();
+
+  hidden = yield testActor.getHighlighterNodeAttribute(
+    "css-grid-canvas", "hidden", highlighter);
+  ok(hidden, "The highlighter is hidden");
+
+  yield highlighter.finalize();
+});
--- a/devtools/server/actors/highlighters/css-grid.js
+++ b/devtools/server/actors/highlighters/css-grid.js
@@ -691,16 +691,28 @@ CssGridHighlighter.prototype = extend(Au
    *
    * @return  {Boolean} true if the current node has a CSS grid layout, false otherwise.
    */
   isGrid() {
     return this.currentNode.getGridFragments().length > 0;
   },
 
   /**
+   * Is a given grid fragment valid? i.e. does it actually have tracks? In some cases, we
+   * may have a fragment that defines column tracks but doesn't have any rows (or vice
+   * versa). In which case we do not want to draw anything for that fragment.
+   *
+   * @param {Object} fragment
+   * @return {Boolean}
+   */
+  isValidFragment(fragment) {
+    return fragment.cols.tracks.length && fragment.rows.tracks.length;
+  },
+
+  /**
    * The AutoRefreshHighlighter's _hasMoved method returns true only if the
    * element's quads have changed. Override it so it also returns true if the
    * element's grid has changed (which can happen when you change the
    * grid-template-* CSS properties with the highlighter displayed).
    */
   _hasMoved() {
     let hasMoved = AutoRefreshHighlighter.prototype._hasMoved.call(this);
 
@@ -735,18 +747,17 @@ CssGridHighlighter.prototype = extend(Au
     this.clearGridAreas();
     this.clearGridCell();
 
     // Update the current matrix used in our canvas' rendering
     this.updateCurrentMatrix();
 
     // Start drawing the grid fragments.
     for (let i = 0; i < this.gridData.length; i++) {
-      let fragment = this.gridData[i];
-      this.renderFragment(fragment);
+      this.renderFragment(this.gridData[i]);
     }
 
     // Display the grid area highlights if needed.
     if (this.options.showAllGridAreas) {
       this.showAllGridAreas();
     } else if (this.options.showGridArea) {
       this.showGridArea(this.options.showGridArea);
     }
@@ -1025,16 +1036,20 @@ CssGridHighlighter.prototype = extend(Au
       trackIndex--;
     }
 
     // The grid line index is the grid track index + 1.
     return trackIndex + 1;
   },
 
   renderFragment(fragment) {
+    if (!this.isValidFragment(fragment)) {
+      return;
+    }
+
     this.renderLines(fragment.cols, COLUMNS, "left", "top", "height",
                      this.getFirstRowLinePos(fragment),
                      this.getLastRowLinePos(fragment));
     this.renderLines(fragment.rows, ROWS, "top", "left", "width",
                      this.getFirstColLinePos(fragment),
                      this.getLastColLinePos(fragment));
 
     // Line numbers are rendered in a 2nd step to avoid overlapping with existing lines.