Bug 1504552 - Correctly handle search engine aliases and other keywords when restriction characters are present. r=mak

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

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

browser/base/content/test/urlbar/browser_urlbarHighlightSearchAlias.js

@@ -62,6 +62,19 @@ add_task(async function charsBeforeAlias() {
 });
 
 
+// In a search string that starts with a restriction character followed by an
+// alias, the alias should be neither recognized nor highlighted.
+add_task(async function restrictionCharBeforeAlias() {
+  gURLBar.search(UrlbarTokenizer.RESTRICT.BOOKMARK + " " + ALIAS);
+  await promiseSearchComplete();
+  await waitForAutocompleteResultAt(0);
+  assertAlias(false);
+
+  EventUtils.synthesizeKey("KEY_Escape");
+  await promisePopupHidden(gURLBar.popup);
+});
+
+
 // Aliases are case insensitive, and the alias in the result uses the case that
 // the user typed in the input.  Make sure that searching with an alias using a
 // weird case still causes the alias to be highlighted.
@@ -175,6 +188,26 @@ add_task(async function nonHeuristicAliases() {
 });
 
 
+// Aliases that don't start with @ shouldn't be highlighted.
+add_task(async function nonAtMarkAlias() {
+  let alias = "notatmarkalias";
+  let engine = Services.search.getEngineByName("Test");
+  engine.alias = "notatmarkalias";
+  Assert.equal(engine.alias, alias);
+
+  gURLBar.search(alias);
+  await promiseSearchComplete();
+  await waitForAutocompleteResultAt(0);
+  assertFirstResultIsAlias(true, alias);
+  assertHighlighted(false);
+
+  EventUtils.synthesizeKey("KEY_Escape");
+  await promisePopupHidden(gURLBar.popup);
+
+  engine.alias = ALIAS;
+});
+
+
 async function doSimpleTest(revertBetweenSteps) {
   // When autofill is enabled, searching for "@tes" will autofill to "@test",
   // which gets in the way of this test task, so temporarily disable it.

browser/components/urlbar/UrlbarValueFormatter.jsm

@@ -261,8 +261,7 @@ class UrlbarValueFormatter {
   }
 
   /**
-   * If the input value starts with a search alias, this formatter method
-   * highlights it.
+   * If the input value starts with an @engine search alias, this highlights it.
    *
    * @returns {boolean}
    *   True if formatting was applied and false if not.
@@ -282,12 +281,21 @@ class UrlbarValueFormatter {
       return false;
     }
 
-    // To determine whether the input contains a search engine alias, check the
-    // value of the selected result -- whether it's a search engine result with
-    // an alias.  Actually, check the selected listbox item, not the result in
-    // the controller, because we want to continue highlighting the alias when
-    // the popup is closed and the search has stopped.  The selected index when
-    // the popup is closed is zero, however, which is why we check the previous
+    let editor = this.urlbarInput.editor;
+    let textNode = editor.rootElement.firstChild;
+    let value = textNode.textContent;
+    let trimmedValue = value.trim();
+
+    if (!trimmedValue.startsWith("@")) {
+      return false;
+    }
+
+    // To determine whether the input contains a valid alias, check the value of
+    // the selected result -- whether it's a search engine result with an alias.
+    // Actually, check the selected listbox item, not the result in the
+    // controller, because we want to continue highlighting the alias when the
+    // popup is closed and the search has stopped.  The selected index when the
+    // popup is closed is zero, however, which is why we also check the previous
     // selected index.
     let itemIndex =
       popup.selectedIndex < 0 ? popup._previousSelectedIndex :
@@ -316,10 +324,6 @@ class UrlbarValueFormatter {
       return false;
     }
 
-    let editor = this.urlbarInput.editor;
-    let textNode = editor.rootElement.firstChild;
-    let value = textNode.textContent;
-
     // Make sure the item's input matches the current urlbar input because the
     // urlbar input can change without the popup results changing.  Most notably
     // that happens when the user performs a search using an alias: The popup
@@ -331,7 +335,7 @@ class UrlbarValueFormatter {
     // its input is "@engine ".  So in order to make sure the item's input
     // matches the current urlbar input, we need to check that the urlbar input
     // starts with the item's input.
-    if (!value.trim().startsWith(decodeURIComponent(action.params.input).trim())) {
+    if (!trimmedValue.startsWith(action.params.input.trim())) {
       return false;
     }
 

toolkit/components/places/UnifiedComplete.js

@@ -527,6 +527,38 @@ function looksLikeOrigin(str) {
   return REGEXP_ORIGIN.test(str);
 }
 
+/**
+ * Returns the portion of a string starting at the index where another string
+ * begins.
+ *
+ * @param   {string} sourceStr
+ *          The string to search within.
+ * @param   {string} targetStr
+ *          The string to search for.
+ * @returns {string} The substring within sourceStr starting at targetStr, or
+ *          the empty string if targetStr does not occur in sourceStr.
+ */
+function substringAt(sourceStr, targetStr) {
+  let index = sourceStr.indexOf(targetStr);
+  return index < 0 ? "" : sourceStr.substr(index);
+}
+
+/**
+ * Returns the portion of a string starting at the index where another string
+ * ends.
+ *
+ * @param   {string} sourceStr
+ *          The string to search within.
+ * @param   {string} targetStr
+ *          The string to search for.
+ * @returns {string} The substring within sourceStr where targetStr ends, or the
+ *          empty string if targetStr does not occur in sourceStr.
+ */
+function substringAfter(sourceStr, targetStr) {
+  let index = sourceStr.indexOf(targetStr);
+  return index < 0 ? "" : sourceStr.substr(index + targetStr.length);
+}
+
 /**
  * Manages a single instance of an autocomplete search.
  *
@@ -583,6 +615,20 @@ function Search(searchString, searchParam, autocompleteListener,
 
   this._searchTokens =
     this.filterTokens(getUnfilteredSearchTokens(this._searchString));
+
+  // The heuristic token is the first filtered search token, but only when it's
+  // actually the first thing in the search string.  If a prefix or restriction
+  // character occurs first, then the heurstic token is null.  We use the
+  // heuristic token to help determine the heuristic result.  It may be a Places
+  // keyword, a search engine alias, an extension keyword, or simply a URL or
+  // part of the search string the user has typed.  We won't know until we
+  // create the heuristic result.
+  this._heuristicToken =
+    this._searchTokens[0] &&
+      this._trimmedOriginalSearchString.startsWith(this._searchTokens[0]) ?
+    this._searchTokens[0] :
+    null;
+
   this._keywordSubstitute = null;
 
   this._prohibitSearchSuggestions = prohibitSearchSuggestions;
@@ -695,11 +741,12 @@ Search.prototype = {
 
   /**
    * Given an array of tokens, this function determines which query should be
-   * ran.  It also removes any special search tokens.
+   * ran.  It also removes any special search tokens.  The given array of tokens
+   * is modified in place and returned.
    *
    * @param tokens
-   *        An array of search tokens.
-   * @return the filtered list of tokens to search with.
+   *        An array of search tokens.  This array is modified in place.
+   * @return The given array of tokens, modified to remove special search tokens.
    */
   filterTokens(tokens) {
     let foundToken = false;
@@ -861,9 +908,9 @@ Search.prototype = {
     // Only add extension suggestions if the first token is a registered keyword
     // and the search string has characters after the first token.
     let extensionsCompletePromise = Promise.resolve();
-    if (this._searchTokens.length > 0 &&
-        ExtensionSearchHandler.isKeywordRegistered(this._searchTokens[0]) &&
-        this._originalSearchString.length > this._searchTokens[0].length &&
+    if (this._heuristicToken &&
+        ExtensionSearchHandler.isKeywordRegistered(this._heuristicToken) &&
+        substringAfter(this._originalSearchString, this._heuristicToken) &&
         !this._searchEngineAliasMatch) {
       // Do not await on this, since extensions cannot notify when they are done
       // adding results, it may take too long.
@@ -878,13 +925,9 @@ Search.prototype = {
     if (this._enableActions &&
         this.hasBehavior("search") &&
         !this._inPrivateWindow) {
-      // Get the query string stripped of any engine alias or restriction token.
-      // In the former case, _searchTokens[0] will be the alias, so use
-      // this._searchEngineHeuristicMatch.query.  In the latter case, the token
-      // has been removed from _searchTokens, so use _searchTokens.join().
       let query =
         this._searchEngineAliasMatch ? this._searchEngineAliasMatch.query :
-        this._searchTokens.join(" ");
+        substringAt(this._originalSearchString, this._searchTokens[0]);
       if (query) {
         // Limit the string sent for search suggestions to a maximum length.
         query = query.substr(0, UrlbarPrefs.get("maxCharsForSearchSuggestions"));
@@ -1115,16 +1158,13 @@ Search.prototype = {
   },
 
   async _matchSearchEngineTokenAliasForAutofill() {
-    // We need a single "@engine" search token.
-    if (this._searchTokens.length != 1) {
-      return false;
-    }
-    let token = this._searchTokens[0];
-    if (token[0] != "@" || token.length == 1) {
+    // We need an "@engine" heuristic token.
+    let token = this._heuristicToken;
+    if (!token || token.length == 1 || !token.startsWith("@")) {
       return false;
     }
 
-    // See if any engine token alias starts with the search token.
+    // See if any engine has a token alias that starts with the heuristic token.
     let engines = await PlacesSearchAutocompleteProvider.tokenAliasEngines();
     for (let { engine, tokenAliases } of engines) {
       for (let alias of tokenAliases) {
@@ -1375,9 +1415,11 @@ Search.prototype = {
   },
 
   _matchExtensionHeuristicResult() {
-    if (ExtensionSearchHandler.isKeywordRegistered(this._searchTokens[0]) &&
-        this._originalSearchString.length > this._searchTokens[0].length) {
-      let description = ExtensionSearchHandler.getDescription(this._searchTokens[0]);
+    if (this._heuristicToken &&
+        ExtensionSearchHandler.isKeywordRegistered(this._heuristicToken) &&
+        substringAfter(this._originalSearchString, this._heuristicToken)) {
+      let description =
+        ExtensionSearchHandler.getDescription(this._heuristicToken);
       this._addExtensionMatch(this._originalSearchString, description);
       return true;
     }
@@ -1385,13 +1427,17 @@ Search.prototype = {
   },
 
   async _matchPlacesKeyword() {
-    // The first word could be a keyword, so that's what we'll search.
-    let keyword = this._strippedPrefix + this._searchTokens[0];
-    let entry = await PlacesUtils.keywords.fetch(keyword);
-    if (!entry)
+    if (!this._heuristicToken) {
       return false;
+    }
+    let keyword = this._heuristicToken;
+    let entry = await PlacesUtils.keywords.fetch(keyword);
+    if (!entry) {
+      return false;
+    }
 
-    let searchString = this._trimmedOriginalSearchString.substr(keyword.length + 1);
+    let searchString =
+      substringAfter(this._originalSearchString, keyword).trim();
 
     let url = null, postData = null;
     try {
@@ -1495,11 +1541,11 @@ Search.prototype = {
   },
 
   async _matchSearchEngineAlias() {
-    if (this._searchTokens.length < 1) {
+    if (!this._heuristicToken) {
       return false;
     }
 
-    let alias = this._searchTokens[0];
+    let alias = this._heuristicToken;
     let engine = await PlacesSearchAutocompleteProvider.engineForAlias(alias);
     if (!engine) {
       return false;
@@ -1508,7 +1554,7 @@ Search.prototype = {
     this._searchEngineAliasMatch = {
       engine,
       alias,
-      query: this._trimmedOriginalSearchString.substr(alias.length + 1),
+      query: substringAfter(this._originalSearchString, alias).trim(),
     };
     this._addSearchEngineMatch(this._searchEngineAliasMatch);
     if (!this._keywordSubstitute) {
@@ -1539,7 +1585,7 @@ Search.prototype = {
     this._addMatch({
       value: PlacesUtils.mozActionURI("extension", {
         content,
-        keyword: this._searchTokens[0],
+        keyword: this._heuristicToken,
       }),
       comment,
       icon: "chrome://browser/content/extension.svg",
@@ -1609,10 +1655,10 @@ Search.prototype = {
   },
 
   _matchExtensionSuggestions() {
-    let promise = ExtensionSearchHandler.handleSearch(this._searchTokens[0], this._originalSearchString,
+    let promise = ExtensionSearchHandler.handleSearch(this._heuristicToken, this._originalSearchString,
       suggestions => {
         for (let suggestion of suggestions) {
-          let content = `${this._searchTokens[0]} ${suggestion.content}`;
+          let content = `${this._heuristicToken} ${suggestion.content}`;
           this._addExtensionMatch(content, suggestion.description);
         }
       }

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

@@ -12,7 +12,7 @@
  * same keyword appear in the list.
  */
 
-add_task(async function test_keyword_searc() {
+add_task(async function test_keyword_search() {
   let uri1 = NetUtil.newURI("http://abc/?search=%s");
   let uri2 = NetUtil.newURI("http://abc/?search=ThisPageIsInHistory");
   let uri3 = NetUtil.newURI("http://somedomain.example/key");

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

@@ -99,6 +99,25 @@ add_task(async function getPost() {
         }),
       ],
     });
+
+    // When a restriction token is used before the alias, the alias should *not*
+    // be recognized.  It should be treated as part of the search string.  Try
+    // all the restriction tokens to test that.  We should get a single "search
+    // with" heuristic result without an alias.
+    for (let restrictToken in UrlbarTokenizer.RESTRICT) {
+      let search = `${restrictToken} ${alias} query string`;
+      await check_autocomplete({
+        search,
+        searchParam: "enable-actions",
+        matches: [
+          makeSearchMatch(search, {
+            engineName: "MozSearch",
+            searchQuery: search,
+            heuristic: true,
+          }),
+        ],
+      });
+    }
   }
 
   await cleanup();