Bug 1359759 - 2 - Clear React warnings in GridOutline and prevent re-renders; r=gl draft
authorPatrick Brosset <pbrosset@mozilla.com>
Wed, 10 May 2017 16:07:12 +0200
changeset 575521 473ca1e1777d4e14f4f45a0b306b8153775cb1e6
parent 575520 d6dcdd07aef58e3077a4ad1dd8b17bbcae1c6505
child 575522 950c2945f3fb53bf7e89d25d77df9e50a3d15b85
push id58080
push userbmo:pbrosset@mozilla.com
push dateWed, 10 May 2017 15:09:21 +0000
reviewersgl
bugs1359759
milestone55.0a1
Bug 1359759 - 2 - Clear React warnings in GridOutline and prevent re-renders; r=gl Made the GridOutline component work with only 1 grid at a time. It already did, but in a not so obvious way. Removed the setState that happened during the render call to avoid React warnings. Cleaned up various data attribute to use the dataset property instead. Removed the mouseover/out that controled the background color of the highlighted cells. This now happens in CSS :hover, using currentColor. Avoided React warnings related to missing "key" props. Made changes to grid-inspector to avoid loops of re-renders when the highlighter would highlight a cell on hover. The component would wait for highlighter's events to dispatch store actions. Instead, we dispatch them first, then when the events come, we check if things have really changed! This way, the events will still have effect if they come from the rule-view for instance, but not if they come from the grid outline itself. MozReview-Commit-ID: 6LK8B1P8iMU
devtools/client/inspector/grids/components/GridOutline.js
devtools/client/inspector/grids/grid-inspector.js
devtools/client/themes/layout.css
--- a/devtools/client/inspector/grids/components/GridOutline.js
+++ b/devtools/client/inspector/grids/components/GridOutline.js
@@ -36,39 +36,66 @@ module.exports = createClass({
     onShowGridCellHighlight: PropTypes.func.isRequired,
     onShowGridLineNamesHighlight: PropTypes.func.isRequired,
   },
 
   mixins: [ addons.PureRenderMixin ],
 
   getInitialState() {
     return {
-      selectedGrids: [],
+      selectedGrid: null,
       height: 0,
       width: 0,
     };
   },
 
   componentWillMount() {
     // Throttle the grid highlighting of grid cells. It makes the UX smoother by not
     // lagging the grid cell highlighting if a lot of grid cells are mouseover in a
     // quick succession.
     this.highlightCell = throttle(this.highlightCell, GRID_CELL_MOUSEOVER_TIMEOUT);
   },
 
   componentWillReceiveProps({ grids }) {
-    if (this.state.selectedGrids.length < 2) {
-      this.setState({
-        height: 0,
-        width: 0,
-      });
+    let selectedGrid = grids.find(grid => grid.highlighted);
+
+    // Store the height of the grid container in the component state to prevent overflow
+    // issues. We want to store the width of the grid container as well so that the
+    // viewbox is only the calculated width of the grid outline.
+    let { width, height } = selectedGrid
+                            ? this.getTotalWidthAndHeight(selectedGrid)
+                            : { width: 0, height: 0 };
+
+    this.setState({ height, width, selectedGrid });
+  },
+
+  /**
+   * Get the width and height of a given grid.
+   *
+   * @param  {Object} grid
+   *         A single grid container in the document.
+   * @return {Object} An object like { width, height }
+   */
+  getTotalWidthAndHeight(grid) {
+    // TODO: We are drawing the first fragment since only one is currently being stored.
+    // In the future we will need to iterate over all fragments of a grid.
+    const { gridFragments } = grid;
+    const { rows, cols } = gridFragments[0];
+
+    let height = 0;
+    for (let i = 0; i < rows.lines.length - 1; i++) {
+      height += GRID_CELL_SCALE_FACTOR * (rows.tracks[i].breadth / 100);
     }
-    this.setState({
-      selectedGrids: grids.filter(grid => grid.highlighted),
-    });
+
+    let width = 0;
+    for (let i = 0; i < cols.lines.length - 1; i++) {
+      width += GRID_CELL_SCALE_FACTOR * (cols.tracks[i].breadth / 100);
+    }
+
+    return { width, height };
   },
 
   /**
    * Returns the grid area name if the given grid cell is part of a grid area, otherwise
    * null.
    *
    * @param  {Number} columnNumber
    *         The column number of the grid cell.
@@ -109,24 +136,22 @@ module.exports = createClass({
   },
 
   highlightCell({ target }) {
     const {
       grids,
       onShowGridAreaHighlight,
       onShowGridCellHighlight,
     } = this.props;
-    const name = target.getAttribute("data-grid-area-name");
-    const id = target.getAttribute("data-grid-id");
-    const fragmentIndex = target.getAttribute("data-grid-fragment-index");
-    const color = target.getAttribute("stroke");
-    const rowNumber = target.getAttribute("data-grid-row");
-    const columnNumber = target.getAttribute("data-grid-column");
-
-    target.setAttribute("fill", color);
+    const name = target.dataset.gridAreaName;
+    const id = target.dataset.gridId;
+    const fragmentIndex = target.dataset.gridFragmentIndex;
+    const color = target.closest(".grid-cell-group").dataset.gridLineColor;
+    const rowNumber = target.dataset.gridRow;
+    const columnNumber = target.dataset.gridColumn;
 
     if (name) {
       onShowGridAreaHighlight(grids[id].nodeFront, name, color);
     }
 
     if (fragmentIndex && rowNumber && columnNumber) {
       onShowGridCellHighlight(grids[id].nodeFront, color, fragmentIndex,
         rowNumber, columnNumber);
@@ -135,68 +160,53 @@ module.exports = createClass({
 
   /**
     * Renders the grid outline for the given grid container object.
     *
     * @param  {Object} grid
     *         A single grid container in the document.
     */
   renderGrid(grid) {
-    const { id, color, gridFragments } = grid;
     // TODO: We are drawing the first fragment since only one is currently being stored.
     // In the future we will need to iterate over all fragments of a grid.
     let gridFragmentIndex = 0;
+    const { id, color, gridFragments } = grid;
     const { rows, cols, areas } = gridFragments[gridFragmentIndex];
+
     const numberOfColumns = cols.lines.length - 1;
     const numberOfRows = rows.lines.length - 1;
     const rectangles = [];
     let x = 1;
     let y = 1;
     let width = 0;
     let height = 0;
-    // The grid outline border height/width is the total height/width of grid cells drawn.
-    let totalHeight = 0;
-    let totalWidth = 0;
 
-      // Draw the cells contained within the grid outline border.
+    // Draw the cells contained within the grid outline border.
     for (let rowNumber = 1; rowNumber <= numberOfRows; rowNumber++) {
       height = GRID_CELL_SCALE_FACTOR * (rows.tracks[rowNumber - 1].breadth / 100);
+
       for (let columnNumber = 1; columnNumber <= numberOfColumns; columnNumber++) {
         width = GRID_CELL_SCALE_FACTOR * (cols.tracks[columnNumber - 1].breadth / 100);
 
         const gridAreaName = this.getGridAreaName(columnNumber, rowNumber, areas);
         const gridCell = this.renderGridCell(id, gridFragmentIndex, x, y,
-          rowNumber, columnNumber, color, gridAreaName, width, height);
+                                             rowNumber, columnNumber, color, gridAreaName,
+                                             width, height);
 
         rectangles.push(gridCell);
         x += width;
       }
 
       x = 1;
       y += height;
-      totalHeight += height;
-    }
-
-    // Find the total width of the grid container so we can draw the border for it
-    for (let columnNumber = 0; columnNumber < numberOfColumns; columnNumber++) {
-      totalWidth += GRID_CELL_SCALE_FACTOR * (cols.tracks[columnNumber].breadth / 100);
-    }
-
-    // Store the height of the grid container in the component state to prevent overflow
-    // issues. We want to store the width of the grid container as well so that the
-    // viewbox is only the calculated width of the grid outline.
-    if (totalHeight > this.state.height || totalWidth > this.state.width) {
-      this.setState({
-        height: totalHeight + 20,
-        width: totalWidth,
-      });
     }
 
     // Draw a rectangle that acts as the grid outline border.
-    const border = this.renderGridOutlineBorder(totalWidth, totalHeight, color);
+    const border = this.renderGridOutlineBorder(this.state.width, this.state.height,
+                                                color);
     rectangles.unshift(border);
 
     return rectangles;
   },
 
   /**
    * Renders the grid cell of a grid fragment.
    *
@@ -218,52 +228,56 @@ module.exports = createClass({
    *         The width of grid cell.
    * @param  {Number} height
    *         The height of the grid cell.
    */
   renderGridCell(id, gridFragmentIndex, x, y, rowNumber, columnNumber, color,
     gridAreaName, width, height) {
     return dom.rect(
       {
-        className: "grid-outline-cell",
+        "key": `${id}-${rowNumber}-${columnNumber}`,
+        "className": "grid-outline-cell",
         "data-grid-area-name": gridAreaName,
         "data-grid-fragment-index": gridFragmentIndex,
         "data-grid-id": id,
         "data-grid-row": rowNumber,
         "data-grid-column": columnNumber,
         x,
         y,
         width,
         height,
         fill: "none",
-        stroke: color,
         onMouseOver: this.onMouseOverCell,
         onMouseOut: this.onMouseLeaveCell,
       }
     );
   },
 
-  renderGridOutline(grids) {
+  renderGridOutline(grid) {
+    let { color } = grid;
+
     return dom.g(
       {
-        className: "grid-cell-group",
+        "className": "grid-cell-group",
+        "data-grid-line-color": color,
+        "style": { color }
       },
-      grids.map(grid => this.renderGrid(grid))
+      this.renderGrid(grid)
     );
   },
 
   renderGridOutlineBorder(borderWidth, borderHeight, color) {
     return dom.rect(
       {
+        key: "border",
         className: "grid-outline-border",
         x: 1,
         y: 1,
         width: borderWidth,
-        height: borderHeight,
-        stroke: color,
+        height: borderHeight
       }
     );
   },
 
   /**
  * Renders the grid line of a grid fragment.
    *
    * @param  {Number} id
@@ -284,16 +298,17 @@ module.exports = createClass({
    *         The grid line number of the line being rendered.
    * @param  {String} lineType
    *         The grid line name(s) of the line being rendered.
    */
   renderGridLine(id, gridFragmentIndex, color, x1, y1, x2, y2,
     gridLineNumber, lineType) {
     return dom.line(
       {
+        key: `${id}-${lineType}-${gridLineNumber}`,
         className: "grid-outline-line",
         "data-grid-fragment-index": gridFragmentIndex,
         "data-grid-id": id,
         "data-grid-line-color": color,
         "data-grid-line-number": gridLineNumber,
         "data-grid-line-type": lineType,
         x1,
         y1,
@@ -301,22 +316,22 @@ module.exports = createClass({
         y2,
         onMouseOver: this.onMouseOverLine,
         onMouseOut: this.onMouseLeaveLine,
         stroke: "#000000",
       }
     );
   },
 
-  renderGridLines(grids) {
+  renderGridLines(grid) {
     return dom.g(
       {
         className: "grid-outline-lines",
       },
-      grids.map(grid => this.renderLines(grid))
+      this.renderLines(grid)
     );
   },
 
   renderLines(grid) {
     const { id, color, gridFragments } = grid;
     const { width, height } = this.state;
     let gridFragmentIndex = 0;
     const { rows, cols } = gridFragments[gridFragmentIndex];
@@ -361,62 +376,60 @@ module.exports = createClass({
   },
 
   onMouseLeaveCell({ target }) {
     const {
       grids,
       onShowGridAreaHighlight,
       onShowGridCellHighlight,
     } = this.props;
-    const id = target.getAttribute("data-grid-id");
-    const color = target.getAttribute("stroke");
-
-    target.setAttribute("fill", "none");
+    const id = target.dataset.gridId;
+    const color = target.closest(".grid-cell-group").dataset.gridLineColor;
 
     onShowGridAreaHighlight(grids[id].nodeFront, null, color);
     onShowGridCellHighlight(grids[id].nodeFront, color);
   },
 
   onMouseOverCell(event) {
     event.persist();
     this.highlightCell(event);
   },
 
   onMouseLeaveLine({ target }) {
     const { grids, onShowGridLineNamesHighlight } = this.props;
-    const fragmentIndex = target.getAttribute("data-grid-fragment-index");
-    const id = target.getAttribute("data-grid-id");
-    const color = target.getAttribute("data-grid-line-color");
+    const fragmentIndex = target.dataset.gridFragmentIndex;
+    const id = target.dataset.gridId;
+    const color = target.closest(".grid-cell-group").dataset.gridLineColor;
 
     onShowGridLineNamesHighlight(grids[id].nodeFront, fragmentIndex, color);
   },
 
   onMouseOverLine({ target }) {
     const { grids, onShowGridLineNamesHighlight } = this.props;
-    const fragmentIndex = target.getAttribute("data-grid-fragment-index");
-    const id = target.getAttribute("data-grid-id");
-    const lineNumber = target.getAttribute("data-grid-line-number");
-    const type = target.getAttribute("data-grid-line-type");
-    const color = target.getAttribute("data-grid-line-color");
+    const fragmentIndex = target.dataset.gridFragmentIndex;
+    const id = target.dataset.gridId;
+    const lineNumber = target.dataset.gridLineNumber;
+    const type = target.dataset.gridLineType;
+    const color = target.closest(".grid-cell-group").dataset.gridLineColor;
 
     onShowGridLineNamesHighlight(grids[id].nodeFront, fragmentIndex, color,
       lineNumber, type);
   },
 
   render() {
-    const { selectedGrids, height, width } = this.state;
+    const { selectedGrid, height, width } = this.state;
 
-    return selectedGrids.length ?
+    return selectedGrid ?
       dom.svg(
         {
           className: "grid-outline",
           width: "100%",
           height: this.getHeight(),
           viewBox: `${TRANSLATE_X} ${TRANSLATE_Y} ${width} ${height}`,
         },
-        this.renderGridOutline(selectedGrids),
-        this.renderGridLines(selectedGrids)
+        this.renderGridOutline(selectedGrid),
+        this.renderGridLines(selectedGrid)
       )
       :
       null;
   },
 
 });
--- a/devtools/client/inspector/grids/grid-inspector.js
+++ b/devtools/client/inspector/grids/grid-inspector.js
@@ -217,16 +217,32 @@ GridInspector.prototype = {
 
     let showGridLineNumbers = Services.prefs.getBoolPref(SHOW_GRID_LINE_NUMBERS);
     let showInfinteLines = Services.prefs.getBoolPref(SHOW_INFINITE_LINES_PREF);
 
     dispatch(updateShowGridLineNumbers(showGridLineNumbers));
     dispatch(updateShowInfiniteLines(showInfinteLines));
   },
 
+  showGridHighlighter(node, settings) {
+    this.lastHighlighterColor = settings.color;
+    this.lastHighlighterNode = node;
+    this.lastHighlighterState = true;
+
+    this.highlighters.showGridHighlighter(node, settings);
+  },
+
+  toggleGridHighlighter(node, settings) {
+    this.lastHighlighterColor = settings.color;
+    this.lastHighlighterNode = node;
+    this.lastHighlighterState = node !== this.highlighters.gridHighlighterShown;
+
+    this.highlighters.toggleGridHighlighter(node, settings);
+  },
+
   /**
    * Updates the grid panel by dispatching the new grid data. This is called when the
    * layout view becomes visible or the view needs to be updated with new grid data.
    *
    * @param  {Array|null} gridFronts
    *         Optional array of all GridFront in the current page.
    */
   updateGridPanel: Task.async(function* (gridFronts) {
@@ -282,18 +298,32 @@ GridInspector.prototype = {
    *         The NodeFront of the grid container element for which the grid highlighter
    *         is shown for.
    * @param  {Object} options
    *         The highlighter options used for the highlighter being shown/hidden.
    */
   onHighlighterChange(event, nodeFront, options) {
     let highlighted = event === "grid-highlighter-shown";
     let { color } = options;
-    this.store.dispatch(updateGridHighlighted(nodeFront, highlighted));
-    this.store.dispatch(updateGridColor(nodeFront, color));
+
+    // Only tell the store that the highlighter changed if it did change.
+    // If we're still highlighting the same node, with the same color, no need to force
+    // a refresh.
+    if (this.lastHighlighterState !== highlighted ||
+        this.lastHighlighterNode !== nodeFront) {
+      this.store.dispatch(updateGridHighlighted(nodeFront, highlighted));
+    }
+
+    if (this.lastHighlighterColor !== color || this.lastHighlighterNode !== nodeFront) {
+      this.store.dispatch(updateGridColor(nodeFront, color));
+    }
+
+    this.lastHighlighterColor = null;
+    this.lastHighlighterNode = null;
+    this.lastHighlighterState = null;
   },
 
   /**
    * Handler for the "markupmutation" event fired by the inspector. On markup mutations,
    * update the grid panel content.
    */
   onMarkupMutation() {
     this.updateGridPanel();
@@ -320,17 +350,17 @@ GridInspector.prototype = {
     this.store.dispatch(updateGridColor(node, color));
     let { grids } = this.store.getState();
 
     // If the grid for which the color was updated currently has a highlighter, update
     // the color.
     for (let grid of grids) {
       if (grid.nodeFront === node && grid.highlighted) {
         let highlighterSettings = this.getGridHighlighterSettings(node);
-        this.highlighters.showGridHighlighter(node, highlighterSettings);
+        this.showGridHighlighter(node, highlighterSettings);
       }
     }
   },
 
   /**
    * Highlights the grid area in the CSS Grid Highlighter for the given grid.
    *
    * @param  {NodeFront} node
@@ -344,17 +374,19 @@ GridInspector.prototype = {
    *         is highlighted for.
    */
   onShowGridAreaHighlight(node, gridAreaName, color) {
     let { highlighterSettings } = this.store.getState();
 
     highlighterSettings.showGridArea = gridAreaName;
     highlighterSettings.color = color;
 
-    this.highlighters.showGridHighlighter(node, highlighterSettings);
+    this.showGridHighlighter(node, highlighterSettings);
+
+    this.store.dispatch(updateGridHighlighted(node, true));
   },
 
   /**
    * Highlights the grid cell in the CSS Grid Highlighter for the given grid.
    *
    * @param  {NodeFront} node
    *         The NodeFront of the grid container element for which the grid
    *         highlighter is highlighted for.
@@ -372,17 +404,19 @@ GridInspector.prototype = {
    *         is highlighted for.
    */
   onShowGridCellHighlight(node, color, gridFragmentIndex, rowNumber, columnNumber) {
     let { highlighterSettings } = this.store.getState();
 
     highlighterSettings.showGridCell = { gridFragmentIndex, rowNumber, columnNumber };
     highlighterSettings.color = color;
 
-    this.highlighters.showGridHighlighter(node, highlighterSettings);
+    this.showGridHighlighter(node, highlighterSettings);
+
+    this.store.dispatch(updateGridHighlighted(node, true));
   },
 
   /**
    * Highlights the grid line in the CSS Grid Highlighter for the given grid.
    *
    * @param  {NodeFront} node
    *         The NodeFront of the grid container element for which the grid
    *         highlighter is highlighted for.
@@ -401,20 +435,21 @@ GridInspector.prototype = {
   onShowGridLineNamesHighlight(node, gridFragmentIndex, color, lineNumber, type) {
     let { highlighterSettings } = this.store.getState();
 
     highlighterSettings.showGridLineNames = {
       gridFragmentIndex,
       lineNumber,
       type
     };
-
     highlighterSettings.color = color;
 
-    this.highlighters.showGridHighlighter(node, highlighterSettings);
+    this.showGridHighlighter(node, highlighterSettings);
+
+    this.store.dispatch(updateGridHighlighted(node, true));
   },
 
   /**
    * Handler for the inspector sidebar select event. Starts listening for
    * "grid-layout-changed" if the layout panel is visible. Otherwise, stop
    * listening for grid layout changes. Finally, refresh the layout view if
    * it is visible.
    */
@@ -435,17 +470,20 @@ GridInspector.prototype = {
    * Toggles on/off the grid highlighter for the provided grid container element.
    *
    * @param  {NodeFront} node
    *         The NodeFront of the grid container element for which the grid
    *         highlighter is toggled on/off for.
    */
   onToggleGridHighlighter(node) {
     let highlighterSettings = this.getGridHighlighterSettings(node);
-    this.highlighters.toggleGridHighlighter(node, highlighterSettings);
+    this.toggleGridHighlighter(node, highlighterSettings);
+
+    this.store.dispatch(updateGridHighlighted(node,
+      node !== this.highlighters.gridHighlighterShown));
   },
 
   /**
    * Handler for a change in the show grid line numbers checkbox in the
    * GridDisplaySettings component. Toggles on/off the option to show the grid line
    * numbers in the grid highlighter. Refreshes the shown grid highlighter for the
    * grids currently highlighted.
    *
@@ -456,17 +494,17 @@ GridInspector.prototype = {
     this.store.dispatch(updateShowGridLineNumbers(enabled));
     Services.prefs.setBoolPref(SHOW_GRID_LINE_NUMBERS, enabled);
 
     let { grids } = this.store.getState();
 
     for (let grid of grids) {
       if (grid.highlighted) {
         let highlighterSettings = this.getGridHighlighterSettings(grid.nodeFront);
-        this.highlighters.showGridHighlighter(grid.nodeFront, highlighterSettings);
+        this.showGridHighlighter(grid.nodeFront, highlighterSettings);
       }
     }
   },
 
   /**
    * Handler for a change in the extend grid lines infinitely checkbox in the
    * GridDisplaySettings component. Toggles on/off the option to extend the grid
    * lines infinitely in the grid highlighter. Refreshes the shown grid highlighter
@@ -479,16 +517,16 @@ GridInspector.prototype = {
     this.store.dispatch(updateShowInfiniteLines(enabled));
     Services.prefs.setBoolPref(SHOW_INFINITE_LINES_PREF, enabled);
 
     let { grids } = this.store.getState();
 
     for (let grid of grids) {
       if (grid.highlighted) {
         let highlighterSettings = this.getGridHighlighterSettings(grid.nodeFront);
-        this.highlighters.showGridHighlighter(grid.nodeFront, highlighterSettings);
+        this.showGridHighlighter(grid.nodeFront, highlighterSettings);
       }
     }
   },
 
 };
 
 module.exports = GridInspector;
--- a/devtools/client/themes/layout.css
+++ b/devtools/client/themes/layout.css
@@ -87,28 +87,31 @@
  */
 
 #grid-outline {
   margin: 5px auto;
 }
 
 .grid-outline-border {
   fill: none;
+  stroke: currentColor;
   stroke-width: 0.75;
   vector-effect: non-scaling-stroke;
 }
 
 .grid-outline-cell {
   pointer-events: all;
+  stroke: currentColor;
   stroke-dasharray: 0.5, 2;
   vector-effect: non-scaling-stroke;
 }
 
 .grid-outline-cell:hover {
   opacity: 0.45;
+  fill: currentColor;
 }
 
 .grid-outline-line {
   opacity: 0;
   stroke-width: 10;
 }
 
 /**