Bug 1510790 - Introduce Redux selector for tracked changes to return the nested tree structure; r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Fri, 11 Jan 2019 11:24:34 +0000
changeset 453450 ebee32dc7abedd8621353f4ee27af9cf077e427e
parent 453449 639afd87506132ba0a52b687c535a40791176787
child 453451 9a22edb5c335044baf5fd63f28553d5f6cfb4103
push id35357
push usernerli@mozilla.com
push dateFri, 11 Jan 2019 21:54:07 +0000
treeherdermozilla-central@0ce024c91511 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1510790
milestone66.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 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
devtools/client/inspector/changes/components/ChangesApp.js
devtools/client/inspector/changes/moz.build
devtools/client/inspector/changes/reducers/changes.js
devtools/client/inspector/changes/selectors/changes.js
devtools/client/inspector/changes/selectors/moz.build
devtools/client/inspector/changes/test/browser.ini
devtools/client/inspector/changes/test/browser_changes_declaration_identical_rules.js
--- a/devtools/client/inspector/changes/components/ChangesApp.js
+++ b/devtools/client/inspector/changes/components/ChangesApp.js
@@ -5,39 +5,30 @@
 "use strict";
 
 const { createFactory, PureComponent } = require("devtools/client/shared/vendor/react");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 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 = []) {
     const removals = remove
       // Sorting changed declarations in the order they appear in the Rules view.
       .sort((a, b) => a.index > b.index)
       .map(({property, value, index}) => {
         return CSSDeclaration({
@@ -58,26 +49,19 @@ class ChangesApp extends PureComponent {
           property,
           value,
         });
       });
 
     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";
     } else if (rule.changeType === "rule-remove") {
       diffClass = "diff-remove";
     }
 
     return dom.div(
@@ -92,18 +76,18 @@ class ChangesApp extends PureComponent {
         {
           className: `level selector ${diffClass}`,
           title: selector,
         },
         selector,
         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),
       dom.div({ className: `level bracket-close ${diffClass}` }, "}")
     );
   }
 
   renderDiff(changes = {}) {
@@ -122,17 +106,17 @@ class ChangesApp extends PureComponent {
             className: "href",
             title: href,
           },
           dom.span({}, path),
           isFramed && this.renderFrameBadge(href)
         ),
         // Render changed rules within this source.
         Object.entries(rules).map(([ruleId, rule]) => {
-          return this.renderRule(ruleId, rule, rules);
+          return this.renderRule(ruleId, rule);
         })
       );
     });
   }
 
   renderFrameBadge(href = "") {
     return dom.span(
       {
@@ -146,24 +130,27 @@ class ChangesApp extends PureComponent {
   renderEmptyState() {
     return dom.div({ className: "devtools-sidepanel-no-result" },
       dom.p({}, getStr("changes.noChanges")),
       dom.p({}, getStr("changes.noChangesDescription"))
     );
   }
 
   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);
--- a/devtools/client/inspector/changes/moz.build
+++ b/devtools/client/inspector/changes/moz.build
@@ -3,16 +3,17 @@
 # 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/.
 
 DIRS += [
     'actions',
     'components',
     'reducers',
+    'selectors',
     'utils',
 ]
 
 DevToolsModules(
     'ChangesView.js',
 )
 
 BROWSER_CHROME_MANIFESTS += ['test/browser.ini']
--- a/devtools/client/inspector/changes/reducers/changes.js
+++ b/devtools/client/inspector/changes/reducers/changes.js
@@ -81,17 +81,17 @@ function createRule(ruleData, rules) {
     })
     // Then, create new entries in the rules collection and assign dependencies.
     .map((ruleId, index, array) => {
       const { selector } = ruleAncestry[index];
       const prevRuleId = array[index - 1];
       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.
       if (nextRuleId && !rules[ruleId].children.includes(nextRuleId)) {
         rules[ruleId].children.push(nextRuleId);
       }
 
       // The previous ruleId is higher in the rule tree, therefore it's the parent.
@@ -128,16 +128,17 @@ function removeRule(ruleId, rules) {
  * which contain collections of added and removed CSS declarations.
  *
  * Structure:
  *    <sourceId>: {
  *      type: // {String} One of: "stylesheet", "inline" or "element"
  *      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
  *          parent:      // {String} <ruleId> of the parent rule
  *          add: [       // {Array} of objects with CSS declarations
  *            {
  *              property:    // {String} CSS property name
  *              value:       // {String} CSS property value
new file mode 100644
--- /dev/null
+++ b/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,
+};
copy from devtools/client/inspector/changes/moz.build
copy to devtools/client/inspector/changes/selectors/moz.build
--- a/devtools/client/inspector/changes/moz.build
+++ b/devtools/client/inspector/changes/selectors/moz.build
@@ -1,18 +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/.
 
-DIRS += [
-    'actions',
-    'components',
-    'reducers',
-    'utils',
-]
-
 DevToolsModules(
-    'ChangesView.js',
+    'changes.js',
 )
-
-BROWSER_CHROME_MANIFESTS += ['test/browser.ini']
--- a/devtools/client/inspector/changes/test/browser.ini
+++ b/devtools/client/inspector/changes/test/browser.ini
@@ -10,13 +10,14 @@ support-files =
   !/devtools/client/shared/test/shared-redux-head.js
   !/devtools/client/shared/test/telemetry-test-helpers.js
   !/devtools/client/shared/test/test-actor.js
   !/devtools/client/shared/test/test-actor-registry.js
 
 [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]
 [browser_changes_declaration_rename.js]
 [browser_changes_rule_selector.js]
new file mode 100644
--- /dev/null
+++ b/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");
+});