Bug 1503896 - (Part 5) Add change type to change metadata and mark whole rules as added/removed; r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Wed, 07 Nov 2018 10:00:31 +0000
changeset 444844 ecc6903c579bf13a3f79ee77c5700e5defc0ce6b
parent 444843 0a015306cf2b9d86f524036800ef95fc24bd335a
child 444845 10a65f18e697a58685832661d670242408b2a11a
push id72515
push userrcaliman@mozilla.com
push dateWed, 07 Nov 2018 11:29:00 +0000
treeherderautoland@10a65f18e697 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1503896
milestone65.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 1503896 - (Part 5) Add change type to change metadata and mark whole rules as added/removed; r=pbro Depends on D10586 Adds a new `type` param to the `change` object passed from server to the client to describe the change type. For changes to rules, the client marks the whole rule as either added or removed and styles it accordingly in the Changes panel. Change types for declarations are not used at this time, but are put in for consistency and future-proofing. Differential Revision: https://phabricator.services.mozilla.com/D11116
devtools/client/inspector/changes/components/ChangesApp.js
devtools/client/inspector/changes/reducers/changes.js
devtools/client/themes/changes.css
devtools/server/actors/styles.js
--- a/devtools/client/inspector/changes/components/ChangesApp.js
+++ b/devtools/client/inspector/changes/components/ChangesApp.js
@@ -60,36 +60,43 @@ class ChangesApp extends PureComponent {
 
     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(
       {
         key: ruleId,
         className: "rule",
       },
       dom.div(
         {
-          className: "level selector",
+          className: `level selector ${diffClass}`,
           title: selector,
         },
         selector,
         dom.span({ className: "bracket-open" }, "{")
       ),
       // Render any nested child rules if they are present.
       rule.children.length > 0 && rule.children.map(childRuleId => {
         return this.renderRule(childRuleId, rules[childRuleId], rules);
       }),
       // Render any changed CSS declarations.
       this.renderDeclarations(rule.remove, rule.add),
-      dom.span({ className: "level bracket-close" }, "}")
+      dom.div({ className: `level bracket-close ${diffClass}` }, "}")
     );
   }
 
   renderDiff(changes = {}) {
     // Render groups of style sources: stylesheets and element style attributes.
     return Object.entries(changes).map(([sourceId, source]) => {
       const href = source.href || `inline stylesheet #${source.index}`;
       const rules = source.rules;
--- a/devtools/client/inspector/changes/reducers/changes.js
+++ b/devtools/client/inspector/changes/reducers/changes.js
@@ -128,16 +128,17 @@ function removeRule(ruleId, rules) {
  *
  * Structure:
  *    <sourceId>: {
  *      type: // "stylesheet" or "element"
  *      href: // Stylesheet or document URL
  *      rules: {
  *        <ruleId>: {
  *          selector: "" // String CSS selector or CSS at-rule text
+ *          changeType:  // Optional string: "rule-add" or "rule-remove"
  *          children: [] // Array of <ruleId> for child rules of this rule.
  *          parent:      // <ruleId> of the parent rule
  *          add: {
  *            <property>: <value> // CSS declaration
  *            ...
  *          },
  *          remove: {
  *            <property>: <value> // CSS declaration
@@ -161,31 +162,34 @@ const reducers = {
       add: {},
       remove: {},
     };
 
     change = { ...defaults, ...change };
     state = cloneState(state);
 
     const { type, href, index } = change.source;
-    const { selector, ancestors, ruleIndex } = change;
+    const { selector, ancestors, ruleIndex, type: changeType } = change;
     const sourceId = getSourceHash(change.source);
     const ruleId = getRuleHash({ selector, ancestors, ruleIndex });
     // Type coerce to boolean: `false` if value is undefined or `null`, `true` otherwise.
     const hasAdd = !!change.add;
     const hasRemove = !!change.remove;
 
     // Copy or create object identifying the source (styelsheet/element) for this change.
     const source = Object.assign({}, state[sourceId], { type, href, index });
     // Copy or create collection of all rules ever changed in this source.
     const rules = Object.assign({}, source.rules);
     // Refrence or create object identifying the rule for this change.
     let rule = rules[ruleId];
     if (!rule) {
       rule = createRule({ selector, ancestors, ruleIndex }, rules);
+      if (changeType.startsWith("rule-")) {
+        rule.changeType = changeType;
+      }
     }
     // Copy or create collection of all CSS declarations ever added to this rule.
     const add = Object.assign({}, rule.add);
     // Copy or create collection of all CSS declarations ever removed from this rule.
     const remove = Object.assign({}, rule.remove);
 
     if (hasRemove) {
       Object.entries(change.remove).forEach(([property, value]) => {
--- a/devtools/client/themes/changes.css
+++ b/devtools/client/themes/changes.css
@@ -49,18 +49,18 @@
 #sidebar-panel-changes .rule > .rule > .rule .level {
   padding-left: calc(var(--diff-level-offset) * 3);
 }
 
 #sidebar-panel-changes .rule > .rule > .rule > .rule .level {
   padding-left: calc(var(--diff-level-offset) * 4);
 }
 
-#sidebar-panel-changes .rule .selector,
-#sidebar-panel-changes .rule .bracket-close {
+#sidebar-panel-changes .rule .selector:not(.diff-remove):not(.diff-add),
+#sidebar-panel-changes .rule .bracket-close:not(.diff-remove):not(.diff-add) {
   margin-left: calc(-1 * var(--diff-level-offset) + 5px);
 }
 
 #sidebar-panel-changes .rule .bracket-open {
   display: inline-block;
   margin-left: 5px;
 }
 
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -1568,16 +1568,17 @@ var StyleRuleActor = protocol.ActorClass
     const prevDisabled = !!commentOffsets;
     // Append the "!important" string if defined in the previous priority flag.
     prevValue = (prevValue && prevPriority) ? `${prevValue} !important` : prevValue;
 
     const data = this.metadata;
 
     switch (change.type) {
       case "set":
+        data.type = prevValue ? "declaration-add" : "declaration-update";
         // If `change.newName` is defined, use it because the property is being renamed.
         // Otherwise, a new declaration is being created or the value of an existing
         // declaration is being updated. In that case, use the provided `change.name`.
         const name = change.newName ? change.newName : change.name;
         // Append the "!important" string if defined in the incoming priority flag.
         const newValue = change.priority ? `${change.value} !important` : change.value;
         // Reuse the previous value string, when the property is renamed.
         // Otherwise, use the incoming value string.
@@ -1595,16 +1596,17 @@ var StyleRuleActor = protocol.ActorClass
         // have no context of "disabled", only "add" or remove, like diffs.
         if (prevDisabled && !change.newName && prevValue === newValue) {
           data.remove = null;
         }
 
         break;
 
       case "remove":
+        data.type = "declaration-remove";
         data.add = null;
         data.remove = { [change.name]: prevValue };
         break;
     }
 
     TrackChangeEmitter.trackChange(data);
   },
 
@@ -1625,23 +1627,25 @@ var StyleRuleActor = protocol.ActorClass
       return acc;
     }, {});
 
     // Logging two distinct operations to remove the old rule and add a new one.
     // TODO: Make TrackChangeEmitter support transactions so these two operations are
     // grouped together when implementing undo/redo.
     TrackChangeEmitter.trackChange({
       ...this.metadata,
+      type: "rule-remove",
       add: null,
       remove: declarations,
       selector: oldSelector,
     });
 
     TrackChangeEmitter.trackChange({
       ...this.metadata,
+      type: "rule-add",
       add: declarations,
       remove: null,
       selector: newSelector,
     });
   },
 
   /**
    * Calls modifySelector2() which needs to be kept around for backwards compatibility.