Bug 1499049 - (Part 7) Fix issues with toggled and renamed CSS declarations r=pbro;
authorRazvan Caliman <rcaliman@mozilla.com>
Thu, 25 Oct 2018 11:07:29 +0000
changeset 443107 c4f23d37432644acf51200bb418a5029e66b0a68
parent 443106 f7565ed1376dc6cc9ffa6b9f054a9a4aeaa2f480
child 443108 629deef4613c09a0f081fc804507355b250aab77
push id34939
push usernerli@mozilla.com
push dateFri, 26 Oct 2018 15:47:55 +0000
treeherdermozilla-central@760a16bf0d2b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1499049
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 1499049 - (Part 7) Fix issues with toggled and renamed CSS declarations r=pbro; Depends on D8967 This patch fixes an issue where toggling a new CSS declaration OFF then back ON would make the Changes panel lose track of its existence. This happened because tracked changes don't have context about "disabled" state, only "add" or "remove", like diffs. Additionally, this patch fixes the case where renaming a property would erroneously track two distinct operations (rename + add). To fix this, we prevent the inline editor for the property name from the CSS Rules view to advance and focus the property value, which then immediately blurred triggering a fake second operation to be tracked. Auto-advancing to the property value inline editor still works if adding a new declaration or if the property name ends with a colon, ":", therefore old behaviour is not lost. MozReview-Commit-ID: Faw2DeCJJYk Differential Revision: https://phabricator.services.mozilla.com/D9257
devtools/server/actors/styles.js
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -1484,25 +1484,27 @@ var StyleRuleActor = protocol.ActorClass
   /**
    * Take an object with instructions to modify a CSS declaration and emit a
    * "track-change" event with normalized metadata which describes the change.
    *
    * @param {Object} change
    *        Data about a modification to a rule. @see |modifyProperties()|
    */
   logChange(change) {
-    let prevValue = this._declarations[change.index]
-      ? this._declarations[change.index].value
-      : null;
-    const prevName = this._declarations[change.index]
-      ? this._declarations[change.index].name
-      : null;
-    const prevPriority = this._declarations[change.index]
-      ? this._declarations[change.index].priority
-      : null;
+    // Destructure properties from the previous CSS declaration at this index, if any,
+    // to new variable names to indicate the previous state.
+    let {
+      value: prevValue,
+      name: prevName,
+      priority: prevPriority,
+      commentOffsets,
+    } = this._declarations[change.index] || {};
+    // A declaration is disabled if it has a `commentOffsets` array.
+    // Here we type coerce the value to a boolean with double-bang (!!)
+    const prevDisabled = !!commentOffsets;
     // Append the "!important" string if defined in the previous priority flag.
     prevValue = (prevValue && prevPriority) ? `${prevValue} !important` : prevValue;
 
     // Metadata about a change.
     const data = {};
     // Collect information about the rule's ancestors (@media, @supports, @keyframes).
     // Used to show context for this change in the UI and to match the rule for undo/redo.
     data.ancestors = this.ancestorRules.map(rule => {
@@ -1564,16 +1566,25 @@ var StyleRuleActor = protocol.ActorClass
         // Otherwise, use the incoming value string.
         const value = change.newName ? prevValue : newValue;
 
         data.add = { property: name, value };
         // If there is a previous value, log its removal together with the previous
         // property name. Using the previous name handles the case for renaming a property
         // and is harmless when updating an existing value (the name stays the same).
         data.remove = prevValue ? { property: prevName, value: prevValue } : null;
+
+        // When toggling a declaration from OFF to ON, if not renaming the property,
+        // do not mark the previous declaration for removal, otherwise the add and
+        // remove operations will cancel each other out when tracked. Tracked changes
+        // have no context of "disabled", only "add" or remove, like diffs.
+        if (prevDisabled && !change.newName && prevValue === newValue) {
+          data.remove = null;
+        }
+
         break;
 
       case "remove":
         data.add = null;
         data.remove = { property: change.name, value: prevValue };
         break;
     }