Backed out 3 changesets (bug 1530341, bug 1529606) for causing devtools failures on browser_parsable_css.js CLOSED TREE
authorarthur.iakab <aiakab@mozilla.com>
Mon, 25 Feb 2019 21:35:09 +0200
changeset 460955 4613c00d3163dac8523877feeed7a661efd9510b
parent 460954 b202dfe38a82d3074a4b1fd2b33232c8a7c3241e
child 460956 7bffd747d54adf39be339cfac01d785e4654ea55
push id35613
push usernerli@mozilla.com
push dateTue, 26 Feb 2019 03:52:35 +0000
treeherdermozilla-central@faec87a80ed1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1530341, 1529606
milestone67.0a1
backs out8a760030e793ae022a93bfc1b62e29686f7df5f6
2c74db63f9aa20def8805cda096774d939ff5a78
96e76fdfd8b247997bbf9545ca043934f7279608
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
Backed out 3 changesets (bug 1530341, bug 1529606) for causing devtools failures on browser_parsable_css.js CLOSED TREE Backed out changeset 8a760030e793 (bug 1530341) Backed out changeset 2c74db63f9aa (bug 1530341) Backed out changeset 96e76fdfd8b2 (bug 1529606)
devtools/client/inspector/changes/ChangesContextMenu.js
devtools/client/inspector/changes/ChangesView.js
devtools/client/inspector/changes/components/ChangesApp.js
devtools/client/inspector/changes/selectors/changes.js
devtools/client/inspector/changes/test/browser_changes_rule_selector.js
devtools/client/inspector/changes/test/unit/test_changes_stylesheet.js
devtools/client/locales/en-US/changes.properties
devtools/client/themes/changes.css
devtools/client/themes/variables.css
--- a/devtools/client/inspector/changes/ChangesContextMenu.js
+++ b/devtools/client/inspector/changes/ChangesContextMenu.js
@@ -23,16 +23,17 @@ class ChangesContextMenu {
     this.inspector = this.view.inspector;
     // Document object to which the Changes panel belongs to.
     this.document = this.view.document;
     // DOM element container for the Changes panel content.
     this.panel = this.document.getElementById("sidebar-panel-changes");
     // Window object to which the Changes panel belongs to.
     this.window = this.document.defaultView;
 
+    this._onCopyChanges = this.view.copyChanges.bind(this.view);
     this._onCopyRule = this.view.copyRule.bind(this.view);
     this._onCopySelection = this.view.copySelection.bind(this.view);
     this._onSelectAll = this._onSelectAll.bind(this);
   }
 
   show(event) {
     this._openMenu({
       target: event.target,
@@ -52,29 +53,51 @@ class ChangesContextMenu {
       accesskey: getStr("changes.contextmenu.copy.accessKey"),
       click: this._onCopySelection,
       disabled: !this._hasTextSelected(),
     });
     menu.append(menuitemCopy);
 
     const ruleEl = target.closest("[data-rule-id]");
     const ruleId = ruleEl ? ruleEl.dataset.ruleId : null;
+    const sourceEl = target.closest("[data-source-id]");
+    const sourceId = sourceEl ? sourceEl.dataset.sourceId : null;
 
-    if (ruleId) {
+    // When both rule id and source id are found, deal with just for that rule.
+    if (ruleId && sourceId) {
+      // Copy Changes option
+      menu.append(new MenuItem({
+        label: getStr("changes.contextmenu.copyChanges"),
+        click: () => this._onCopyChanges(ruleId, sourceId),
+      }));
+
       // Copy Rule option
       menu.append(new MenuItem({
         label: getStr("changes.contextmenu.copyRule"),
         click: () => this._onCopyRule(ruleId),
       }));
 
       menu.append(new MenuItem({
         type: "separator",
       }));
     }
 
+    // When only the source id is found, deal with all changed rules in that source.
+    if (!ruleId && sourceId) {
+      // Copy All Changes option
+      menu.append(new MenuItem({
+        label: getStr("changes.contextmenu.copyAllChanges"),
+        click: () => this._onCopyChanges(null, sourceId),
+      }));
+
+      menu.append(new MenuItem({
+        type: "separator",
+      }));
+    }
+
     // Select All option
     const menuitemSelectAll = new MenuItem({
       label: getStr("changes.contextmenu.selectAll"),
       accesskey: getStr("changes.contextmenu.selectAll.accessKey"),
       click: this._onSelectAll,
     });
     menu.append(menuitemSelectAll);
 
--- a/devtools/client/inspector/changes/ChangesView.js
+++ b/devtools/client/inspector/changes/ChangesView.js
@@ -35,18 +35,16 @@ class ChangesView {
     this.telemetry = this.inspector.telemetry;
     this.window = window;
 
     this.onAddChange = this.onAddChange.bind(this);
     this.onClearChanges = this.onClearChanges.bind(this);
     this.onChangesFront = this.onChangesFront.bind(this);
     this.onContextMenu = this.onContextMenu.bind(this);
     this.onCopy = this.onCopy.bind(this);
-    this.onCopyAllChanges = this.copyAllChanges.bind(this);
-    this.onCopyRule = this.copyRule.bind(this);
     this.destroy = this.destroy.bind(this);
 
     this.init();
   }
 
   get contextMenu() {
     if (!this._contextMenu) {
       this._contextMenu = new ChangesContextMenu(this);
@@ -54,18 +52,16 @@ class ChangesView {
 
     return this._contextMenu;
   }
 
   init() {
     const changesApp = ChangesApp({
       onContextMenu: this.onContextMenu,
       onCopy: this.onCopy,
-      onCopyAllChanges: this.onCopyAllChanges,
-      onCopyRule: this.onCopyRule,
     });
 
     // listen to the front for initialization, add listeners
     // when it is ready
     this._getChangesFront();
 
     // Expose the provider to let inspector.js use it in setupSidebar.
     this.provider = createElement(Provider, {
@@ -104,24 +100,16 @@ class ChangesView {
       // The connection to the server may have been cut, for
       // example during test
       // teardown. Here we just catch the error and silently
       // ignore it.
     }
   }
 
   /**
-   * Handler for the "Copy All Changes" button. Simple wrapper that just calls
-   * |this.copyChanges()| with no filters in order to trigger default operation.
-   */
-  copyAllChanges() {
-    this.copyChanges();
-  }
-
-  /**
    * Handler for the "Copy Changes" option from the context menu.
    * Builds a CSS text with the aggregated changes and copies it to the clipboard.
    *
    * Optional rule and source ids can be used to filter the scope of the operation:
    * - if both a rule id and source id are provided, copy only the changes to the
    * matching rule within the matching source.
    * - if only a source id is provided, copy the changes to all rules within the
    * matching source.
--- a/devtools/client/inspector/changes/components/ChangesApp.js
+++ b/devtools/client/inspector/changes/components/ChangesApp.js
@@ -18,51 +18,23 @@ class ChangesApp extends PureComponent {
   static get propTypes() {
     return {
       // Nested CSS rule tree structure of CSS changes grouped by source (stylesheet)
       changesTree: PropTypes.object.isRequired,
       // Event handler for "contextmenu" event
       onContextMenu: PropTypes.func.isRequired,
       // Event handler for "copy" event
       onCopy: PropTypes.func.isRequired,
-      // Event handler for click on "Copy All Changes" button
-      onCopyAllChanges: PropTypes.func.isRequired,
-      // Event handler for click on "Copy Rule" button
-      onCopyRule: PropTypes.func.isRequired,
     };
   }
 
   constructor(props) {
     super(props);
   }
 
-  renderCopyAllChangesButton() {
-    return dom.button(
-      {
-        className: "changes__copy-all-changes-button",
-        onClick: this.props.onCopyAllChanges,
-        title: getStr("changes.contextmenu.copyAllChangesDescription"),
-      },
-      getStr("changes.contextmenu.copyAllChanges")
-    );
-  }
-
-  renderCopyButton(ruleId) {
-    return dom.button(
-      {
-        className: "changes__copy-rule-button",
-        onClick: e => {
-          this.props.onCopyRule(ruleId);
-        },
-        title: getStr("changes.contextmenu.copyRuleDescription"),
-      },
-      getStr("changes.contextmenu.copyRule")
-    );
-  }
-
   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({
           key: "remove-" + property + index,
           className: "level diff-remove",
@@ -87,24 +59,23 @@ class ChangesApp extends PureComponent {
 
     return [removals, additions];
   }
 
   renderRule(ruleId, rule, level = 0) {
     return dom.div(
       {
         key: ruleId,
-        className: "changes__rule devtools-monospace",
+        className: "rule devtools-monospace",
         "data-rule-id": ruleId,
         style: {
           "--diff-level": level,
         },
       },
       this.renderSelectors(rule.selectors),
-      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` }, "}")
     );
@@ -131,17 +102,17 @@ class ChangesApp extends PureComponent {
     }
 
     const elements = [];
 
     for (const [selector, diffClass] of selectorDiffClassMap) {
       elements.push(dom.div(
         {
           key: selector,
-          className: `level changes__selector ${diffClass}`,
+          className: `level selector ${diffClass}`,
           title: selector,
         },
         getDiffMarker(diffClass),
         selector,
         dom.span({}, " {")
       ));
     }
 
@@ -198,17 +169,16 @@ class ChangesApp extends PureComponent {
     return dom.div(
       {
         className: "theme-sidebar inspector-tabpanel",
         id: "sidebar-panel-changes",
         onContextMenu: this.props.onContextMenu,
         onCopy: this.props.onCopy,
       },
       !hasChanges && this.renderEmptyState(),
-      hasChanges && this.renderCopyAllChangesButton(),
       hasChanges && this.renderDiff(this.props.changesTree)
     );
   }
 }
 
 /**
  * Get a React element with text content of either a plus or minus sign according to
  * the given CSS class name used to mark added or removed lines in the changes diff view.
--- a/devtools/client/inspector/changes/selectors/changes.js
+++ b/devtools/client/inspector/changes/selectors/changes.js
@@ -220,21 +220,20 @@ function getChangesStylesheet(state, fil
 
     return removals + additions;
   }
 
   // Iterate through all sources in the change tree and build a CSS stylesheet string.
   return Object.entries(changeTree).reduce((stylesheetText, [sourceId, source]) => {
     const { href, rules } = source;
     // Write code comment with source origin
-    stylesheetText += `\n/* ${getSourceForDisplay(source)} | ${href} */\n`;
+    stylesheetText += `/* ${getSourceForDisplay(source)} | ${href} */\n`;
     // Write CSS rules
     stylesheetText += Object.entries(rules).reduce((str, [ruleId, rule]) => {
-      // Add a new like only after top-level rules (level == 0)
-      str += writeRule(ruleId, rule, 0) + "\n";
+      str += writeRule(ruleId, rule, 0);
       return str;
     }, "");
 
     return stylesheetText;
   }, "");
 }
 
 module.exports = {
--- a/devtools/client/inspector/changes/test/browser_changes_rule_selector.js
+++ b/devtools/client/inspector/changes/test/browser_changes_rule_selector.js
@@ -35,20 +35,20 @@ add_task(async function() {
   const onRuleViewChanged = once(ruleView, "ruleview-changed");
   info("Pressing Enter key to commit the change");
   EventUtils.synthesizeKey("KEY_Enter");
   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");
+  const rules = panel.querySelectorAll(".rule");
   is(rules.length, 1, "One rule was tracked as changed");
 
-  const selectors = rules.item(0).querySelectorAll(".changes__selector");
+  const selectors = rules.item(0).querySelectorAll(".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.");
--- a/devtools/client/inspector/changes/test/unit/test_changes_stylesheet.js
+++ b/devtools/client/inspector/changes/test/unit/test_changes_stylesheet.js
@@ -5,39 +5,39 @@
 
 // Check that getChangesStylesheet() serializes tracked changes from nested CSS rules
 // into the expected stylesheet format.
 
 const { getChangesStylesheet } = require("devtools/client/inspector/changes/selectors/changes");
 
 const { CHANGES_STATE } = require("resource://test/mocks");
 
-// Wrap multi-line string in backticks to ensure exact check in test, including new lines.
+// Wrap multi-line string in backticks and trim to ensure exact string check in test.
 const STYLESHEET_FOR_ANCESTOR = `
 /* Inline #0 | http://localhost:5000/at-rules-nested.html */
 
 @media (min-width: 50em) {
   @supports (display: grid) {
     body {
       /* background-color: royalblue; */
       background-color: red;
     }
   }
 }
-`;
+`.trim();
 
-// Wrap multi-line string in backticks to ensure exact check in test, including new lines.
+// Wrap multi-line string in backticks and trim to ensure exact string check in test.
 const STYLESHEET_FOR_DESCENDANT = `
 /* Inline #0 | http://localhost:5000/at-rules-nested.html */
 
 body {
   /* background-color: royalblue; */
   background-color: red;
 }
-`;
+`.trim();
 
 add_test(() => {
   info("Check stylesheet generated for the first ancestor in the CSS rule tree.");
   equal(getChangesStylesheet(CHANGES_STATE), STYLESHEET_FOR_ANCESTOR,
     "Stylesheet includes all ancestors.");
 
   info("Check stylesheet generated for the last descendant in the CSS rule tree.");
   const filter = { sourceIds: ["source1"], ruleIds: ["rule3"] };
--- a/devtools/client/locales/en-US/changes.properties
+++ b/devtools/client/locales/en-US/changes.properties
@@ -28,33 +28,28 @@ changes.iframeLabel=iframe
 # LOCALIZATION NOTE (changes.contextmenu.copy): Label for "Copy" option in Changes panel
 # context menu
 changes.contextmenu.copy=Copy
 
 # LOCALIZATION NOTE (changes.contextmenu.copy.accessKey): Access key for "Copy"
 # option in the Changes panel.
 changes.contextmenu.copy.accessKey=C
 
+# LOCALIZATION NOTE (changes.contextmenu.copyChanges): Label for "Copy Changes" option in
+# Changes panel context menu which copies only the subset of changed CSS declarations.
+changes.contextmenu.copyChanges=Copy Changes
+
 # LOCALIZATION NOTE (changes.contextmenu.copyAllChanges): Label for "Copy All Changes"
 # option in Changes panel context menu which copies all changed CSS declarations from a
 # stylesheet
 changes.contextmenu.copyAllChanges=Copy All Changes
 
-# LOCALIZATION NOTE (changes.contextmenu.copyAllChangesDescription): Detailed explanation
-# for "Copy All Changes" option in Changes panel. Used as title attribute on "Copy All
-# Changes" button
-changes.contextmenu.copyAllChangesDescription=Copy a list of all CSS changes to clipboard.
-
 # LOCALIZATION NOTE (changes.contextmenu.copyRule): Label for "Copy Rule" option in
 # Changes panel context menu which copies the complete contents of a CSS rule.
 changes.contextmenu.copyRule=Copy Rule
 
-# LOCALIZATION NOTE (changes.contextmenu.copyRuleDescription): Detailed explanation for
-# "Copy Rule" option in Changes panel. Used as title attribute on "Copy Rule" button.
-changes.contextmenu.copyRuleDescription=Copy contents of this CSS rule to clipboard.
-
 # LOCALIZATION NOTE (changes.contextmenu.selectAll): Label for "Select All" option in the
 # Changes panel context menu to select all text content.
 changes.contextmenu.selectAll=Select All
 
 # LOCALIZATION NOTE (changes.contextmenu.selectAll.accessKey): Access key for "Select All"
 # option in the Changes panel.
 changes.contextmenu.selectAll.accessKey=A
--- a/devtools/client/themes/changes.css
+++ b/devtools/client/themes/changes.css
@@ -1,33 +1,29 @@
 /* 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/. */
 
  /* CSS Variables specific to the Changes panel that aren't defined by the themes */
  :root {
-   --changes-button-background-color: var(--grey-90-a10);
-   --changes-button-background-color-hover: var(--grey-90-a15);
-   --changes-button-background-color-active: var(--grey-90-a20);
    --diff-add-background-color: #f1feec;
    --diff-add-text-color: #54983f;
    --diff-remove-background-color: #fbf2f5;
    --diff-remove-text-color: #bf7173;
    --diff-source-background: var(--theme-toolbar-background);
    --diff-level: 0;
    --diff-level-offset: 10px;
    /*
     Minimum padding so content on the first level (zero) isn't touching the edge. Added
     and removed lines will re-declare this to add extra padding to clear the +/- icons.
    */
    --diff-level-min-offset: 5px;
  }
 
 :root.theme-dark {
-  --changes-button-background-color: #252526;
   --diff-add-background-color: rgba(18, 188, 0, 0.15);
   --diff-add-text-color: #12BC00;
   --diff-remove-background-color: rgba(255, 0, 57, 0.15);
   --diff-remove-text-color: #FF0039;
   --diff-source-background: #222225;
 }
 
 :root[dir="rtl"] {
@@ -76,64 +72,20 @@
 #sidebar-panel-changes .level {
   padding-top: 3px;
   padding-right: 5px;
   padding-bottom: 3px;
   padding-left: calc(var(--diff-level-min-offset) +
                      var(--diff-level-offset) * var(--diff-level));
 }
 
-.changes__copy-all-changes-button {
-  -moz-context-properties: fill;
-  background: url(chrome://devtools/skin/images/copy.svg) no-repeat;
-  background-size: 16px;
-  border: none;
-  color: var(--theme-body-color);
-  fill: currentColor;
-  height: 16px;
-  margin: 7px 10px;
-  padding-left: 21px;
-}
-
-/* Hide the Copy Rule button by default. */
-.changes__copy-rule-button {
-  background-color: var(--changes-button-background-color);
-  border-radius: 5px;
-  border: none;
-  color: var(--theme-body-color);
-  display: none;
-  padding: 1px 5px;
-  position: absolute;
-  right: 5px;
-  top: 2px;
-}
-
-.changes__copy-rule-button:hover {
-  background-color: var(--changes-button-background-color-hover);
-}
-
-.changes__copy-rule-button:active {
-  background-color: var(--changes-button-background-color-active);
-}
-
-.changes__rule {
-  position: relative;
-}
-
-.changes__selector {
+#sidebar-panel-changes .selector {
   word-wrap: break-word;
 }
 
-/* Show the Copy Rule button when hovering over the rule's selector elements */
-.changes__selector:hover + .changes__copy-rule-button,
-.changes__selector:hover + .changes__selector + .changes__copy-rule-button,
-.changes__copy-rule-button:hover {
-  display: block;
-}
-
 #sidebar-panel-changes .declaration-name {
   margin-left: 10px;
 }
 
 .diff-add,
 .diff-remove {
   --diff-level-min-offset: 15px;
   position: relative;
--- a/devtools/client/themes/variables.css
+++ b/devtools/client/themes/variables.css
@@ -250,20 +250,9 @@
   --grey-45: #939395;
   --grey-50: #737373;
   --grey-55: #5c5c5f;
   --grey-60: #4a4a4f;
   --grey-70: #38383d;
   --grey-80: #2a2a2e;
   --grey-85: #1b1b1d;
   --grey-90: #0c0c0d;
-  --grey-90-a05: rgba(12, 12, 13, 0.05);
-  --grey-90-a10: rgba(12, 12, 13, 0.1);
-  --grey-90-a15: rgba(12, 12, 13, 0.15);
-  --grey-90-a20: rgba(12, 12, 13, 0.2);
-  --grey-90-a30: rgba(12, 12, 13, 0.3);
-  --grey-90-a40: rgba(12, 12, 13, 0.4);
-  --grey-90-a50: rgba(12, 12, 13, 0.5);
-  --grey-90-a60: rgba(12, 12, 13, 0.6);
-  --grey-90-a70: rgba(12, 12, 13, 0.7);
-  --grey-90-a80: rgba(12, 12, 13, 0.8);
-  --grey-90-a90: rgba(12, 12, 13, 0.9);
 }