Bug 1584545 - Clear the autofill placeholder if the first result is not an autofill one. r=Standard8

Fixes a problem where the autofill placeholder is applied regardless of the
first result. This is particularly critical for keywords and aliases.

It also changes the title of keyword results without a search string, because
currently it looks like they browse to the root of the host, but instead they
visit the bookmark url. It's better to show the bookmark url so the user knows
what will be visited.

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

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

browser/components/urlbar/UrlbarInput.jsm

@@ -811,6 +811,25 @@ class UrlbarInput {
     this.setValueFromResult(result);
   }
 
+  /**
+   * Invoked by the view when the first result is received.
+   * To prevent selection flickering, we apply autofill on input through a
+   * placeholder, without waiting for results.
+   * But, if the first result is not an autofill one, the autofill prediction
+   * was wrong and we should restore the original user typed string.
+   * @param {UrlbarResult} firstResult The first result received.
+   */
+  maybeClearAutofillPlaceholder(firstResult) {
+    if (
+      this._autofillPlaceholder &&
+      !firstResult.autofill &&
+      // Avoid clobbering added spaces (for token aliases, for example).
+      !this.value.endsWith(" ")
+    ) {
+      this._setValue(this.window.gBrowser.userTypedValue, false);
+    }
+  }
+
   /**
    * Starts a query based on the current input value.
    *

browser/components/urlbar/UrlbarView.jsm

@@ -442,6 +442,10 @@ class UrlbarView {
           (trimmedValue[0] != UrlbarTokenizer.RESTRICT.SEARCH ||
             trimmedValue.length != 1)
       );
+
+      // The input field applies autofill on input, without waiting for results.
+      // Once we get results, we can ask it to correct wrong predictions.
+      this.input.maybeClearAutofillPlaceholder(queryContext.results[0]);
     }
 
     this._openPanel();

browser/components/urlbar/tests/browser/browser_autoFill_placeholder.js

@@ -155,7 +155,49 @@ add_task(async function noMatch2() {
   await cleanUp();
 });
 
-async function searchAndCheck(searchString, expectedAutofillValue) {
+add_task(async function clear_placeholder_for_keyword_or_alias() {
+  info("Clear the autofill placeholder if a keyword is typed");
+  await PlacesTestUtils.addVisits("http://example.com/");
+  await PlacesUtils.keywords.insert({
+    keyword: "ex",
+    url: "http://somekeyword.com/",
+  });
+  let engine = await Services.search.addEngineWithDetails("AutofillTest", {
+    alias: "exam",
+    template: "http://example.com/?search={searchTerms}",
+  });
+  registerCleanupFunction(async function() {
+    await PlacesUtils.keywords.remove("ex");
+    await Services.search.removeEngine(engine);
+  });
+
+  // Do an initial search that triggers autofill so that the placeholder has an
+  // initial value.
+  await promiseAutocompleteResultPopup("e", window, true);
+  let details = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
+  Assert.ok(details.autofill);
+  Assert.equal(gURLBar.value, "example.com/");
+  Assert.equal(gURLBar.selectionStart, "e".length);
+  Assert.equal(gURLBar.selectionEnd, "example.com/".length);
+
+  // The values are initially autofilled on input, then the placeholder is
+  // removed when the first non-autofill result arrives.
+
+  // Matches the keyword.
+  await searchAndCheck("ex", "example.com/", "ex");
+  await searchAndCheck("EXA", "EXAmple.com/", "EXAmple.com/");
+  // Matches the alias.
+  await searchAndCheck("eXaM", "eXaMple.com/", "eXaM");
+  await searchAndCheck("examp", "example.com/", "example.com/");
+
+  await cleanUp();
+});
+
+async function searchAndCheck(
+  searchString,
+  expectedAutofillValue,
+  onCompleteValue = ""
+) {
   gURLBar.value = searchString;
 
   // Placeholder autofill is done on input, so fire an input event.  As the
@@ -172,6 +214,13 @@ async function searchAndCheck(searchString, expectedAutofillValue) {
   Assert.equal(gURLBar.selectionEnd, expectedAutofillValue.length);
 
   await UrlbarTestUtils.promiseSearchComplete(window);
+
+  if (onCompleteValue) {
+    // Check the final value after the results arrived.
+    Assert.equal(gURLBar.value, onCompleteValue);
+    Assert.equal(gURLBar.selectionStart, searchString.length);
+    Assert.equal(gURLBar.selectionEnd, onCompleteValue.length);
+  }
 }
 
 async function cleanUp() {

browser/components/urlbar/tests/browser/browser_keyword.js

@@ -82,8 +82,8 @@ add_task(async function test_display_keyword_without_query() {
   );
   Assert.equal(
     result.displayed.title,
-    "example.com",
-    "Node should contain the name of the bookmark"
+    "https://example.com/browser/browser/components/urlbar/tests/browser/print_postdata.sjs?q=",
+    "Node should contain the url of the bookmark"
   );
   Assert.equal(
     result.displayed.action,

browser/components/urlbar/tests/browser/browser_view_resultTypes_display.js

@@ -171,9 +171,10 @@ add_task(async function test_keyword_result() {
 
     let details = await UrlbarTestUtils.getDetailsOfResultAt(window, 0);
 
+    // Because only the keyword is typed, we show the bookmark url.
     assertElementsDisplayed(details, {
       separator: true,
-      title: "example.com",
+      title: TEST_URL + "?q=",
       type: UrlbarUtils.RESULT_TYPE.KEYWORD,
     });
 

toolkit/components/places/UnifiedComplete.jsm

@@ -1597,8 +1597,8 @@ Search.prototype = {
       keyword
     ).trim();
 
-    let url = null,
-      postData = null;
+    let url = null;
+    let postData = null;
     try {
       [url, postData] = await BrowserUtils.parseUrlAndPostData(
         entry.url.href,
@@ -1621,18 +1621,21 @@ Search.prototype = {
         postData,
       });
     }
-    // The title will end up being "host: queryString"
-    let comment = entry.url.host;
 
-    this._addMatch({
+    let match = {
       value,
-      comment,
       // Don't use the url with replaced strings, since the icon doesn't change
       // but the string does, it may cause pointless icon flicker on typing.
       icon: iconHelper(entry.url),
       style,
       frecency: Infinity,
-    });
+    };
+    // If there is a query string, the title will be "host: queryString".
+    if (this._searchTokens.length > 1) {
+      match.comment = entry.url.host;
+    }
+
+    this._addMatch(match);
     if (!this._keywordSubstitute) {
       this._keywordSubstitute = entry.url.host;
     }

toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js

@@ -117,7 +117,6 @@ add_task(async function test_keyword_search() {
     matches: [
       {
         uri: NetUtil.newURI("http://abc/?search="),
-        title: "abc",
         style: ["keyword", "heuristic"],
       },
       {
@@ -138,7 +137,6 @@ add_task(async function test_keyword_search() {
     matches: [
       {
         uri: NetUtil.newURI("http://abc/?search="),
-        title: "abc",
         style: ["keyword", "heuristic"],
       },
       {

toolkit/components/places/tests/unifiedcomplete/test_keyword_search_actions.js

@@ -190,7 +190,6 @@ add_task(async function test_keyword_search() {
           keyword: "key2",
           input: "key2",
         }),
-        title: "def",
         style: ["action", "keyword", "heuristic"],
       },
       { uri: uri5, title: "Keyword", style: ["bookmark"] },
@@ -208,7 +207,6 @@ add_task(async function test_keyword_search() {
           keyword: "key2",
           input: "key2 ",
         }),
-        title: "def",
         style: ["action", "keyword", "heuristic"],
       },
       { uri: uri5, title: "Keyword", style: ["bookmark"] },