Bug 1463674 - Refactor char and chevron width measurement; r=bgrins. draft
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Thu, 05 Jul 2018 16:40:03 +0200
changeset 818820 d35caaf19b14d9a5cfbddaf58d20bc6c7aeb4aaa
parent 818819 aa728e3ee3254e2ef7eb1768e3f8621decca0535
child 818821 bf88130eb8e92b2bf29dac5024f0dc49f727e9c7
push id116354
push userbmo:nchevobbe@mozilla.com
push dateMon, 16 Jul 2018 16:01:04 +0000
reviewersbgrins
bugs1463674
milestone63.0a1
Bug 1463674 - Refactor char and chevron width measurement; r=bgrins. Create a separate function to measure the chevron width, and make the function that measure the char width pure by only returning the width. The assignment to internal properties (_inputCharWidth, _chevronWidth), is now done in componentDidMount which simplify reading this code. MozReview-Commit-ID: FitY97Y03Sg
devtools/client/sourceeditor/editor.js
devtools/client/webconsole/components/JSTerm.js
--- a/devtools/client/sourceeditor/editor.js
+++ b/devtools/client/sourceeditor/editor.js
@@ -53,32 +53,33 @@ const { OS } = Services.appinfo;
 
 const CM_SCRIPTS = [
   "chrome://devtools/content/sourceeditor/codemirror/codemirror.bundle.js",
 ];
 
 const CM_IFRAME = "chrome://devtools/content/sourceeditor/codemirror/cmiframe.html";
 
 const CM_MAPPING = [
+  "clearHistory",
+  "defaultCharWidth",
+  "extendSelection",
   "focus",
+  "getCursor",
+  "getScrollInfo",
+  "getSelection",
+  "getViewport",
   "hasFocus",
   "lineCount",
-  "somethingSelected",
-  "getCursor",
+  "openDialog",
+  "redo",
+  "refresh",
+  "replaceSelection",
   "setSelection",
-  "getSelection",
-  "replaceSelection",
-  "extendSelection",
+  "somethingSelected",
   "undo",
-  "redo",
-  "clearHistory",
-  "openDialog",
-  "refresh",
-  "getScrollInfo",
-  "getViewport"
 ];
 
 const editors = new WeakMap();
 
 Editor.modes = {
   css: { name: "css" },
   fs: { name: "x-shader/x-fragment" },
   haxe: { name: "haxe" },
--- a/devtools/client/webconsole/components/JSTerm.js
+++ b/devtools/client/webconsole/components/JSTerm.js
@@ -165,24 +165,16 @@ class JSTerm extends Component {
 
     const doc = this.hud.document;
     const toolbox = gDevTools.getToolbox(this.hud.owner.target);
     const tooltipDoc = toolbox ? toolbox.doc : doc;
     // The popup will be attached to the toolbox document or HUD document in the case
     // such as the browser console which doesn't have a toolbox.
     this.autocompletePopup = new AutocompletePopup(tooltipDoc, autocompleteOptions);
 
-    this.inputBorderSize = this.inputNode
-      ? this.inputNode.getBoundingClientRect().height - this.inputNode.clientHeight
-      : 0;
-
-    // Update the character width and height needed for the popup offset
-    // calculations.
-    this._updateCharSize();
-
     if (this.props.codeMirrorEnabled) {
       if (this.node) {
         this.editor = new Editor({
           autofocus: true,
           enableCodeFolding: false,
           gutters: [],
           lineWrapping: true,
           mode: Editor.modes.js,
@@ -275,16 +267,24 @@ class JSTerm extends Component {
       }
     } else if (this.inputNode) {
       this.inputNode.addEventListener("keypress", this._keyPress);
       this.inputNode.addEventListener("input", this._inputEventHandler);
       this.inputNode.addEventListener("keyup", this._inputEventHandler);
       this.focus();
     }
 
+    this.inputBorderSize = this.inputNode
+      ? this.inputNode.getBoundingClientRect().height - this.inputNode.clientHeight
+      : 0;
+
+    // Update the character and chevron width needed for the popup offset calculations.
+    this._inputCharWidth = this._getInputCharWidth();
+    this._chevronWidth = this.editor ? null : this._getChevronWidth();
+
     this.hud.window.addEventListener("blur", this._blurEventHandler);
     this.lastInputValue && this.setInputValue(this.lastInputValue);
   }
 
   shouldComponentUpdate(nextProps, nextState) {
     // XXX: For now, everything is handled in an imperative way and we
     // only want React to do the initial rendering of the component.
     // This should be modified when the actual refactoring will take place.
@@ -1286,44 +1286,65 @@ class JSTerm extends Component {
     if (!this.completeNode) {
       return;
     }
 
     // completion prefix = input, with non-control chars replaced by spaces
     const prefix = suffix ? this.getInputValue().replace(/[\S]/g, " ") : "";
     this.completeNode.value = prefix + suffix;
   }
+
   /**
-   * Calculates the width and height of a single character of the input box.
+   * Calculates and returns the width of a single character of the input box.
    * This will be used in opening the popup at the correct offset.
    *
-   * @private
+   * @returns {Number|null}: Width off the "x" char, or null if the input does not exist.
    */
-  _updateCharSize() {
-    if (this.props.codeMirrorEnabled || !this.inputNode) {
-      return;
+  _getInputCharWidth() {
+    if (!this.inputNode && !this.node) {
+      return null;
+    }
+
+    if (this.editor) {
+      return this.editor.defaultCharWidth();
     }
 
     const doc = this.hud.document;
     const tempLabel = doc.createElement("span");
     const style = tempLabel.style;
     style.position = "fixed";
     style.padding = "0";
     style.margin = "0";
     style.width = "auto";
     style.color = "transparent";
     WebConsoleUtils.copyTextStyles(this.inputNode, tempLabel);
     tempLabel.textContent = "x";
     doc.documentElement.appendChild(tempLabel);
-    this._inputCharWidth = tempLabel.offsetWidth;
+    const width = tempLabel.offsetWidth;
     tempLabel.remove();
-    // Calculate the width of the chevron placed at the beginning of the input
+    return width;
+  }
+
+  /**
+   * Calculates and returns the width of the chevron icon.
+   * This will be used in opening the popup at the correct offset.
+   *
+   * @returns {Number|null}: Width of the icon, or null if the input does not exist.
+   */
+  _getChevronWidth() {
+    if (!this.inputNode) {
+      return null;
+    }
+
+   // Calculate the width of the chevron placed at the beginning of the input
     // box. Remove 4 more pixels to accommodate the padding of the popup.
-    this._chevronWidth = +doc.defaultView.getComputedStyle(this.inputNode)
-                             .paddingLeft.replace(/[^0-9.]/g, "") - 4;
+    const doc = this.hud.document;
+    return doc.defaultView
+      .getComputedStyle(this.inputNode)
+      .paddingLeft.replace(/[^0-9.]/g, "") - 4;
   }
 
   destroy() {
     this.clearCompletion();
 
     this.webConsoleClient.clearNetworkRequests();
     if (this.hud.outputNode) {
       // We do this because it's much faster than letting React handle the ConsoleOutput