Bug 1538117 - Fix autofill when typing the last character of an autofilled value. r=mak
authorDrew Willcoxon <adw@mozilla.com>
Fri, 22 Mar 2019 20:34:44 +0000
changeset 465775 565644972ec23f4a8e54b60efc793b0e23e6d84a
parent 465774 0539b9847dd53c4cdb75108b4e580bc518642690
child 465776 b93d00aab64923e7dbcebd06f422c50f4887e888
push id35746
push usershindli@mozilla.com
push dateSat, 23 Mar 2019 09:46:24 +0000
treeherdermozilla-central@02b7484f316b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1538117
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1538117 - Fix autofill when typing the last character of an autofilled value. r=mak _autofillValueOnInput correctly uses the placeholder string as the autofilled value, but it incorrectly uses _lastSearchString as the current input value. _lastSearchString at that point is -- yes -- the previous search string, not what the user has just typed. So when _autofillValueOnInput sets selectionStart to _lastSesarchString.length, the length is one char less than what it should be. But why does that mess up only the last char typed and not every char? Because when the first result comes in, we correctly autofill it. It's only when the first result is not an autofill result that the incorrect placeholder autofill sticks around -- e.g., just after you type the last char in an @ alias. This patch just gets rid of _autofillValueOnInput and inlines the body in _maybeAutofillOnInput. Differential Revision: https://phabricator.services.mozilla.com/D24551
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_autoFill_typed.js
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -469,17 +469,18 @@ class UrlbarInput {
     } else {
       // For autofilled results, the value that should be canonized is not the
       // autofilled value but the value that the user typed.
       canonizedUrl = this._maybeCanonizeURL(event, result.autofill ?
                        this._lastSearchString : this.textValue);
       if (canonizedUrl) {
         this.value = canonizedUrl;
       } else if (result.autofill) {
-        this._autofillValue(result.autofill);
+        let { value, selectionStart, selectionEnd } = result.autofill;
+        this._autofillValue(value, selectionStart, selectionEnd);
       } else {
         this.value = this._getValueFromResult(result);
       }
     }
     this._resultForCurrentValue = result;
 
     // Also update userTypedValue. See bug 287996.
     this.window.gBrowser.userTypedValue = this.value;
@@ -713,18 +714,25 @@ class UrlbarInput {
     // disappearing and reappearing when the new first result arrives.  Of
     // course we can only autofill the placeholder if it starts with the new
     // search string.
     if (!allowAutofill ||
         this._autofillPlaceholder.length <= value.length ||
         !this._autofillPlaceholder.startsWith(value)) {
       this._autofillPlaceholder = "";
     }
-    if (this._autofillPlaceholder) {
-      this._autofillValueOnInput(this._autofillPlaceholder);
+
+    // Don't ever autofill on input if the caret/selection isn't at the end, or
+    // if the placeholder doesn't start with what the user typed.
+    if (this._autofillPlaceholder &&
+        this.selectionEnd == this.value.length &&
+        this._autofillPlaceholder.toLocaleLowerCase()
+          .startsWith(value.toLocaleLowerCase())) {
+      this._autofillValue(this._autofillPlaceholder, value.length,
+                          this._autofillPlaceholder.length);
     }
 
     return allowAutofill;
   }
 
   _updateTextOverflow() {
     if (!this._overflowing) {
       this.removeAttribute("textoverflow");
@@ -926,50 +934,27 @@ class UrlbarInput {
     }
     value = "http://www." + value;
 
     this.value = value;
     return value;
   }
 
   /**
-   * Autofills a value into the input in response to the user's typing.  The
-   * autofill value must start with the value that's already in the input.  If
-   * it doesn't, this method doesn't do anything.  If it does, this method will
-   * autofill and set the selection automatically.
+   * Autofills a value into the input.  The value will be autofilled regardless
+   * of the input's current value.
    *
    * @param {string} value
    *   The value to autofill.
-   */
-  _autofillValueOnInput(value) {
-    // Don't ever autofill on input if the caret/selection isn't at the end, or
-    // if the value doesn't start with what the user typed.
-    if (this.selectionEnd != this.value.length ||
-        !value.startsWith(this._lastSearchString)) {
-      return;
-    }
-    this._autofillValue({
-      value,
-      selectionStart: this._lastSearchString.length,
-      selectionEnd: value.length,
-    });
-  }
-
-  /**
-   * Autofills a value into the input.  The value will be autofilled regardless
-   * of the input's current value.
-   *
-   * @param {string} options.value
-   *   The value to autofill.
-   * @param {integer} options.selectionStart
+   * @param {integer} selectionStart
    *   The new selectionStart.
-   * @param {integer} options.selectionEnd
+   * @param {integer} selectionEnd
    *   The new selectionEnd.
    */
-  _autofillValue({ value, selectionStart, selectionEnd } = {}) {
+  _autofillValue(value, selectionStart, selectionEnd) {
     // The autofilled value may be a URL that includes a scheme at the
     // beginning.  Do not allow it to be trimmed.
     this._setValue(value, false);
     this.selectionStart = selectionStart;
     this.selectionEnd = selectionEnd;
     this._autofillPlaceholder = value;
   }
 
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -23,16 +23,17 @@ skip-if = true # Bug 1524539 - need to f
 [browser_autocomplete_readline_navigation.js]
 skip-if = os != "mac" # Mac only feature
 [browser_autocomplete_tag_star_visibility.js]
 [browser_autoFill_backspaced.js]
 [browser_autoFill_canonize.js]
 [browser_autoFill_caretNotAtEnd.js]
 [browser_autoFill_preserve.js]
 [browser_autoFill_trimURLs.js]
+[browser_autoFill_typed.js]
 [browser_canonizeURL.js]
 [browser_caret_navigation.js]
 [browser_dragdropURL.js]
 [browser_keepStateAcrossTabSwitches.js]
 [browser_keyword_override.js]
 [browser_keyword_select_and_type.js]
 [browser_keyword.js]
 support-files =
new file mode 100644
--- /dev/null
+++ b/browser/components/urlbar/tests/browser/browser_autoFill_typed.js
@@ -0,0 +1,182 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// This test makes sure that autofill works as expected when typing, character
+// by character.
+
+"use strict";
+
+add_task(async function init() {
+  await cleanUp();
+});
+
+add_task(async function origin() {
+  await PlacesTestUtils.addVisits([
+    "http://example.com/",
+  ]);
+  // all lowercase
+  await typeAndCheck([
+    ["e", "example.com/"],
+    ["x", "example.com/"],
+    ["a", "example.com/"],
+    ["m", "example.com/"],
+    ["p", "example.com/"],
+    ["l", "example.com/"],
+    ["e", "example.com/"],
+    [".", "example.com/"],
+    ["c", "example.com/"],
+    ["o", "example.com/"],
+    ["m", "example.com/"],
+    ["/", "example.com/"],
+  ]);
+  gURLBar.value = "";
+  // mixed case
+  await typeAndCheck([
+    ["E", "Example.com/"],
+    ["x", "Example.com/"],
+    ["A", "ExAmple.com/"],
+    ["m", "ExAmple.com/"],
+    ["P", "ExAmPle.com/"],
+    ["L", "ExAmPLe.com/"],
+    ["e", "ExAmPLe.com/"],
+    [".", "ExAmPLe.com/"],
+    ["C", "ExAmPLe.Com/"],
+    ["o", "ExAmPLe.Com/"],
+    ["M", "ExAmPLe.CoM/"],
+    ["/", "ExAmPLe.CoM/"],
+  ]);
+  await cleanUp();
+});
+
+add_task(async function url() {
+  await PlacesTestUtils.addVisits([
+    "http://example.com/foo/bar",
+  ]);
+  // all lowercase
+  await typeAndCheck([
+    ["e", "example.com/"],
+    ["x", "example.com/"],
+    ["a", "example.com/"],
+    ["m", "example.com/"],
+    ["p", "example.com/"],
+    ["l", "example.com/"],
+    ["e", "example.com/"],
+    [".", "example.com/"],
+    ["c", "example.com/"],
+    ["o", "example.com/"],
+    ["m", "example.com/"],
+    ["/", "example.com/"],
+    ["f", "example.com/foo/"],
+    ["o", "example.com/foo/"],
+    ["o", "example.com/foo/"],
+    ["/", "example.com/foo/"],
+    ["b", "example.com/foo/bar"],
+    ["a", "example.com/foo/bar"],
+    ["r", "example.com/foo/bar"],
+  ]);
+  gURLBar.value = "";
+  // mixed case
+  await typeAndCheck([
+    ["E", "Example.com/"],
+    ["x", "Example.com/"],
+    ["A", "ExAmple.com/"],
+    ["m", "ExAmple.com/"],
+    ["P", "ExAmPle.com/"],
+    ["L", "ExAmPLe.com/"],
+    ["e", "ExAmPLe.com/"],
+    [".", "ExAmPLe.com/"],
+    ["C", "ExAmPLe.Com/"],
+    ["o", "ExAmPLe.Com/"],
+    ["M", "ExAmPLe.CoM/"],
+    ["/", "ExAmPLe.CoM/"],
+    ["f", "ExAmPLe.CoM/foo/"],
+    ["o", "ExAmPLe.CoM/foo/"],
+    ["o", "ExAmPLe.CoM/foo/"],
+    ["/", "ExAmPLe.CoM/foo/"],
+    ["b", "ExAmPLe.CoM/foo/bar"],
+    ["a", "ExAmPLe.CoM/foo/bar"],
+    ["r", "ExAmPLe.CoM/foo/bar"],
+  ]);
+  await cleanUp();
+});
+
+add_task(async function tokenAlias() {
+  // We have built-in engine aliases that may conflict with the one we choose
+  // here in terms of autofill, so be careful and choose a weird alias.
+  await Services.search.addEngineWithDetails("Test", {
+    alias: "@__example",
+    template: "http://example.com/?search={searchTerms}",
+  });
+  registerCleanupFunction(async function() {
+    let engine = Services.search.getEngineByName("Test");
+    await Services.search.removeEngine(engine);
+  });
+  // all lowercase
+  await typeAndCheck([
+    ["@", "@"],
+    ["_", "@__example "],
+    ["_", "@__example "],
+    ["e", "@__example "],
+    ["x", "@__example "],
+    ["a", "@__example "],
+    ["m", "@__example "],
+    ["p", "@__example "],
+    ["l", "@__example "],
+    ["e", "@__example "],
+  ]);
+  gURLBar.value = "";
+  // mixed case
+  await typeAndCheck([
+    ["@", "@"],
+    ["_", "@__example "],
+    ["_", "@__example "],
+    ["E", "@__Example "],
+    ["x", "@__Example "],
+    ["A", "@__ExAmple "],
+    ["m", "@__ExAmple "],
+    ["P", "@__ExAmPle "],
+    ["L", "@__ExAmPLe "],
+    ["e", "@__ExAmPLe "],
+  ]);
+  await cleanUp();
+});
+
+
+async function typeAndCheck(values) {
+  gURLBar.focus();
+  for (let i = 0; i < values.length; i++) {
+    let [char, expectedInputValue] = values[i];
+    info(`Typing: i=${i} char=${char} ` +
+         `substring="${expectedInputValue.substring(0, i + 1)}"`);
+    EventUtils.synthesizeKey(char);
+    if (i == 0 && char == "@") {
+      // A single "@" doesn't trigger autofill, so skip the checks below.  (It
+      // shows all the @ aliases.)
+      continue;
+    }
+    await UrlbarTestUtils.promiseSearchComplete(window);
+    Assert.equal(gURLBar.value, expectedInputValue);
+    Assert.equal(gURLBar.selectionStart, i + 1);
+    Assert.equal(gURLBar.selectionEnd, expectedInputValue.length);
+    let restIsSpaces = !expectedInputValue.substring(i + 1).trim();
+    if (restIsSpaces) {
+      // Autofilled @ aliases have a trailing space.  We should check that the
+      // space is autofilled when each preceding character is typed, but once
+      // the final non-space char is typed, autofill actually stops and the
+      // trailing space is not autofilled.  (Which is maybe not the way it
+      // should work...)  Skip the check below.
+      continue;
+    }
+    let details = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
+    Assert.ok(details.autofill);
+  }
+}
+
+async function cleanUp() {
+  gURLBar.value = "";
+  await UrlbarTestUtils.promisePopupClose(window, () => {
+    EventUtils.synthesizeKey("KEY_Escape");
+  });
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesUtils.history.clear();
+}