Bug 1222737 - Add colons and semi-colons in copied computed properties; r?pbro draft
authorMatt R <matthieu.rigolot@gmail.com>
Thu, 27 Apr 2017 10:47:29 +0100
changeset 586490 1588e309e44dd998e2dfdd50e9601a9d7af1fb1e
parent 585921 cce4d83d2b99ffedbd67a2f40ce26e53e9ae27ab
child 586491 25319db8ecf2baff6c3e5907806763e44ef3062c
push id61431
push userbmo:pbrosset@mozilla.com
push dateTue, 30 May 2017 15:05:11 +0000
reviewerspbro
bugs1222737
milestone55.0a1
Bug 1222737 - Add colons and semi-colons in copied computed properties; r?pbro Added : and ; characters to the DOM directly, visually hidden so they won't appear but will be part of the copied text. Changed divs to spans to make sure we don't have extra line breaks. And simplified the logic in copySelection as a result. MozReview-Commit-ID: EMVXAtv7EOo
devtools/client/inspector/computed/computed.js
devtools/client/themes/computed.css
--- a/devtools/client/inspector/computed/computed.js
+++ b/devtools/client/inspector/computed/computed.js
@@ -736,50 +736,18 @@ CssComputedView.prototype = {
 
   /**
    * Copy the current selection to the clipboard
    */
   copySelection: function () {
     try {
       let win = this.styleWindow;
       let text = win.getSelection().toString().trim();
-      // isPropertyPresent is set when a property name is spotted and
-      // we assume that the next line will be a property value.
-      let isPropertyPresent = false;
-      // Tidy up block headings by moving CSS property names and their
-      // values onto the same line and inserting a colon between them.
-      let textArray = text.split(/[\r\n]+/);
-      let result = "";
 
-      // Parse text array to output string.
-      if (textArray.length > 1) {
-        for (let prop of textArray) {
-          if (CssComputedView.propertyNames.indexOf(prop) !== -1) {
-            // Property name found so setting isPropertyPresent to true
-            isPropertyPresent = true;
-            // Property name
-            result += prop;
-          } else if (isPropertyPresent === true) {
-            // Since isPropertyPresent is true so we assume that this is
-            // a property value and we append it to result preceeded by
-            // a :.
-            result += ": " + prop + ";\n";
-            isPropertyPresent = false;
-          } else {
-            // since isPropertyPresent is not set, we assume this is
-            // normal text and we append it to result without any :.
-            result += prop + "\n";
-          }
-        }
-      } else {
-        // Short text fragment.
-        result = textArray[0];
-      }
-
-      clipboardHelper.copyString(result);
+      clipboardHelper.copyString(text);
     } catch (e) {
       console.error(e);
     }
   },
 
   /**
    * Destructor for CssComputedView.
    */
@@ -1002,65 +970,79 @@ PropertyView.prototype = {
       this.mdnLinkClick(event);
       // Prevent opening the options panel
       event.preventDefault();
       event.stopPropagation();
     });
     this.shortcuts.on("Return", (name, event) => this.onMatchedToggle(event));
     this.shortcuts.on("Space", (name, event) => this.onMatchedToggle(event));
 
-    let nameContainer = doc.createElementNS(HTML_NS, "div");
+    let nameContainer = doc.createElementNS(HTML_NS, "span");
     nameContainer.className = "property-name-container";
     this.element.appendChild(nameContainer);
 
     // Build the twisty expand/collapse
     this.matchedExpander = doc.createElementNS(HTML_NS, "div");
     this.matchedExpander.className = "expander theme-twisty";
     this.matchedExpander.addEventListener("click", this.onMatchedToggle);
     nameContainer.appendChild(this.matchedExpander);
 
     // Build the style name element
-    this.nameNode = doc.createElementNS(HTML_NS, "div");
-    this.nameNode.setAttribute("class", "property-name theme-fg-color5");
+    this.nameNode = doc.createElementNS(HTML_NS, "span");
+    this.nameNode.classList.add("property-name", "theme-fg-color5");
     // Reset its tabindex attribute otherwise, if an ellipsis is applied
     // it will be reachable via TABing
     this.nameNode.setAttribute("tabindex", "");
     // Avoid english text (css properties) from being altered
     // by RTL mode
     this.nameNode.setAttribute("dir", "ltr");
     this.nameNode.textContent = this.nameNode.title = this.name;
     // Make it hand over the focus to the container
     this.onFocus = () => this.element.focus();
     this.nameNode.addEventListener("click", this.onFocus);
+
+    // Build the style name ":" separator
+    let nameSeparator = doc.createElementNS(HTML_NS, "span");
+    nameSeparator.classList.add("visually-hidden");
+    nameSeparator.textContent = ": ";
+    this.nameNode.appendChild(nameSeparator);
+
     nameContainer.appendChild(this.nameNode);
 
-    let valueContainer = doc.createElementNS(HTML_NS, "div");
+    let valueContainer = doc.createElementNS(HTML_NS, "span");
     valueContainer.className = "property-value-container";
     this.element.appendChild(valueContainer);
 
     // Build the style value element
-    this.valueNode = doc.createElementNS(HTML_NS, "div");
-    this.valueNode.setAttribute("class", "property-value theme-fg-color1");
+    this.valueNode = doc.createElementNS(HTML_NS, "span");
+    this.valueNode.classList.add("property-value", "theme-fg-color1");
     // Reset its tabindex attribute otherwise, if an ellipsis is applied
     // it will be reachable via TABing
     this.valueNode.setAttribute("tabindex", "");
     this.valueNode.setAttribute("dir", "ltr");
     // Make it hand over the focus to the container
     this.valueNode.addEventListener("click", this.onFocus);
+
+    // Build the style value ";" separator
+    let valueSeparator = doc.createElementNS(HTML_NS, "span");
+    valueSeparator.classList.add("visually-hidden");
+    valueSeparator.textContent = ";";
+
     valueContainer.appendChild(this.valueNode);
+    valueContainer.appendChild(valueSeparator);
 
     return this.element;
   },
 
   buildSelectorContainer: function () {
     let doc = this.tree.styleDocument;
     let element = doc.createElementNS(HTML_NS, "div");
     element.setAttribute("class", this.propertyContentClassName);
     this.matchedSelectorsContainer = doc.createElementNS(HTML_NS, "div");
-    this.matchedSelectorsContainer.setAttribute("class", "matchedselectors");
+    this.matchedSelectorsContainer.classList.add("matchedselectors");
     element.appendChild(this.matchedSelectorsContainer);
 
     return element;
   },
 
   /**
    * Refresh the panel's CSS property value.
    */
@@ -1160,23 +1142,28 @@ PropertyView.prototype = {
         window: this.tree.styleWindow,
         target: link
       });
       shortcuts.on("Return", () => selector.openStyleEditor());
 
       let status = createChild(p, "span", {
         dir: "ltr",
         class: "rule-text theme-fg-color3 " + selector.statusClass,
-        title: selector.statusText,
+        title: selector.statusText
+      });
+
+      let keyDiv = createChild(status, "div", {
+        class: "fix-get-selection",
         textContent: selector.sourceText
       });
-      let valueSpan = createChild(status, "span", {
-        class: "other-property-value theme-fg-color1"
+
+      let valueDiv = createChild(status, "div", {
+        class: "fix-get-selection other-property-value theme-fg-color1"
       });
-      valueSpan.appendChild(selector.outputFragment);
+      valueDiv.appendChild(selector.outputFragment);
       promises.push(selector.ready);
     }
 
     this.matchedSelectorsContainer.innerHTML = "";
     this.matchedSelectorsContainer.appendChild(frag);
     return promise.all(promises);
   },
 
--- a/devtools/client/themes/computed.css
+++ b/devtools/client/themes/computed.css
@@ -128,16 +128,30 @@
 .theme-firebug .property-content {
   font-family: var(--proportional-font-family);
 }
 
 .theme-firebug .property-view {
   border-bottom: 1px solid rgba(0, 0, 0, 0.1);
 }
 
+/* Bug 1360238 - getSelection displays an extra "\n" on multiple sibling block elements.
+   We rely on this behavior to add an extra "\n" between matched selectors (Bug 1222737).
+   Therefore we use <div> elements around matched selectors and need this class
+   to keep them inline. We do that to avoid doing any formatting logic in JS.
+   Once Bug 1360238 will be fixed, we'll probably have to change the behavior
+   and remove this class. */
+.fix-get-selection {
+  display: inline;
+}
+
+.visually-hidden {
+  opacity: 0;
+}
+
 /* From skin */
 .expander {
   visibility: hidden;
 }
 
 .expandable {
   visibility: visible;
 }