Bug 1518512 - (Part 2) Ensure all text in Changes panel can be copied. r=gl
☠☠ backed out by a7918210d2f1 ☠ ☠
authorRazvan Caliman <rcaliman@mozilla.com>
Fri, 25 Jan 2019 12:40:34 +0000
changeset 515428 c8f0d19844f67cd9a2189f612e4f1a865d697b04
parent 515427 3c03e2282b4f180cbc6dfd2c37ac23d192050fe1
child 515429 e0034670b26aaea119876ed739167136b75e1ec9
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1518512
milestone66.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 1518512 - (Part 2) Ensure all text in Changes panel can be copied. r=gl Depends on D17255 Replaces CSS pseudo-elements for added/removed line markers with text elements that can be copied with the rest of the content. Differential Revision: https://phabricator.services.mozilla.com/D17256
devtools/client/inspector/changes/components/CSSDeclaration.js
devtools/client/inspector/changes/components/ChangesApp.js
devtools/client/themes/changes.css
--- a/devtools/client/inspector/changes/components/CSSDeclaration.js
+++ b/devtools/client/inspector/changes/components/CSSDeclaration.js
@@ -6,33 +6,36 @@
 
 const { PureComponent } = require("devtools/client/shared/vendor/react");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 
 class CSSDeclaration extends PureComponent {
   static get propTypes() {
     return {
+      className: PropTypes.string,
+      marker: PropTypes.object,
       property: PropTypes.string.isRequired,
       value: PropTypes.string.isRequired,
-      className: PropTypes.string,
     };
   }
 
   static get defaultProps() {
     return {
       className: "",
+      marker: null,
     };
   }
 
   render() {
-    const { property, value, className } = this.props;
+    const { className, marker, property, value } = this.props;
 
     return dom.div({ className: `declaration ${className}` },
+      marker,
       dom.span({ className: "declaration-name theme-fg-color3"}, property),
-      ":",
+      ": ",
       dom.span({ className: "declaration-value theme-fg-color1"}, value),
       ";"
     );
   }
 }
 
 module.exports = CSSDeclaration;
--- a/devtools/client/inspector/changes/components/ChangesApp.js
+++ b/devtools/client/inspector/changes/components/ChangesApp.js
@@ -31,28 +31,30 @@ class ChangesApp extends PureComponent {
   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",
+          marker: getDiffMarker("diff-remove"),
           property,
           value,
         });
       });
 
     const additions = add
       // 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: "add-" + property + index,
           className: "level diff-add",
+          marker: getDiffMarker("diff-add"),
           property,
           value,
         });
       });
 
     return [removals, additions];
   }
 
@@ -74,26 +76,30 @@ class ChangesApp extends PureComponent {
           "--diff-level": level,
         },
       },
       dom.div(
         {
           className: `level selector ${diffClass}`,
           title: selector,
         },
+        getDiffMarker(diffClass),
         selector,
-        dom.span({ className: "bracket-open" }, "{")
+        dom.span({ className: "bracket-open" }, " {")
       ),
       // 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 bracket-close ${diffClass}` }, "}")
+      dom.div({ className: `level bracket-close ${diffClass}` },
+        getDiffMarker(diffClass),
+        "}"
+      )
     );
   }
 
   renderDiff(changes = {}) {
     // Render groups of style sources: stylesheets and element style attributes.
     return Object.entries(changes).map(([sourceId, source]) => {
       const path = getSourceForDisplay(source);
       const { href, rules, isFramed } = source;
@@ -145,15 +151,39 @@ class ChangesApp extends PureComponent {
         onContextMenu: this.props.onContextMenu,
       },
       !hasChanges && this.renderEmptyState(),
       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.
+ * This is used as a diff line maker that can be copied over as text with the rest of the
+ * content. CSS pseudo-elements are not part of the document flow and cannot be copied.
+ *
+ * @param  {String} className
+ *         One of "diff-add" or "diff-remove"
+ * @return {Component|null}
+ *         Returns null if the given className isn't recognized. React handles it.
+ */
+function getDiffMarker(className) {
+  let marker = null;
+  switch (className) {
+    case "diff-add":
+      marker = dom.span({ className: "diff-marker" }, "+ ");
+      break;
+    case "diff-remove":
+      marker = dom.span({ className: "diff-marker" }, "- ");
+      break;
+  }
+  return marker;
+}
+
 const mapStateToProps = state => {
   return {
     changesTree: getChangesTree(state.changes),
   };
 };
 
 module.exports = connect(mapStateToProps)(ChangesApp);
--- a/devtools/client/themes/changes.css
+++ b/devtools/client/themes/changes.css
@@ -70,17 +70,16 @@
 }
 
 #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));
-  position: relative;
 }
 
 #sidebar-panel-changes .selector {
   word-wrap: break-word;
 }
 
 #sidebar-panel-changes .rule .bracket-open {
   display: inline-block;
@@ -93,37 +92,35 @@
 
 #sidebar-panel-changes .declaration-value {
   margin-left: 5px;
 }
 
 .diff-add,
 .diff-remove {
   --diff-level-min-offset: 15px;
+  position: relative;
 }
 
-.diff-add::before,
-.diff-remove::before {
+.diff-marker {
   position: absolute;
   left: 5px;
 }
 
 .diff-add {
   background-color: var(--diff-add-background-color);
 }
 
-.diff-add::before {
-  content: "+";
+.diff-add .diff-marker {
   color: var(--diff-add-text-color);
 }
 
 .diff-remove {
   background-color: var(--diff-remove-background-color);
 }
 
-.diff-remove::before{
-  content: "-";
+.diff-remove .diff-marker{
   color: var(--diff-remove-text-color);
 }
 
 #sidebar-panel-changes .devtools-sidepanel-no-result :not(:first-child) {
   font-style: normal;
 }