Bug 1542213 - Track newly added CSS rules in the Changes panel. r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Tue, 23 Apr 2019 07:59:12 +0000
changeset 470455 0d0d5b67b2cc524045672b67a13401cd0dbf939e
parent 470454 03ea838144b57003bee53950ae138e23bf0f8066
child 470456 a4fab3aaa82bf0f9bc23a827b369c51c4db0642d
push id35906
push useraciure@mozilla.com
push dateTue, 23 Apr 2019 22:14:56 +0000
treeherdermozilla-central@0ce3633f8b80 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1542213, 1542288
milestone68.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 1542213 - Track newly added CSS rules in the Changes panel. r=pbro Introduces tracking for newly created CSS rules in the PageStyleActor. Adds a boolean flag, `isNew`, to the tracked rule in the Changes Redux store. Using this flag, render the new rule's selector in the React component as added (plus sign, green background). Ensure selector changes for thew new rule always overwrite the original selector (fix for Bug 1542288) instead of erroneously showing the original selector as removed. Removes obsolete documentation for "changeType" from the tracked rule in the Changes Redux store. This was never used. Differential Revision: https://phabricator.services.mozilla.com/D28215
devtools/client/inspector/changes/components/ChangesApp.js
devtools/client/inspector/changes/reducers/changes.js
devtools/client/inspector/changes/test/browser.ini
devtools/client/inspector/changes/test/browser_changes_rule_add.js
devtools/client/inspector/changes/test/browser_changes_rule_selector.js
devtools/client/inspector/changes/test/head.js
devtools/server/actors/styles.js
--- a/devtools/client/inspector/changes/components/ChangesApp.js
+++ b/devtools/client/inspector/changes/components/ChangesApp.js
@@ -88,52 +88,57 @@ class ChangesApp extends PureComponent {
           value,
         });
       });
 
     return [removals, additions];
   }
 
   renderRule(ruleId, rule, level = 0) {
+    const diffClass = rule.isNew ? "diff-add" : "";
     return dom.div(
       {
         key: ruleId,
         className: "changes__rule devtools-monospace",
         "data-rule-id": ruleId,
         style: {
           "--diff-level": level,
         },
       },
-      this.renderSelectors(rule.selectors),
+      this.renderSelectors(rule.selectors, rule.isNew),
       this.renderCopyButton(ruleId),
       // Render any nested child rules if they exist.
       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` }, "}")
+      // Render the closing bracket with a diff marker if necessary.
+      dom.div({ className: `level ${diffClass}` }, getDiffMarker(diffClass), "}")
     );
   }
 
   /**
    * Return an array of React elements for the rule's selector.
    *
    * @param  {Array} selectors
    *         List of strings as versions of this rule's selector over time.
+   * @param  {Boolean} isNewRule
+   *         Whether the rule was created at runtime.
    * @return {Array}
    */
-  renderSelectors(selectors) {
+  renderSelectors(selectors, isNewRule) {
     const selectorDiffClassMap = new Map();
 
     // The selectors array has just one item if it hasn't changed. Render it as-is.
+    // If the rule was created at runtime, mark the single selector as added.
     // If it has two or more items, the first item was the original selector (mark as
     // removed) and the last item is the current selector (mark as added).
     if (selectors.length === 1) {
-      selectorDiffClassMap.set(selectors[0], "");
+      selectorDiffClassMap.set(selectors[0], isNewRule ? "diff-add" : "");
     } else if (selectors.length >= 2) {
       selectorDiffClassMap.set(selectors[0], "diff-remove");
       selectorDiffClassMap.set(selectors[selectors.length - 1], "diff-add");
     }
 
     const elements = [];
 
     for (const [selector, diffClass] of selectorDiffClassMap) {
--- a/devtools/client/inspector/changes/reducers/changes.js
+++ b/devtools/client/inspector/changes/reducers/changes.js
@@ -88,16 +88,17 @@ function createRule(ruleData, rules) {
       const { selectors } = ruleAncestry[index];
       const prevRuleId = array[index - 1];
       const nextRuleId = array[index + 1];
 
       // Create an entry for this ruleId if one does not exist.
       if (!rules[ruleId]) {
         rules[ruleId] = {
           ruleId,
+          isNew: false,
           selectors,
           add: [],
           remove: [],
           children: [],
           parent: null,
         };
       }
 
@@ -141,21 +142,22 @@ function removeRule(ruleId, rules) {
  *
  * 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
+ *          isNew:       // {Boolean} Whether the tracked rule was created at runtime,
+ *                       //           meaning it didn't originally exist in the source.
  *          selectors:   // {Array} of CSS selectors or CSS at-rule text.
  *                       //         The array has just one item if the selector is never
  *                       //         changed. When the rule's selector is changed, the new
  *                       //         selector is pushed onto this array.
- *          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
  *              index:       // {Number} Position of the declaration within its CSS rule
  *            }
@@ -204,39 +206,46 @@ const reducers = {
       ancestors: [],
       add: [],
       remove: [],
     };
 
     change = { ...defaults, ...change };
     state = cloneState(state);
 
-    const { selector, ancestors, ruleIndex, type: changeType } = change;
+    const { selector, ancestors, ruleIndex } = change;
     // Bug 1525326: remove getSourceHash() and getRuleHash() in Firefox 70 after we no
     // longer support old servers which do not implement the id for the rule and source.
     const sourceId = change.source.id || getSourceHash(change.source);
     const ruleId =
       change.id || getRuleHash({ selectors: [selector], ancestors, ruleIndex });
 
     // Copy or create object identifying the source (styelsheet/element) for this change.
     const source = Object.assign({}, state[sourceId], change.source);
     // Copy or create collection of all rules ever changed in this source.
     const rules = Object.assign({}, source.rules);
     // Reference or create object identifying the rule for this change.
     const rule = rules[ruleId]
       ? rules[ruleId]
       : createRule({id: change.id, selectors: [selector], ancestors, ruleIndex}, rules);
 
+    // Mark the rule if it was created at runtime as a result of an "Add Rule" action.
+    if (change.type === "rule-add") {
+      rule.isNew = true;
+    }
+
     // If the first selector tracked for this rule is identical to the incoming selector,
     // reduce the selectors array to a single one. This handles the case for renaming a
     // selector back to its original name. It has no side effects for other changes which
     // preserve the selector.
+    // If the rule was created at runtime, always reduce the selectors array to one item.
+    // Changes to the new rule's selector always overwrite the original selector.
     // If the selectors are different, push the incoming one to the end of the array to
     // signify that the rule has changed selector. The last item is the current selector.
-    if (rule.selectors[0] === selector) {
+    if (rule.selectors[0] === selector || rule.isNew) {
       rule.selectors = [selector];
     } else {
       rule.selectors.push(selector);
     }
 
     if (change.remove && change.remove.length) {
       for (const decl of change.remove) {
         // Find the position of any added declaration which matches the incoming
@@ -266,17 +275,17 @@ const reducers = {
 
         // Delete any previous add operation which would be canceled out by this remove.
         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") {
+        if (change.type === "declaration-remove") {
           rule.add = rule.add.map((addDecl => {
             if (addDecl.index > decl.index) {
               addDecl.index--;
             }
 
             return addDecl;
           }));
 
@@ -318,19 +327,20 @@ const reducers = {
           // Track new add operation.
           rule.add.push(decl);
         }
       }
     }
 
     // Remove the rule if none of its declarations or selector have changed,
     // but skip cleanup if the selector is in process of being renamed (there are two
-    // changes happening in quick succession: selector-remove + selector-add).
+    // changes happening in quick succession: selector-remove + selector-add) or if the
+    // rule was created at runtime (allow empty new rules to persist).
     if (!rule.add.length && !rule.remove.length && rule.selectors.length === 1 &&
-        !changeType.startsWith("selector-")) {
+        !change.type.startsWith("selector-") && !rule.isNew) {
       removeRule(ruleId, rules);
       source.rules = { ...rules };
     } else {
       source.rules = { ...rules, [ruleId]: rule };
     }
 
     // Remove information about the source if none of its rules changed.
     if (!Object.keys(source.rules).length) {
--- a/devtools/client/inspector/changes/test/browser.ini
+++ b/devtools/client/inspector/changes/test/browser.ini
@@ -15,9 +15,10 @@ 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]
 [browser_changes_declaration_rename.js]
+[browser_changes_rule_add.js]
 [browser_changes_rule_selector.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/changes/test/browser_changes_rule_add.js
@@ -0,0 +1,60 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that adding a new CSS rule in the Rules view is tracked in the Changes panel.
+// Renaming the selector of the new rule should overwrite the tracked selector.
+
+const TEST_URI = `
+  <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);
+  const panel = doc.querySelector("#sidebar-panel-changes");
+
+  await selectNode("div", inspector);
+  await testTrackAddNewRule(store, inspector, ruleView, panel);
+  await testTrackRenameNewRule(store, inspector, ruleView, panel);
+});
+
+async function testTrackAddNewRule(store, inspector, ruleView, panel) {
+  const onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
+  info("Adding a new CSS rule in the Rule view");
+  await addNewRule(inspector, ruleView);
+  info("Pressing escape to leave the editor");
+  EventUtils.synthesizeKey("KEY_Escape");
+  info("Waiting for changes to be tracked");
+  await onTrackChange;
+
+  const addedSelectors = getAddedSelectors(panel);
+  const removedSelectors = getRemovedSelectors(panel);
+  is(addedSelectors.length, 1, "One selector was tracked as added");
+  is(addedSelectors.item(0).title, "div", "New rule's has DIV selector");
+  is(removedSelectors.length, 0, "No selectors tracked as removed");
+}
+
+async function testTrackRenameNewRule(store, inspector, ruleView, panel) {
+  info("Focusing the first rule's selector name in the Rule view");
+  const ruleEditor = getRuleViewRuleEditor(ruleView, 1);
+  const editor = await focusEditableField(ruleView, ruleEditor.selectorText);
+  info("Entering a new selector name");
+  editor.input.value = ".test";
+
+  // Expect two "TRACK_CHANGE" actions: one for removal, one for addition.
+  const onTrackChange = waitUntilAction(store, "TRACK_CHANGE", 2);
+  const onRuleViewChanged = once(ruleView, "ruleview-changed");
+  EventUtils.synthesizeKey("KEY_Enter");
+  await onRuleViewChanged;
+  info("Waiting for changes to be tracked");
+  await onTrackChange;
+
+  const addedSelectors = getAddedSelectors(panel);
+  const removedSelectors = getRemovedSelectors(panel);
+  is(addedSelectors.length, 1, "One selector was tracked as added");
+  is(addedSelectors.item(0).title, ".test", "New rule's selector was updated in place.");
+  is(removedSelectors.length, 0, "No selectors tracked as removed");
+}
--- a/devtools/client/inspector/changes/test/browser_changes_rule_selector.js
+++ b/devtools/client/inspector/changes/test/browser_changes_rule_selector.js
@@ -38,25 +38,22 @@ add_task(async function() {
   info("Waiting for rule view to update");
   await onRuleViewChanged;
   info("Wait for the change to be tracked");
   await onTrackChange;
 
   const rules = panel.querySelectorAll(".changes__rule");
   is(rules.length, 1, "One rule was tracked as changed");
 
-  const selectors = rules.item(0).querySelectorAll(".changes__selector");
-  is(selectors.length, 2, "Two selectors were tracked as changed");
-
-  const firstSelector = selectors.item(0);
-  is(firstSelector.title, "div", "Old selector name was tracked.");
-  ok(firstSelector.classList.contains("diff-remove"), "Old selector was removed.");
-
-  const secondSelector = selectors.item(1);
-  is(secondSelector.title, ".test", "New selector name was tracked.");
-  ok(secondSelector.classList.contains("diff-add"), "New selector was added.");
+  info("Expect old selector to be removed and new selector to be added");
+  const addedSelectors = getAddedSelectors(panel);
+  const removedSelectors = getRemovedSelectors(panel);
+  is(addedSelectors.length, 1, "One new selector was tracked as added");
+  is(addedSelectors.item(0).title, ".test", "New selector is correct");
+  is(removedSelectors.length, 1, "One old selector was tracked as removed");
+  is(removedSelectors.item(0).title, "div", "Old selector is correct");
 
   info("Expect no declarations to have been added or removed during selector change");
   const removeDecl = getRemovedDeclarations(doc, rules.item(0));
   is(removeDecl.length, 0, "No declarations removed");
   const addDecl = getAddedDeclarations(doc, rules.item(0));
   is(addDecl.length, 0, "No declarations added");
 });
--- a/devtools/client/inspector/changes/test/head.js
+++ b/devtools/client/inspector/changes/test/head.js
@@ -60,8 +60,31 @@ function getDeclarations(panelDoc, selec
 
 function getAddedDeclarations(panelDoc, containerNode) {
   return getDeclarations(panelDoc, ".diff-add", containerNode);
 }
 
 function getRemovedDeclarations(panelDoc, containerNode) {
   return getDeclarations(panelDoc, ".diff-remove", containerNode);
 }
+
+/**
+ * Get an array of DOM elements for the CSS selectors rendered in the Changes panel.
+ *
+ * @param  {Document} panelDoc
+ *         Host document of the Changes panel.
+ * @param  {String} selector
+ *         Optional selector to filter rendered selector DOM elements.
+ *         One of ".diff-remove" or ".diff-add".
+ *         If omitted, all selectors will be returned.
+ * @return {Array}
+ */
+function getSelectors(panelDoc, selector = "") {
+  return panelDoc.querySelectorAll(`.changes__selector${selector}`);
+}
+
+function getAddedSelectors(panelDoc) {
+  return getSelectors(panelDoc, ".diff-add");
+}
+
+function getRemovedSelectors(panelDoc) {
+  return getSelectors(panelDoc, ".diff-remove");
+}
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -978,17 +978,28 @@ var PageStyleActor = protocol.ActorClass
 
     // If inserting the rule succeeded, go ahead and edit the source
     // text if requested.
     const sheetActor = this._sheetRef(sheet);
     let {str: authoredText} = await sheetActor.getText();
     authoredText += "\n" + selector + " {\n" + "}";
     await sheetActor.update(authoredText, false);
 
-    return this.getNewAppliedProps(node, sheet.cssRules.item(index));
+    const cssRule = sheet.cssRules.item(index);
+    const ruleActor = this._styleRef(cssRule);
+
+    TrackChangeEmitter.trackChange({
+      ...ruleActor.metadata,
+      type: "rule-add",
+      add: null,
+      remove: null,
+      selector,
+    });
+
+    return this.getNewAppliedProps(node, cssRule);
   },
 });
 exports.PageStyleActor = PageStyleActor;
 
 const SUPPORTED_RULE_TYPES = [
   CSSRule.STYLE_RULE,
   CSSRule.SUPPORTS_RULE,
   CSSRule.KEYFRAME_RULE,