Bug 1524718 - Replace context.autofillValue with result.autofill, and autofill results when they're selected. r=mak

We should replace the context.autofillValue property with a result.autofill property. When the view selects results, it already notifies the input about it by calling input.setValueFromResult(). So we can modify setValueFromResult to check for the presence of result.autofill and thereby get autofill "for free". (The one place where the view doesn't call input.setValueFromResult() when a result is selected is when it selects the preselected result, so this patch adds that.)

result.autofill is an object: { value, selectionStart, selectionEnd }

This is going to help me implement bug 1521702.

One potentially cool thing about doing autofill this way is that any result can now trigger autofill, not only the heuristic result, and do it easily. Of course the user isn't typing when they select a non-heuristic result, so it's probably not fair to call that "autofill", but the result can trigger the selection aspect of autofill. As one example, that might be interesting for search suggestions: Type "foo", key down to the "foobar" suggestion, and the "bar" substring is automatically selected.

Differential Revision: https://phabricator.services.mozilla.com/D18618

--HG--
extra : moz-landing-system : lando

browser/components/urlbar/UrlbarController.jsm

@@ -136,9 +136,6 @@ class UrlbarController {
     }
 
     if (queryContext.lastResultCount == 0) {
-      if (queryContext.autofillValue) {
-        this.input.autofill(queryContext.autofillValue);
-      }
       // The first time we receive results try to connect to the heuristic
       // result.
       this.speculativeConnect(queryContext, 0, "resultsadded");
@@ -273,7 +270,7 @@ class UrlbarController {
     switch (reason) {
       case "resultsadded": {
         // We should connect to an heuristic result, if it exists.
-        if (resultIndex == 0 && (context.preselected || context.autofillValue)) {
+        if ((resultIndex == 0 && context.preselected) || result.autofill) {
           if (result.type == UrlbarUtils.RESULT_TYPE.SEARCH) {
             // Speculative connect only if search suggestions are enabled.
             if (UrlbarPrefs.get("suggest.searches") &&
@@ -281,7 +278,7 @@ class UrlbarController {
               let engine = Services.search.defaultEngine;
               UrlbarUtils.setupSpeculativeConnection(engine, this.browserWindow);
             }
-          } else if (context.autofillValue) {
+          } else if (result.autofill) {
             UrlbarUtils.setupSpeculativeConnection(url, this.browserWindow);
           }
         }

browser/components/urlbar/UrlbarInput.jsm

@@ -367,29 +367,14 @@ class UrlbarInput {
    * @param {UrlbarResult} result The result that was selected.
    */
   setValueFromResult(result) {
-    let val;
+    if (result.autofill) {
-    switch (result.type) {
+      this._setValueFromResultAutofill(result);
-      case UrlbarUtils.RESULT_TYPE.SEARCH:
+    } else {
-        val = result.payload.suggestion || result.payload.query;
+      this.value = this._valueFromResultPayload(result);
-        break;
-      case UrlbarUtils.RESULT_TYPE.OMNIBOX:
-        val = result.payload.content;
-        break;
-      default: {
-        val = result.payload.url;
-        let uri;
-        try {
-          uri = Services.io.newURI(val);
-        } catch (ex) {}
-        if (uri) {
-          val = this.window.losslessDecodeURI(uri);
-        }
-        break;
-      }
     }
-    this.value = val;
+
     // Also update userTypedValue. See bug 287996.
-    this.window.gBrowser.userTypedValue = val;
+    this.window.gBrowser.userTypedValue = this.value;
 
     // The value setter clobbers the actiontype attribute, so update this after that.
     switch (result.type) {
@@ -482,27 +467,6 @@ class UrlbarInput {
     this.textbox.classList.remove("hidden-focus");
   }
 
-  /**
-   * Autofills the given value into the input.  That is, sets the input's value
-   * to the given value and selects the portion of the new value that comes
-   * after the current value.  The given value should therefore start with the
-   * input's current value.  If it doesn't, then this method doesn't do
-   * anything.
-   *
-   * @param {string} value
-   *   The value to autofill.
-   */
-  autofill(value) {
-    if (!value.toLocaleLowerCase()
-        .startsWith(this.textValue.toLocaleLowerCase())) {
-      return;
-    }
-    let len = this.textValue.length;
-    this.value = this.textValue + value.substring(len);
-    this.selectionStart = len;
-    this.selectionEnd = value.length;
-  }
-
   // Getters and Setters below.
 
   get focused() {
@@ -547,6 +511,30 @@ class UrlbarInput {
 
   // Private methods below.
 
+  _setValueFromResultAutofill(result) {
+    this.value = result.autofill.value;
+    this.selectionStart = result.autofill.selectionStart;
+    this.selectionEnd = result.autofill.selectionEnd;
+  }
+
+  _valueFromResultPayload(result) {
+    switch (result.type) {
+      case UrlbarUtils.RESULT_TYPE.SEARCH:
+        return result.payload.suggestion || result.payload.query;
+      case UrlbarUtils.RESULT_TYPE.OMNIBOX:
+        return result.payload.content;
+    }
+
+    try {
+      let uri = Services.io.newURI(result.payload.url);
+      if (uri) {
+        return this.window.losslessDecodeURI(uri);
+      }
+    } catch (ex) {}
+
+    return "";
+  }
+
   _updateTextOverflow() {
     if (!this._overflowing) {
       this.removeAttribute("textoverflow");

browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm

@@ -197,10 +197,19 @@ function convertResultToMatches(context, result, urls) {
     if (!match) {
       continue;
     }
-    // Manage autofillValue and preselected properties for the first match.
+    // Manage autofill and preselected properties for the first match.
     if (i == 0) {
       if (style.includes("autofill") && result.defaultIndex == 0) {
-        context.autofillValue = result.getValueAt(i);
+        let autofillValue = result.getValueAt(i);
+        if (autofillValue.toLocaleLowerCase()
+            .startsWith(context.searchString.toLocaleLowerCase())) {
+          match.autofill = {
+            value: context.searchString +
+                   autofillValue.substring(context.searchString.length),
+            selectionStart: context.searchString.length,
+            selectionEnd: autofillValue.length,
+          };
+        }
       }
       if (style.includes("heuristic")) {
         context.preselected = true;

browser/components/urlbar/UrlbarView.jsm

@@ -156,6 +156,14 @@ class UrlbarView {
     this._rows.appendChild(fragment);
 
     this._openPanel();
+
+    if (queryContext.preselected &&
+        queryContext.lastResultCount == 0 &&
+        queryContext.results.length > 0) {
+      // If we just selected the first result above, then tell the input about
+      // it because it may be an autofill result.
+      this.input.setValueFromResult(queryContext.results[0]);
+    }
   }
 
   /**

browser/components/urlbar/tests/UrlbarTestUtils.jsm

@@ -337,7 +337,7 @@ class UrlbarAbstraction {
       let context = await this.urlbar.lastQueryContextPromise;
       details.url = (UrlbarUtils.getUrlFromResult(context.results[index])).url;
       details.type = context.results[index].type;
-      details.autofill = index == 0 && context.autofillValue;
+      details.autofill = index == 0 && context.results[index].autofill;
       details.image = element.getElementsByClassName("urlbarView-favicon")[0].src;
       details.title = context.results[index].title;
       let actions = element.getElementsByClassName("urlbarView-action");

browser/components/urlbar/tests/browser/browser_UrlbarInput_autofill.js

@@ -2,34 +2,24 @@
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 /**
- * Tests the UrlbarInput.autofill() method.  It needs to be tested in the actual
+ * Tests the autofill functionality of UrlbarInput.
- * browser because UrlbarInput relies on the trimURL() function defined in the
- * browser window's global scope, and this test hits the path that calls it.
  */
 
 "use strict";
 
 add_task(async function test() {
-  let tests = [
+  gURLBar.setValueFromResult({
-    // [initial value, autofill value, expected selected substring in new value]
+    autofill: {
-    ["foo", "foobar", "bar"],
+      value: "foobar",
-    ["FOO", "foobar", "bar"],
+      selectionStart: "foo".length,
-    ["fOo", "foobar", "bar"],
+      selectionEnd: "foobar".length,
-    ["foo", "quuxbar", ""],
+    },
-    ["FOO", "quuxbar", ""],
+    type: UrlbarUtils.RESULT_TYPE.URL,
-    ["fOo", "quuxbar", ""],
+  });
-  ];
+  Assert.equal(gURLBar.value, "foobar",
-
+    "The input value should be correct");
-  gURLBar.focus();
+  Assert.equal(gURLBar.selectionStart, "foo".length,
-  for (let [initial, autofill, expectedSelected] of tests) {
+    "The start of the selection should be correct");
-    gURLBar.value = initial;
+  Assert.equal(gURLBar.selectionEnd, "foobar".length,
-    gURLBar.autofill(autofill);
+    "The end of the selection should be correct");
-    let expectedValue = initial + expectedSelected;
-    Assert.equal(gURLBar.value, expectedValue,
-      "The input value should be correct");
-    Assert.equal(gURLBar.selectionStart, initial.length,
-      "The start of the selection should be correct");
-    Assert.equal(gURLBar.selectionEnd, expectedValue.length,
-      "The end of the selection should be correct");
-  }
 });

browser/components/urlbar/tests/unit/test_UrlbarController_unit.js

@@ -205,45 +205,3 @@ add_task(function test_receiveResults() {
 
   sandbox.resetHistory();
 });
-
-add_task(async function test_autofillValue() {
-  // Ensure the controller doesn't have any previous queries.
-  delete controller._lastQueryContext;
-
-  // Stub the controller's input so we can tell whether input.autofill() is
-  // called.
-  let input = {
-    autofill: sandbox.stub(),
-  };
-  controller.input = input;
-
-  const context = createContext();
-  controller.startQuery(context);
-
-  // Set autofillValue and call receiveResults().
-  context.autofillValue = "test";
-  context.results = [
-    new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
-                     UrlbarUtils.RESULT_SOURCE.TABS,
-                     { url: "http://example.com/1" }),
-  ];
-  controller.receiveResults(context);
-
-  Assert.equal(input.autofill.callCount, 1,
-    "Should have called input.autofill() one time");
-  Assert.deepEqual(input.autofill.args[0], ["test"],
-    "Should have called input.autofill() with context.autofillValue");
-
-  // Call receiveResults() again with more results.
-  context.results.push(
-    new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH,
-                     UrlbarUtils.RESULT_SOURCE.TABS,
-                     { url: "http://example.com/2" }),
-  );
-  controller.receiveResults(context);
-
-  Assert.equal(input.autofill.callCount, 1,
-    "Should not have called input.autofill() again");
-
-  sandbox.resetHistory();
-});

browser/docs/AddressBar.rst

@@ -75,8 +75,6 @@ It is augmented as it progresses through the system, with various information:
              // RESULT_SOURCE.*, that can be returned by the model.
 
     // Properties added by the Model.
-    autofillValue; // {string} the text value that should be autofilled in the
-                   // input, if any.
     preselected; // {boolean} whether the first result should be preselected.
     results; // {array} list of UrlbarResult objects.
     tokens; // {array} tokens extracted from the searchString, each token is an
@@ -375,6 +373,12 @@ properties, supported by all of the results.
     title: {string} A title that may be used as a label for this result.
     icon: {string} Url of an icon for this result.
     payload: {object} Object containing properties for the specific RESULT_TYPE.
+    autofill: {object} An object describing the text that should be
+              autofilled in the input when the result is selected, if any.
+    autofill.value: {string} The autofill value.
+    autofill.selectionStart: {integer} The first index in the autofill
+                             selection.
+    autofill.selectionEnd: {integer} The last index in the autofill selection.
   }
 
 The following RESULT_TYPEs are supported: