Bug 1168246 - part1: CSS autocomplete picks most popular prop;r=pbrosset
authorJulian Descottes <jdescottes@mozilla.com>
Tue, 29 Mar 2016 16:08:17 +0200
changeset 290808 7aacc3495dc8b9d93c3ee3421c6ce1326ad36e11
parent 290807 ee11a508dd472945960264b5ca777dccc5961bdf
child 290809 d8fb6dbbb47874a2ea7f3a5eec7a0784d3fe51ec
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset
bugs1168246
milestone48.0a1
Bug 1168246 - part1: CSS autocomplete picks most popular prop;r=pbrosset Based on the original patch from danemacmillan. * suggestion-picker.js Add a new shared util to find the most popular css property in an array. The list of popular css properties is extracted from chrome devtools code. * autocomplete-popup.js Can specify selected item index when opening the popup or setting items. * inplace-editor.js Use the suggestion-picker to select a default property. MozReview-Commit-ID: JuWZzbBSBqX
devtools/client/shared/autocomplete-popup.js
devtools/client/shared/inplace-editor.js
devtools/client/shared/moz.build
devtools/client/shared/suggestion-picker.js
devtools/client/shared/test/unit/test_suggestion-picker.js
devtools/client/shared/test/unit/xpcshell.ini
--- a/devtools/client/shared/autocomplete-popup.js
+++ b/devtools/client/shared/autocomplete-popup.js
@@ -131,31 +131,50 @@ AutocompletePopup.prototype = {
    * @param nsIDOMNode aAnchor
    *        Optional node to anchor the panel to.
    * @param Number aXOffset
    *        Horizontal offset in pixels from the left of the node to the left
    *        of the popup.
    * @param Number aYOffset
    *        Vertical offset in pixels from the top of the node to the starting
    *        of the popup.
+   * @param integer index
+   *        The position of item to select.
    */
-  openPopup: function AP_openPopup(aAnchor, aXOffset = 0, aYOffset = 0)
+  openPopup: function AP_openPopup(aAnchor, aXOffset = 0, aYOffset = 0, index)
   {
     this.__maxLabelLength = -1;
     this._updateSize();
     this._panel.openPopup(aAnchor, this.position, aXOffset, aYOffset);
 
     if (this.autoSelect) {
-      this.selectFirstItem();
+      this.selectItemAtIndex(index);
     }
 
     this.emit("popup-opened");
   },
 
   /**
+   * Select item at the provided index.
+   *
+   * @param {Number} index
+   *        The position of the item to select.
+   */
+  selectItemAtIndex: function AP_selectItemAtIndex(index)
+  {
+    if (typeof index != "number") {
+      // If no index was provided, select the item closest to the input.
+      let isAboveInput = this.position.includes("before");
+      index = isAboveInput ? this.itemCount - 1 : 0;
+    }
+    this.selectedIndex = index;
+    this._list.ensureIndexIsVisible(this._list.selectedIndex);
+  },
+
+  /**
    * Hide the autocomplete popup panel.
    */
   hidePopup: function AP_hidePopup()
   {
     // Return accessibility focus to the input.
     this._document.activeElement.removeAttribute("aria-activedescendant");
     this._panel.hidePopup();
   },
@@ -232,47 +251,33 @@ AutocompletePopup.prototype = {
     return items;
   },
 
   /**
    * Set the autocomplete items list, in one go.
    *
    * @param array aItems
    *        The list of items you want displayed in the popup list.
+   * @param integer index
+   *        The position of the item to select.
    */
-  setItems: function AP_setItems(aItems)
+  setItems: function AP_setItems(aItems, index)
   {
     this.clearItems();
     aItems.forEach(this.appendItem, this);
 
     // Make sure that the new content is properly fitted by the XUL richlistbox.
     if (this.isOpen) {
       if (this.autoSelect) {
-        this.selectFirstItem();
+        this.selectItemAtIndex(index);
       }
       this._updateSize();
     }
   },
 
-  /**
-   * Selects the first item of the richlistbox. Note that first item here is the
-   * item closes to the input element, which means that 0th index if position is
-   * below, and last index if position is above.
-   */
-  selectFirstItem: function AP_selectFirstItem()
-  {
-    if (this.position.includes("before")) {
-      this.selectedIndex = this.itemCount - 1;
-    }
-    else {
-      this.selectedIndex = 0;
-    }
-    this._list.ensureIndexIsVisible(this._list.selectedIndex);
-  },
-
   __maxLabelLength: -1,
 
   get _maxLabelLength() {
     if (this.__maxLabelLength != -1) {
       return this.__maxLabelLength;
     }
 
     let max = 0;
--- a/devtools/client/shared/inplace-editor.js
+++ b/devtools/client/shared/inplace-editor.js
@@ -35,16 +35,17 @@ const CONTENT_TYPES = {
 };
 const MAX_POPUP_ENTRIES = 10;
 
 const FOCUS_FORWARD = Ci.nsIFocusManager.MOVEFOCUS_FORWARD;
 const FOCUS_BACKWARD = Ci.nsIFocusManager.MOVEFOCUS_BACKWARD;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://devtools/shared/event-emitter.js");
+const { findMostRelevantCssPropertyIndex } = require("./suggestion-picker");
 
 /**
  * Mark a span editable.  |editableField| will listen for the span to
  * be focused and create an InlineEditor to handle text input.
  * Changes will be committed when the InlineEditor's input is blurred
  * or dropped when the user presses escape.
  *
  * @param {Object} options
@@ -240,17 +241,17 @@ function InplaceEditor(options, event) {
 
   this.input.focus();
 
   if (typeof options.selectAll == "undefined" || options.selectAll) {
     this.input.select();
   }
 
   if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input.value == "") {
-    this._maybeSuggestCompletion(true);
+    this._maybeSuggestCompletion(false);
   }
 
   this.input.addEventListener("blur", this._onBlur, false);
   this.input.addEventListener("keypress", this._onKeyPress, false);
   this.input.addEventListener("input", this._onInput, false);
   this.input.addEventListener("dblclick", this._stopEventPropagation, false);
   this.input.addEventListener("click", this._stopEventPropagation, false);
   this.input.addEventListener("mousedown", this._stopEventPropagation, false);
@@ -944,17 +945,17 @@ InplaceEditor.prototype = {
     if (event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_BACK_SPACE ||
         event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_DELETE ||
         event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_LEFT ||
         event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_RIGHT) {
       if (this.popup && this.popup.isOpen) {
         this.popup.hidePopup();
       }
     } else if (!cycling && !event.metaKey && !event.altKey && !event.ctrlKey) {
-      this._maybeSuggestCompletion();
+      this._maybeSuggestCompletion(true);
     }
 
     if (this.multiline &&
         event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_RETURN &&
         event.shiftKey) {
       prevent = false;
     } else if (this._advanceChars(event.charCode, this.input.value,
                                   this.input.selectionStart) ||
@@ -1121,25 +1122,26 @@ InplaceEditor.prototype = {
     if (this.validate && this.input) {
       this.validate(this.input.value);
     }
   },
 
   /**
    * Handles displaying suggestions based on the current input.
    *
-   * @param {Boolean} noAutoInsert
-   *        true if you don't want to automatically insert the first suggestion
+   * @param {Boolean} autoInsert
+   *        Pass true to automatically insert the most relevant suggestion.
    */
-  _maybeSuggestCompletion: function(noAutoInsert) {
+  _maybeSuggestCompletion: function(autoInsert) {
     // Input can be null in cases when you intantaneously switch out of it.
     if (!this.input) {
       return;
     }
     let preTimeoutQuery = this.input.value;
+
     // Since we are calling this method from a keypress event handler, the
     // |input.value| does not include currently typed character. Thus we perform
     // this method async.
     this.doc.defaultView.setTimeout(() => {
       if (this._preventSuggestions) {
         this._preventSuggestions = false;
         return;
       }
@@ -1224,34 +1226,23 @@ InplaceEditor.prototype = {
           }
           if (startCheckQuery == null) {
             // This emit is mainly to make the test flow simpler.
             this.emit("after-suggest", "nothing to autocomplete");
             return;
           }
         }
       }
-      if (!noAutoInsert) {
-        list.some(item => {
-          if (startCheckQuery != null && item.startsWith(startCheckQuery)) {
-            input.value = query + item.slice(startCheckQuery.length) +
-                          input.value.slice(query.length);
-            input.setSelectionRange(query.length, query.length + item.length -
-                                                  startCheckQuery.length);
-            this._updateSize();
-            return true;
-          }
-        });
-      }
 
       if (!this.popup) {
         // This emit is mainly to make the test flow simpler.
         this.emit("after-suggest", "no popup");
         return;
       }
+
       let finalList = [];
       let length = list.length;
       for (let i = 0, count = 0; i < length && count < MAX_POPUP_ENTRIES; i++) {
         if (startCheckQuery != null && list[i].startsWith(startCheckQuery)) {
           count++;
           finalList.push({
             preLabel: startCheckQuery,
             label: list[i]
@@ -1261,33 +1252,50 @@ InplaceEditor.prototype = {
           // which would have started with query, assuming that list is sorted.
           break;
         } else if (startCheckQuery != null && list[i][0] > startCheckQuery[0]) {
           // We have crossed all possible matches alphabetically.
           break;
         }
       }
 
+      // Pick the best first suggestion from the provided list of suggestions.
+      let cssValues = finalList.map(item => item.label);
+      let mostRelevantIndex = findMostRelevantCssPropertyIndex(cssValues);
+
+      // Insert the most relevant item from the final list as the input value.
+      if (autoInsert && finalList[mostRelevantIndex]) {
+        let item = finalList[mostRelevantIndex].label;
+        input.value = query + item.slice(startCheckQuery.length) +
+                      input.value.slice(query.length);
+        input.setSelectionRange(query.length, query.length + item.length -
+                                              startCheckQuery.length);
+        this._updateSize();
+      }
+
+      // Display the list of suggestions if there are more than one.
       if (finalList.length > 1) {
-        // Calculate the offset for the popup to be opened.
-        let x = (this.input.selectionStart - startCheckQuery.length) *
-                this.inputCharWidth;
+        // Calculate the popup horizontal offset.
+        let indent = this.input.selectionStart - query.length;
+        let offset = indent * this.inputCharWidth;
+
+        // Select the most relevantItem if autoInsert is allowed
+        let selectedIndex = autoInsert ? mostRelevantIndex : -1;
+
+        // Open the suggestions popup.
         this.popup.setItems(finalList);
-        this.popup.openPopup(this.input, x);
-        if (noAutoInsert) {
-          this.popup.selectedIndex = -1;
-        }
+        this.popup.openPopup(this.input, offset, 0, selectedIndex);
       } else {
         this.popup.hidePopup();
       }
       // This emit is mainly for the purpose of making the test flow simpler.
       this.emit("after-suggest");
       this._doValidation();
     }, 0);
-  }
+  },
 };
 
 /**
  * Copy text-related styles from one element to another.
  */
 function copyTextStyles(from, to) {
   let win = from.ownerDocument.defaultView;
   let style = win.getComputedStyle(from);
--- a/devtools/client/shared/moz.build
+++ b/devtools/client/shared/moz.build
@@ -35,15 +35,16 @@ DevToolsModules(
     'l10n.js',
     'node-attribute-parser.js',
     'options-view.js',
     'output-parser.js',
     'poller.js',
     'prefs.js',
     'source-utils.js',
     'SplitView.jsm',
+    'suggestion-picker.js',
     'telemetry.js',
     'theme-switching.js',
     'theme.js',
     'undo.js',
     'view-source.js',
     'webgl-utils.js',
 )
new file mode 100644
--- /dev/null
+++ b/devtools/client/shared/suggestion-picker.js
@@ -0,0 +1,176 @@
+/* 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/. */
+
+"use strict";
+
+/**
+ * Allows to find the lowest ranking index in an index
+ * of suggestions, by comparing it to another array of "most relevant" items
+ * which has been sorted by relevance.
+ *
+ * Example usage:
+ *  let sortedBrowsers = ["firefox", "safari", "edge", "chrome"];
+ *  let myBrowsers = ["brave", "chrome", "firefox"];
+ *  let bestBrowserIndex = findMostRelevantIndex(myBrowsers, sortedBrowsers);
+ *  // returns "2", the index of firefox in myBrowsers array
+ *
+ * @param {Array} items
+ *        Array of items to compare against sortedItems.
+ * @param {Array} sortedItems
+ *        Array of sorted items that suggestions are evaluated against. Array
+ *        should be sorted by relevance, most relevant item first.
+ * @return {Number}
+ */
+function findMostRelevantIndex(items, sortedItems) {
+  if (!Array.isArray(items) || !Array.isArray(sortedItems)) {
+    throw new Error("Please provide valid items and sortedItems arrays.");
+  }
+
+  // If the items array is empty, no valid index can be found.
+  if (!items.length) {
+    return -1;
+  }
+
+  // Return 0 if no match was found in the suggestion list.
+  let bestIndex = 0;
+  let lowestIndex = Infinity;
+  items.forEach((item, i) => {
+    let index = sortedItems.indexOf(item);
+    if (index !== -1 && index <= lowestIndex) {
+      lowestIndex = index;
+      bestIndex = i;
+    }
+  });
+
+  return bestIndex;
+}
+
+/**
+ * Top 100 CSS property names sorted by relevance, most relevant first.
+ *
+ * List based on the one used by Chrome devtools :
+ * https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
+ * WebKit/Source/devtools/front_end/sdk/CSSMetadata.js&q=CSSMetadata&
+ * sq=package:chromium&type=cs&l=676
+ *
+ * The data is a mix of https://www.chromestatus.com/metrics/css and usage
+ * metrics from popular sites collected via https://gist.github.com/NV/3751436
+ *
+ * @type {Array}
+ */
+const SORTED_CSS_PROPERTIES = [
+  "width",
+  "margin",
+  "height",
+  "padding",
+  "font-size",
+  "border",
+  "display",
+  "position",
+  "text-align",
+  "background",
+  "background-color",
+  "top",
+  "font-weight",
+  "color",
+  "overflow",
+  "font-family",
+  "margin-top",
+  "float",
+  "opacity",
+  "cursor",
+  "left",
+  "text-decoration",
+  "background-image",
+  "right",
+  "line-height",
+  "margin-left",
+  "visibility",
+  "margin-bottom",
+  "padding-top",
+  "z-index",
+  "margin-right",
+  "background-position",
+  "vertical-align",
+  "padding-left",
+  "background-repeat",
+  "border-bottom",
+  "padding-right",
+  "border-top",
+  "padding-bottom",
+  "clear",
+  "white-space",
+  "bottom",
+  "border-color",
+  "max-width",
+  "border-radius",
+  "border-right",
+  "outline",
+  "border-left",
+  "font-style",
+  "content",
+  "min-width",
+  "min-height",
+  "box-sizing",
+  "list-style",
+  "border-width",
+  "box-shadow",
+  "font",
+  "border-collapse",
+  "text-shadow",
+  "text-indent",
+  "border-style",
+  "max-height",
+  "text-overflow",
+  "background-size",
+  "text-transform",
+  "zoom",
+  "list-style-type",
+  "border-spacing",
+  "word-wrap",
+  "overflow-y",
+  "transition",
+  "border-top-color",
+  "border-bottom-color",
+  "border-top-right-radius",
+  "letter-spacing",
+  "border-top-left-radius",
+  "border-bottom-left-radius",
+  "border-bottom-right-radius",
+  "overflow-x",
+  "pointer-events",
+  "border-right-color",
+  "transform",
+  "border-top-width",
+  "border-bottom-width",
+  "border-right-width",
+  "direction",
+  "animation",
+  "border-left-color",
+  "clip",
+  "border-left-width",
+  "table-layout",
+  "src",
+  "resize",
+  "word-break",
+  "background-clip",
+  "transform-origin",
+  "font-variant",
+  "filter",
+  "quotes",
+  "word-spacing"
+];
+
+/**
+ * Helper to find the most relevant CSS property name in a provided array.
+ *
+ * @param items {Array}
+ *              Array of CSS property names.
+ */
+function findMostRelevantCssPropertyIndex(items) {
+  return findMostRelevantIndex(items, SORTED_CSS_PROPERTIES);
+}
+
+exports.findMostRelevantIndex = findMostRelevantIndex;
+exports.findMostRelevantCssPropertyIndex = findMostRelevantCssPropertyIndex;
new file mode 100644
--- /dev/null
+++ b/devtools/client/shared/test/unit/test_suggestion-picker.js
@@ -0,0 +1,149 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Test the suggestion-picker helper methods.
+ */
+const {require} = Components.utils.import("resource://devtools/shared/Loader.jsm", {});
+const {
+  findMostRelevantIndex,
+  findMostRelevantCssPropertyIndex
+} = require("devtools/client/shared/suggestion-picker");
+
+/**
+ * Run all tests defined below.
+ */
+function run_test() {
+  ensureMostRelevantIndexProvidedByHelperFunction();
+  ensureMostRelevantIndexProvidedByClassMethod();
+  ensureErrorThrownWithInvalidArguments();
+}
+
+/**
+ * Generic test data.
+ */
+const TEST_DATA = [
+  {
+    // Match in sortedItems array.
+    items: ["chrome", "edge", "firefox"],
+    sortedItems: ["firefox", "chrome", "edge"],
+    expectedIndex: 2
+  }, {
+    // No match in sortedItems array.
+    items: ["apple", "oranges", "banana"],
+    sortedItems: ["kiwi", "pear", "peach"],
+    expectedIndex: 0
+  }, {
+    // Empty items array.
+    items: [],
+    sortedItems: ["empty", "arrays", "can't", "have", "relevant", "indexes"],
+    expectedIndex: -1
+  }
+];
+
+function ensureMostRelevantIndexProvidedByHelperFunction() {
+  do_print("Running ensureMostRelevantIndexProvidedByHelperFunction()");
+
+  for (let testData of TEST_DATA) {
+    let { items, sortedItems, expectedIndex } = testData;
+    let mostRelevantIndex = findMostRelevantIndex(items, sortedItems);
+    strictEqual(mostRelevantIndex, expectedIndex);
+  }
+}
+
+/**
+ * CSS properties test data.
+ */
+const CSS_TEST_DATA = [
+  {
+    items: [
+      "backface-visibility",
+      "background",
+      "background-attachment",
+      "background-blend-mode",
+      "background-clip",
+      "background-color",
+      "background-image",
+      "background-origin",
+      "background-position",
+      "background-repeat"
+    ],
+    expectedIndex: 1
+  },
+  {
+    items: [
+      "caption-side",
+      "clear",
+      "clip",
+      "clip-path",
+      "clip-rule",
+      "color",
+      "color-interpolation",
+      "color-interpolation-filters",
+      "content",
+      "counter-increment"
+    ],
+    expectedIndex: 5
+  },
+  {
+    items: [
+      "direction",
+      "display",
+      "dominant-baseline"
+    ],
+    expectedIndex: 1
+  },
+  {
+    items: [
+      "object-fit",
+      "object-position",
+      "offset-block-end",
+      "offset-block-start",
+      "offset-inline-end",
+      "offset-inline-start",
+      "opacity",
+      "order",
+      "orphans",
+      "outline"
+    ],
+    expectedIndex: 6
+  },
+  {
+    items: [
+      "white-space",
+      "widows",
+      "width",
+      "will-change",
+      "word-break",
+      "word-spacing",
+      "word-wrap",
+      "writing-mode"
+    ],
+    expectedIndex: 2
+  }
+];
+
+function ensureMostRelevantIndexProvidedByClassMethod() {
+  do_print("Running ensureMostRelevantIndexProvidedByClassMethod()");
+
+  for (let testData of CSS_TEST_DATA) {
+    let { items, expectedIndex } = testData;
+    let mostRelevantIndex = findMostRelevantCssPropertyIndex(items);
+    strictEqual(mostRelevantIndex, expectedIndex);
+  }
+}
+
+function ensureErrorThrownWithInvalidArguments() {
+  do_print("Running ensureErrorThrownWithInvalidTypeArgument()");
+
+  let expectedError = "Please provide valid items and sortedItems arrays.";
+  // No arguments passed.
+  throws(() => findMostRelevantIndex(), expectedError);
+  // Invalid arguments passed.
+  throws(() => findMostRelevantIndex([]), expectedError);
+  throws(() => findMostRelevantIndex(null, []), expectedError);
+  throws(() => findMostRelevantIndex([], "string"), expectedError);
+  throws(() => findMostRelevantIndex("string", []), expectedError);
+}
--- a/devtools/client/shared/test/unit/xpcshell.ini
+++ b/devtools/client/shared/test/unit/xpcshell.ini
@@ -11,11 +11,12 @@ skip-if = toolkit == 'android' || toolki
 [test_bezierCanvas.js]
 [test_cubicBezier.js]
 [test_escapeCSSComment.js]
 [test_parseDeclarations.js]
 [test_parsePseudoClassesAndAttributes.js]
 [test_parseSingleValue.js]
 [test_rewriteDeclarations.js]
 [test_source-utils.js]
+[test_suggestion-picker.js]
 [test_undoStack.js]
 [test_VariablesView_filtering-without-controller.js]
 [test_VariablesView_getString_promise.js]