Bug 1510790 - Introduce Redux selector for tracked changes to return the nested tree structure; r=pbro

This patch introduces a Redux selector for the Changes slice. A selector here is a fancy word for a method that returns a subset from the state without having to expose the state's structure to the consumer. It is also useful for returning a derived version of the state which we're using here.

The tracked changed CSS rules in the Redux state are not stored in a nested structure. A shallow structure is easier to query and to work with in our use case. But the Changes panel needs to display CSS rules in a hierarchical structure (ex: a style rule which is a child of a @media rule). To do this, the React component used to build up the nested structure as it consumed the changes from the Redux state. To prevent rendering duplicates of rules (once as part of an ancestor's children and once as a standalone rule), the React component kept a reference of rules it had previously rendered. This had a flaw because it didn't account for the rule's stylesheet. The problem was that rules with identical selectors from different stylesheets would not all be rendered because they would be accidentally marked as previously rendered.

This is too much knowledge of the business logic for the React component anyway.

The Redux state should present itself in a way that's simple for the React component to consume. Hence the introduction of the `getChangesTree()` selector in this patch. This method allows us to present a comfortable structure to React while keeping the Redux structure comfortable for us to work with. This separation enables increased flexibility to restructure the Redux state without impacting the React components.

More about Redux selectors here: https://redux.js.org/recipes/computing-derived-data

Differential Revision: https://phabricator.services.mozilla.com/D16068

--HG--
rename : devtools/client/inspector/changes/moz.build => devtools/client/inspector/changes/selectors/moz.build
extra : moz-landing-system : lando

devtools/client/inspector/changes/components/ChangesApp.js

@@ -10,29 +10,20 @@ const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 const { connect } = require("devtools/client/shared/vendor/react-redux");
 
 const CSSDeclaration = createFactory(require("./CSSDeclaration"));
+const { getChangesTree } = require("../selectors/changes");
 const { getSourceForDisplay } = require("../utils/changes-utils");
 const { getStr } = require("../utils/l10n");
 
 class ChangesApp extends PureComponent {
   static get propTypes() {
     return {
-      // Redux state slice assigned to Track Changes feature; passed as prop by connect()
-      changes: PropTypes.object.isRequired,
+      // Nested CSS rule tree structure of CSS changes grouped by source (stylesheet)
+      changesTree: PropTypes.object.isRequired,
     };
   }
 
   constructor(props) {
     super(props);
-    // In the Redux store, all rules exist in a collection at the same level of nesting.
-    // Parent rules come before child rules. Parent/child dependencies are set
-    // via parameters in each rule pointing to the corresponding rule ids.
-    //
-    // To render rules, we traverse the descendant rule tree and render each child rule
-    // found. This means we get into situations where we can render the same rule multiple
-    // times: once as a child of its parent and once standalone.
-    //
-    // By keeping a log of rules previously rendered we prevent needless multi-rendering.
-    this.renderedRules = [];
   }
 
   renderDeclarations(remove = [], add = []) {
@@ -63,16 +54,9 @@ class ChangesApp extends PureComponent {
     return [removals, additions];
   }
 
-  renderRule(ruleId, rule, rules, level = 0) {
+  renderRule(ruleId, rule, level = 0) {
     const selector = rule.selector;
 
-    if (this.renderedRules.includes(ruleId)) {
-      return null;
-    }
-
-    // Mark this rule as rendered so we don't render it again.
-    this.renderedRules.push(ruleId);
-
     let diffClass = "";
     if (rule.changeType === "rule-add") {
       diffClass = "diff-add";
@@ -97,8 +81,8 @@ class ChangesApp extends PureComponent {
         dom.span({ className: "bracket-open" }, "{")
       ),
       // Render any nested child rules if they exist.
-      rule.children.map(childRuleId => {
-        return this.renderRule(childRuleId, rules[childRuleId], rules, level + 1);
+      rule.children.map(childRule => {
+        return this.renderRule(childRule.ruleId, childRule, level + 1);
       }),
       // Render any changed CSS declarations.
       this.renderDeclarations(rule.remove, rule.add),
@@ -127,7 +111,7 @@ class ChangesApp extends PureComponent {
         ),
         // Render changed rules within this source.
         Object.entries(rules).map(([ruleId, rule]) => {
-          return this.renderRule(ruleId, rule, rules);
+          return this.renderRule(ruleId, rule);
         })
       );
     });
@@ -151,19 +135,22 @@ class ChangesApp extends PureComponent {
   }
 
   render() {
-    // Reset log of rendered rules.
-    this.renderedRules = [];
-    const hasChanges = Object.keys(this.props.changes).length > 0;
-
+    const hasChanges = Object.keys(this.props.changesTree).length > 0;
     return dom.div(
       {
         className: "theme-sidebar inspector-tabpanel",
         id: "sidebar-panel-changes",
       },
       !hasChanges && this.renderEmptyState(),
-      hasChanges && this.renderDiff(this.props.changes)
+      hasChanges && this.renderDiff(this.props.changesTree)
     );
   }
 }
 
-module.exports = connect(state => state)(ChangesApp);
+const mapStateToProps = state => {
+  return {
+    changesTree: getChangesTree(state.changes),
+  };
+};
+
+module.exports = connect(mapStateToProps)(ChangesApp);

devtools/client/inspector/changes/moz.build

@@ -8,6 +8,7 @@ DIRS += [
     'actions',
     'components',
     'reducers',
+    'selectors',
     'utils',
 ]
 

devtools/client/inspector/changes/reducers/changes.js

@@ -86,7 +86,7 @@ function createRule(ruleData, rules) {
       const nextRuleId = array[index + 1];
 
       // Copy or create an entry for this rule.
-      const defaults = { selector, add: [], remove: [], children: [] };
+      const defaults = { selector, ruleId, add: [], remove: [], children: [] };
       rules[ruleId] = Object.assign(defaults, rules[ruleId]);
 
       // The next ruleId is lower in the rule tree, therefore it's a child of this rule.
@@ -133,6 +133,7 @@ function removeRule(ruleId, rules) {
  *      href: // {String|null} Stylesheet or document URL; null for inline stylesheets
  *      rules: {
  *        <ruleId>: {
+ *          ruleId:      // {String} <ruleId> of this rule
  *          selector:    // {String} CSS selector or CSS at-rule text
  *          changeType:  // {String} Optional; one of: "rule-add" or "rule-remove"
  *          children: [] // {Array} of <ruleId> for child rules of this rule

devtools/client/inspector/changes/selectors/changes.js

@@ -0,0 +1,79 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
+/* 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";
+
+/**
+ * In the Redux state, changed CSS rules are grouped by source (stylesheet) and stored in
+ * a single level array, regardless of nesting.
+ * This method returns a nested tree structure of the changed CSS rules so the React
+ * consumer components can traverse it easier when rendering the nested CSS rules view.
+ * Keeping this interface updated allows the Redux state structure to change without
+ * affecting the consumer components.
+ *
+ * @param {Object} state
+ *        Redux slice for tracked changes.
+ * @return {Object}
+ */
+function getChangesTree(state) {
+  /**
+   * Recursively replace a rule's array of child rule ids with the referenced child rules.
+   * Mark visited rules so as not to handle them (and their children) again.
+   *
+   * Returns the rule object with expanded children or null if previously visited.
+   *
+   * @param  {String} ruleId
+   * @param  {Object} rule
+   * @param  {Array} rules
+   * @param  {Set} visitedRules
+   * @return {Object|null}
+   */
+  function expandRuleChildren(ruleId, rule, rules, visitedRules) {
+    if (visitedRules.has(ruleId)) {
+      return null;
+    }
+
+    visitedRules.add(ruleId);
+
+    return {
+      ...rule,
+      children: rule.children.map(childRuleId =>
+          expandRuleChildren(childRuleId, rules[childRuleId], rules, visitedRules)),
+    };
+  }
+
+  return Object.entries(state).reduce((sourcesObj, [sourceId, source]) => {
+    const { rules } = source;
+    // Log of visited rules in this source. Helps avoid duplication when traversing the
+    // descendant rule tree. This Set is unique per source. It will be passed down to
+    // be populated with ids of rules once visited. This ensures that only visited rules
+    // unique to this source will be skipped and prevents skipping identical rules from
+    // other sources (ex: rules with the same selector and the same index).
+    const visitedRules = new Set();
+
+    // Build a new collection of sources keyed by source id.
+    sourcesObj[sourceId] = {
+      ...source,
+      // Build a new collection of rules keyed by rule id.
+      rules: Object.entries(rules).reduce((rulesObj, [ruleId, rule]) => {
+        // Expand the rule's array of child rule ids with the referenced child rules.
+        // Skip exposing null values which mean the rule was previously visited as part
+        // of an ancestor descendant tree.
+        const expandedRule = expandRuleChildren(ruleId, rule, rules, visitedRules);
+        if (expandedRule !== null) {
+          rulesObj[ruleId] = expandedRule;
+        }
+
+        return rulesObj;
+      }, {}),
+    };
+
+    return sourcesObj;
+  }, {});
+}
+
+module.exports = {
+  getChangesTree,
+};

devtools/client/inspector/changes/selectors/moz.build

@@ -0,0 +1,9 @@
+# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
+# vim: set filetype=python:
+# 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/.
+
+DevToolsModules(
+    'changes.js',
+)

devtools/client/inspector/changes/test/browser.ini

@@ -15,6 +15,7 @@ support-files =
 [browser_changes_declaration_disable.js]
 [browser_changes_declaration_duplicate.js]
 [browser_changes_declaration_edit_value.js]
+[browser_changes_declaration_identical_rules.js]
 [browser_changes_declaration_remove_ahead.js]
 [browser_changes_declaration_remove_disabled.js]
 [browser_changes_declaration_remove.js]

devtools/client/inspector/changes/test/browser_changes_declaration_identical_rules.js

@@ -0,0 +1,57 @@
+/* vim: set ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test tracking changes to CSS declarations in different stylesheets but in rules
+// with identical selectors.
+
+const TEST_URI = `
+  <style type='text/css'>
+    div {
+      color: red;
+    }
+  </style>
+  <style type='text/css'>
+    div {
+      font-size: 1em;
+    }
+  </style>
+  <div></div>
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const { inspector, view: ruleView } = await openRuleView();
+  const { document: doc, store } = selectChangesView(inspector);
+
+  await selectNode("div", inspector);
+  const rule1 = getRuleViewRuleEditor(ruleView, 1).rule;
+  const rule2 = getRuleViewRuleEditor(ruleView, 2).rule;
+  const prop1 = rule1.textProps[0];
+  const prop2 = rule2.textProps[0];
+  let onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Disable the declaration in the first rule");
+  await togglePropStatus(ruleView, prop1);
+  info("Wait for change to be tracked");
+  await onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Disable the declaration in the second rule");
+  await togglePropStatus(ruleView, prop2);
+  info("Wait for change to be tracked");
+  await onTrackChange;
+
+  const removeDecl = getRemovedDeclarations(doc);
+  is(removeDecl.length, 2, "Two declarations tracked as removed");
+  // The last of the two matching rules shows up first in Rule view given that the
+  // specificity is the same. This is correct. If the properties were the same, the latest
+  // declaration would overwrite the first and thus show up on top.
+  is(removeDecl[0].property, "font-size", "Correct property name for second declaration");
+  is(removeDecl[0].value, "1em", "Correct property value for second declaration");
+  is(removeDecl[1].property, "color", "Correct property name for first declaration");
+  is(removeDecl[1].value, "red", "Correct property value for first declaration");
+});