Bug 1260419 - ruleview CSS autocomplete: more suggestions & better sorting;r=pbro draft
authorJulian Descottes <jdescottes@mozilla.com>
Thu, 07 Apr 2016 00:43:30 +0200
changeset 363336 5a4842974672bebc7427a546b4e4014201147465
parent 363287 bf7a7e5b7a7d45f8b6b5d135d9f8453a6b60d7d7
child 363337 516307dbc3d6142e107a10e9c3fb44ee93acc551
push id17174
push userjdescottes@mozilla.com
push dateWed, 04 May 2016 14:47:16 +0000
reviewerspbro
bugs1260419
milestone49.0a1
Bug 1260419 - ruleview CSS autocomplete: more suggestions & better sorting;r=pbro Usability changes to the ruleview autocomplete: - max suggestions is now capped to 500 to make sure as many suggestions as possible as possible are displayed - vendor-prefixed properties are moved to the end of the list - !important is no longer the first suggested item MozReview-Commit-ID: AOfHyqS3n8s
devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_01.js
devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_02.js
devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js
devtools/client/inspector/rules/test/browser_rules_completion-existing-property_02.js
devtools/client/inspector/rules/test/browser_rules_completion-new-property_01.js
devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js
devtools/client/shared/inplace-editor.js
devtools/client/themes/common.css
--- a/devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_01.js
+++ b/devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_01.js
@@ -33,39 +33,39 @@ const TEST_DATA = [
   ["e", "style", 5, 5, false],
   ["=", "style=", 6, 6, false],
   ["\"", "style=\"", 7, 7, false],
   ["d", "style=\"display", 8, 14, true],
   ["VK_TAB", "style=\"display", 14, 14, true],
   ["VK_TAB", "style=\"dominant-baseline", 24, 24, true],
   ["VK_TAB", "style=\"direction", 16, 16, true],
   ["click_1", "style=\"display", 14, 14, false],
-  [":", "style=\"display:-moz-box", 15, 23, true],
+  [":", "style=\"display:block", 15, 20, true],
   ["n", "style=\"display:none", 16, 19, false],
   ["VK_BACK_SPACE", "style=\"display:n", 16, 16, false],
   ["VK_BACK_SPACE", "style=\"display:", 15, 15, false],
-  [" ", "style=\"display: -moz-box", 16, 24, true],
-  [" ", "style=\"display:  -moz-box", 17, 25, true],
+  [" ", "style=\"display: block", 16, 21, true],
+  [" ", "style=\"display:  block", 17, 22, true],
   ["i", "style=\"display:  inherit", 18, 24, true],
   ["VK_RIGHT", "style=\"display:  inherit", 24, 24, false],
   [";", "style=\"display:  inherit;", 25, 25, false],
   [" ", "style=\"display:  inherit; ", 26, 26, false],
   [" ", "style=\"display:  inherit;  ", 27, 27, false],
   ["VK_LEFT", "style=\"display:  inherit;  ", 26, 26, false],
   ["c", "style=\"display:  inherit; color ", 27, 31, true],
   ["VK_RIGHT", "style=\"display:  inherit; color ", 31, 31, false],
   [" ", "style=\"display:  inherit; color  ", 32, 32, false],
   ["c", "style=\"display:  inherit; color c ", 33, 33, false],
   ["VK_BACK_SPACE", "style=\"display:  inherit; color  ", 32, 32, false],
   [":", "style=\"display:  inherit; color :aliceblue ", 33, 42, true],
   ["c", "style=\"display:  inherit; color :cadetblue ", 34, 42, true],
   ["VK_DOWN", "style=\"display:  inherit; color :chartreuse ", 34, 43, true],
   ["VK_RIGHT", "style=\"display:  inherit; color :chartreuse ", 43, 43, false],
-  [" ", "style=\"display:  inherit; color :chartreuse !important; ",
-   44, 55, true],
+  [" ", "style=\"display:  inherit; color :chartreuse aliceblue ",
+   44, 53, true],
   ["!", "style=\"display:  inherit; color :chartreuse !important; ",
    45, 55, false],
   ["VK_RIGHT", "style=\"display:  inherit; color :chartreuse !important; ",
    55, 55, false],
   ["VK_RETURN", "style=\"display:  inherit; color :chartreuse !important;\"",
    -1, -1, false]
 ];
 
--- a/devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_02.js
+++ b/devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_02.js
@@ -71,19 +71,20 @@ const TEST_DATA_SINGLE = [
 const TEST_DATA_INNER = [
   ["s", "s", 1, 1, false],
   ["t", "st", 2, 2, false],
   ["y", "sty", 3, 3, false],
   ["l", "styl", 4, 4, false],
   ["e", "style", 5, 5, false],
   ["=", "style=", 6, 6, false],
   ["\"", "style=\"", 7, 7, false],
-  ["b", "style=\"background", 8, 17, true],
+  ["b", "style=\"border", 8, 13, true],
+  ["a", "style=\"background", 9, 17, true],
   ["VK_RIGHT", "style=\"background", 17, 17, false],
-  [":", "style=\"background:-moz-element", 18, 30, true],
+  [":", "style=\"background:aliceblue", 18, 27, true],
   ["u", "style=\"background:unset", 19, 23, true],
   ["r", "style=\"background:url", 20, 21, false],
   ["l", "style=\"background:url", 21, 21, false],
   ["(", "style=\"background:url(", 22, 22, false],
   ["'", "style=\"background:url('", 23, 23, false],
   ["1", "style=\"background:url('1", 24, 24, false],
   ["'", "style=\"background:url('1'", 25, 25, false],
   [")", "style=\"background:url('1')", 26, 26, false],
--- a/devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js
+++ b/devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js
@@ -2,28 +2,26 @@
 /* Any copyright is dedicated to the Public Domain.
  http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 // Tests that CSS property names are autocompleted and cycled correctly when
 // editing an existing property in the rule view.
 
-const MAX_ENTRIES = 10;
-
 // format :
 //  [
 //    what key to press,
 //    expected input box value after keypress,
 //    selectedIndex of the popup,
 //    total items in the popup
 //  ]
 var testData = [
   ["VK_RIGHT", "font", -1, 0],
-  ["-", "font-size", 4, MAX_ENTRIES],
+  ["-", "font-size", 4, 17],
   ["f", "font-family", 0, 2],
   ["VK_BACK_SPACE", "font-f", -1, 0],
   ["VK_BACK_SPACE", "font-", -1, 0],
   ["VK_BACK_SPACE", "font", -1, 0],
   ["VK_BACK_SPACE", "fon", -1, 0],
   ["VK_BACK_SPACE", "fo", -1, 0],
   ["VK_BACK_SPACE", "f", -1, 0],
   ["VK_BACK_SPACE", "", -1, 0],
@@ -40,17 +38,17 @@ var testData = [
   ["VK_BACK_SPACE", "dis", -1, 0],
   ["VK_BACK_SPACE", "di", -1, 0],
   ["VK_BACK_SPACE", "d", -1, 0],
   ["VK_BACK_SPACE", "", -1, 0],
   ["VK_HOME", "", -1, 0],
   ["VK_END", "", -1, 0],
   ["VK_PAGE_UP", "", -1, 0],
   ["VK_PAGE_DOWN", "", -1, 0],
-  ["f", "filter", 3, MAX_ENTRIES],
+  ["f", "font-size", 19, 32],
   ["i", "filter", 3, 4],
   ["VK_LEFT", "filter", -1, 0],
   ["VK_LEFT", "filter", -1, 0],
   ["i", "fiilter", -1, 0],
   ["VK_ESCAPE", null, -1, 0],
 ];
 
 const TEST_URI = "<h1 style='font: 24px serif'>Header</h1>";
--- a/devtools/client/inspector/rules/test/browser_rules_completion-existing-property_02.js
+++ b/devtools/client/inspector/rules/test/browser_rules_completion-existing-property_02.js
@@ -17,17 +17,17 @@
 //    expect ruleview-changed
 //  ]
 var testData = [
   ["b", {}, "beige", 0, 8, true],
   ["l", {}, "black", 0, 4, true],
   ["VK_DOWN", {}, "blanchedalmond", 1, 4, true],
   ["VK_DOWN", {}, "blue", 2, 4, true],
   ["VK_RIGHT", {}, "blue", -1, 0, false],
-  [" ", {}, "blue !important", 0, 10, true],
+  [" ", {}, "blue aliceblue", 0, 158, true],
   ["!", {}, "blue !important", 0, 0, true],
   ["VK_BACK_SPACE", {}, "blue !", -1, 0, true],
   ["VK_BACK_SPACE", {}, "blue ", -1, 0, true],
   ["VK_BACK_SPACE", {}, "blue", -1, 0, true],
   ["VK_TAB", {shiftKey: true}, "color", -1, 0, true],
   ["VK_BACK_SPACE", {}, "", -1, 0, false],
   ["d", {}, "display", 1, 3, false],
   ["VK_TAB", {}, "blue", -1, 0, true],
--- a/devtools/client/inspector/rules/test/browser_rules_completion-new-property_01.js
+++ b/devtools/client/inspector/rules/test/browser_rules_completion-new-property_01.js
@@ -2,18 +2,16 @@
 /* Any copyright is dedicated to the Public Domain.
  http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 // Tests that CSS property names are autocompleted and cycled correctly when
 // creating a new property in the rule view.
 
-const MAX_ENTRIES = 10;
-
 // format :
 //  [
 //    what key to press,
 //    expected input box value after keypress,
 //    selectedIndex of the popup,
 //    total items in the popup
 //  ]
 var testData = [
@@ -26,17 +24,17 @@ var testData = [
   ["VK_UP", "display", 1, 3],
   ["VK_BACK_SPACE", "d", -1, 0],
   ["i", "display", 1, 2],
   ["s", "display", -1, 0],
   ["VK_BACK_SPACE", "dis", -1, 0],
   ["VK_BACK_SPACE", "di", -1, 0],
   ["VK_BACK_SPACE", "d", -1, 0],
   ["VK_BACK_SPACE", "", -1, 0],
-  ["f", "filter", 3, MAX_ENTRIES],
+  ["f", "font-size", 19, 32],
   ["i", "filter", 3, 4],
   ["VK_ESCAPE", null, -1, 0],
 ];
 
 const TEST_URI = "<h1 style='border: 1px solid red'>Header</h1>";
 
 add_task(function* () {
   yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
--- a/devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js
+++ b/devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js
@@ -13,33 +13,33 @@
 //    modifers,
 //    expected input box value after keypress,
 //    selectedIndex of the popup,
 //    total items in the popup,
 //    expect ruleview-changed
 //  ]
 var testData = [
   ["d", {}, "display", 1, 3, false],
-  ["VK_TAB", {}, "", -1, 10, true],
-  ["VK_DOWN", {}, "-moz-box", 0, 10, true],
+  ["VK_TAB", {}, "", -1, 41, true],
+  ["VK_DOWN", {}, "block", 0, 41, true],
   ["n", {}, "none", -1, 0, true],
   ["VK_TAB", {shiftKey: true}, "display", -1, 0, true],
   ["VK_BACK_SPACE", {}, "", -1, 0, false],
-  ["o", {}, "opacity", 6, 10, false],
+  ["o", {}, "overflow", 13, 16, false],
   ["u", {}, "outline", 0, 5, false],
   ["VK_DOWN", {}, "outline-color", 1, 5, false],
   ["VK_TAB", {}, "none", -1, 0, true],
   ["r", {}, "rebeccapurple", 0, 6, true],
   ["VK_DOWN", {}, "red", 1, 6, true],
   ["VK_DOWN", {}, "rgb", 2, 6, true],
   ["VK_DOWN", {}, "rgba", 3, 6, true],
   ["VK_DOWN", {}, "rosybrown", 4, 6, true],
   ["VK_DOWN", {}, "royalblue", 5, 6, true],
   ["VK_RIGHT", {}, "royalblue", -1, 0, false],
-  [" ", {}, "royalblue !important", 0, 10, true],
+  [" ", {}, "royalblue aliceblue", 0, 159, true],
   ["!", {}, "royalblue !important", 0, 0, true],
   ["VK_ESCAPE", {}, null, -1, 0, true]
 ];
 
 const TEST_URI = `
   <style type="text/css">
     h1 {
       border: 1px solid red;
--- a/devtools/client/shared/inplace-editor.js
+++ b/devtools/client/shared/inplace-editor.js
@@ -28,17 +28,20 @@ const Services = require("Services");
 
 const HTML_NS = "http://www.w3.org/1999/xhtml";
 const CONTENT_TYPES = {
   PLAIN_TEXT: 0,
   CSS_VALUE: 1,
   CSS_MIXED: 2,
   CSS_PROPERTY: 3,
 };
-const MAX_POPUP_ENTRIES = 10;
+
+// The limit of 500 autocomplete suggestions should not be reached but is kept
+// for safety.
+const MAX_POPUP_ENTRIES = 500;
 
 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");
 
@@ -723,16 +726,18 @@ InplaceEditor.prototype = {
       if (mid !== null) {
         return {
           value: first + mid + last,
           start: start,
           end: start + mid.length
         };
       }
     }
+
+    return null;
   },
 
   /**
    * Increment the property value for numbers.
    *
    * @param {String} rawValue
    *        Raw value to increment.
    * @param {Number} increment
@@ -1332,39 +1337,54 @@ 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);
+      // Sort items starting with [a-z0-9] first, to make sure vendor-prefixed
+      // values and "!important" are suggested only after standard values.
+      finalList.sort((item1, item2) => {
+        // Get the expected alphabetical comparison between the items.
+        let comparison = item1.label.localeCompare(item2.label);
+        if (/^\w/.test(item1.label) != /^\w/.test(item2.label)) {
+          // One starts with [a-z0-9], one does not: flip the comparison.
+          comparison = -1 * comparison;
+        }
+        return comparison;
+      });
+
+      let index = 0;
+      if (startCheckQuery) {
+        // Only select a "best" suggestion when the user started a query.
+        let cssValues = finalList.map(item => item.label);
+        index = findMostRelevantCssPropertyIndex(cssValues);
+      }
 
       // Insert the most relevant item from the final list as the input value.
-      if (autoInsert && finalList[mostRelevantIndex]) {
-        let item = finalList[mostRelevantIndex].label;
+      if (autoInsert && finalList[index]) {
+        let item = finalList[index].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 popup horizontal offset.
         let indent = this.input.selectionStart - query.length;
         let offset = indent * this.inputCharDimensions.width;
         offset = this._isSingleLine() ? offset : 0;
 
         // Select the most relevantItem if autoInsert is allowed
-        let selectedIndex = autoInsert ? mostRelevantIndex : -1;
+        let selectedIndex = autoInsert ? index : -1;
 
         // Open the suggestions popup.
         this.popup.setItems(finalList);
         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.
--- a/devtools/client/themes/common.css
+++ b/devtools/client/themes/common.css
@@ -37,27 +37,33 @@
 /* Autocomplete Popup */
 
 .devtools-autocomplete-popup {
   -moz-appearance: none !important;
   box-shadow: 0 1px 0 hsla(209,29%,72%,.25) inset;
   background-color: transparent;
   border-radius: 3px;
   overflow-x: hidden;
-  max-height: 40rem;
+  max-height: 20rem;
 }
 
 :root[platform="linux"] .devtools-autocomplete-popup {
-  max-height: 32rem;
+  /* Root font size is bigger on Linux, adjust rem-based values. */
+  max-height: 16rem;
 }
 
 .devtools-autocomplete-listbox {
   -moz-appearance: none !important;
   background-color: transparent;
   border-width: 0px !important;
+  margin: 0;
+}
+
+.devtools-autocomplete-listbox > scrollbox {
+  padding: 2px;
 }
 
 .devtools-autocomplete-listbox > richlistitem,
 .devtools-autocomplete-listbox > richlistitem[selected] {
   width: 100%;
   background-color: transparent;
   border-radius: 4px;
 }