Bug 1508748 - Avoid duplicate tracking of the same removed CSS declaration. r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Tue, 04 Dec 2018 12:04:13 +0000
changeset 508501 9d1eeceace1b9b954791a2554c4e497c6fe8ab46
parent 508500 05e7fea07c8d4ac68741ae706a35d8905985785d
child 508502 5516f3e7b817f0ec3543d0fa6295c01e4363238b
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1508748
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 1508748 - Avoid duplicate tracking of the same removed CSS declaration. r=pbro When a declaration is disabled, it is tracked as removed in the Changes panel. When deleting a disabled declaration, we take care not track it as removed again. Includes stricter checks when matching previously tracked added and removed declarations. Differential Revision: https://phabricator.services.mozilla.com/D13616
devtools/client/inspector/changes/reducers/changes.js
devtools/client/inspector/changes/test/browser.ini
devtools/client/inspector/changes/test/browser_changes_declaration_remove_disabled.js
--- a/devtools/client/inspector/changes/reducers/changes.js
+++ b/devtools/client/inspector/changes/reducers/changes.js
@@ -211,28 +211,40 @@ const reducers = {
       }
     }
 
     if (change.remove && change.remove.length) {
       for (const decl of change.remove) {
         // Find the position of any added declaration which matches the incoming
         // declaration to be removed.
         const addIndex = rule.add.findIndex(addDecl => {
-          return addDecl.index === decl.index;
+          return addDecl.index === decl.index &&
+                 addDecl.property === decl.property &&
+                 addDecl.value === decl.value;
+        });
+
+        // Find the position of any removed declaration which matches the incoming
+        // declaration to be removed. It's possible to get duplicate remove operations
+        // when, for example, disabling a declaration then deleting it.
+        const removeIndex = rule.remove.findIndex(removeDecl => {
+          return removeDecl.index === decl.index &&
+                 removeDecl.property === decl.property &&
+                 removeDecl.value === decl.value;
         });
 
         // Track the remove operation only if the property was not previously introduced
         // by an add operation. This ensures repeated changes of the same property
-        // register as a single remove operation of its original value.
-        if (addIndex < 0) {
+        // register as a single remove operation of its original value. Avoid tracking the
+        // remove declaration if already tracked (happens on disable followed by delete).
+        if (addIndex < 0 && removeIndex < 0) {
           rule.remove.push(decl);
         }
 
         // Delete any previous add operation which would be canceled out by this remove.
-        if (rule.add[addIndex] && rule.add[addIndex].value === decl.value) {
+        if (rule.add[addIndex]) {
           rule.add.splice(addIndex, 1);
         }
 
         // Update the indexes of previously tracked declarations which follow this removed
         // one so future tracking continues to point to the right declarations.
         if (changeType === "declaration-remove") {
           rule.add = rule.add.map((addDecl => {
             if (addDecl.index > decl.index) {
@@ -253,28 +265,29 @@ const reducers = {
       }
     }
 
     if (change.add && change.add.length) {
       for (const decl of change.add) {
         // Find the position of any removed declaration which matches the incoming
         // declaration to be added.
         const removeIndex = rule.remove.findIndex(removeDecl => {
-          return removeDecl.index === decl.index;
+          return removeDecl.index === decl.index &&
+                 removeDecl.value === decl.value &&
+                 removeDecl.property === decl.property;
         });
 
         // Find the position of any added declaration which matches the incoming
         // declaration to be added in case we need to replace it.
         const addIndex = rule.add.findIndex(addDecl => {
-          return addDecl.index === decl.index;
+          return addDecl.index === decl.index &&
+                 addDecl.property === decl.property;
         });
 
-        if (rule.remove[removeIndex] &&
-            rule.remove[removeIndex].value === decl.value &&
-            rule.remove[removeIndex].property === decl.property) {
+        if (rule.remove[removeIndex]) {
           // Delete any previous remove operation which would be canceled out by this add.
           rule.remove.splice(removeIndex, 1);
         } else if (rule.add[addIndex]) {
           // Replace previous add operation for declaration at this index.
           rule.add.splice(addIndex, 1, decl);
         } else {
           // Track new add operation.
           rule.add.push(decl);
--- a/devtools/client/inspector/changes/test/browser.ini
+++ b/devtools/client/inspector/changes/test/browser.ini
@@ -11,11 +11,12 @@ support-files =
   !/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_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_remove_disabled.js
@@ -0,0 +1,99 @@
+/* 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 that disabling a CSS declaration and then removing it from the Rule view
+// is tracked as removed only once. Toggling leftover declarations should not introduce
+// duplicate changes.
+
+const TEST_URI = `
+  <style type='text/css'>
+    div {
+      color: red;
+      background: black;
+    }
+  </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 rule = getRuleViewRuleEditor(ruleView, 1).rule;
+
+  info("Using the second declaration");
+  await testRemoveValue(ruleView, store, doc, rule.textProps[1]);
+  info("Using the first declaration");
+  await testToggle(ruleView, store, doc, rule.textProps[0]);
+  info("Using the first declaration");
+  await testRemoveName(ruleView, store, doc, rule.textProps[0]);
+});
+
+async function testRemoveValue(ruleView, store, doc, prop) {
+  info("Test removing disabled declaration by clearing its property value.");
+  let onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Disable the declaration");
+  await togglePropStatus(ruleView, prop);
+  info("Wait for change to be tracked");
+  await onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Remove the disabled declaration by clearing its value");
+  await setProperty(ruleView, prop, null);
+  await onTrackChange;
+
+  const removeDecl = getRemovedDeclarations(doc);
+  is(removeDecl.length, 1, "Only one declaration tracked as removed");
+}
+
+async function testToggle(ruleView, store, doc, prop) {
+  info("Test toggling leftover declaration off and on will not track extra changes.");
+  let onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Disable the declaration");
+  await togglePropStatus(ruleView, prop);
+  await onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Re-enable the declaration");
+  await togglePropStatus(ruleView, prop);
+  await onTrackChange;
+
+  const removeDecl = getRemovedDeclarations(doc);
+  is(removeDecl.length, 1, "Still just one declaration tracked as removed");
+}
+
+async function testRemoveName(ruleView, store, doc, prop) {
+  info("Test removing disabled declaration by clearing its property name.");
+  let onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Disable the declaration");
+  await togglePropStatus(ruleView, prop);
+  await onTrackChange;
+
+  onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Remove the disabled declaration by clearing its name");
+  await removeProperty(ruleView, prop);
+  await onTrackChange;
+
+  info(`Expecting two declarations removed:
+  - one removed by its value in the other test
+  - one removed by its name from this test
+  `);
+
+  const removeDecl = getRemovedDeclarations(doc);
+  is(removeDecl.length, 2, "Two declarations tracked as removed");
+  is(removeDecl[0].property, "background", "First declaration name correct");
+  is(removeDecl[0].value, "black", "First declaration value correct");
+  is(removeDecl[1].property, "color", "Second declaration name correct");
+  is(removeDecl[1].value, "red", "Second declaration value correct");
+}