Bug 1697678 - Choose quick suggest suggestions only from matching results. r=daleharvey, a=jcristau

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

browser/components/urlbar/UrlbarProviderQuickSuggest.jsm

@@ -115,7 +115,7 @@ class ProviderQuickSuggest extends UrlbarProvider {
   async startQuery(queryContext, addCallback) {
     let instance = this.queryInstance;
     let suggestion = await UrlbarQuickSuggest.query(
-      queryContext.trimmedSearchString
+      queryContext.searchString.trimStart()
     );
     if (!suggestion || instance != this.queryInstance) {
       return;

browser/components/urlbar/UrlbarQuickSuggest.jsm

@@ -239,57 +239,123 @@ class KeywordTree {
     tree.set(RESULT_KEY, id);
   }
 
-  /*
+  /**
    * Get the result for a given phrase.
+   *
+   * @param {string} query
+   *   The query string.
+   * @returns {object}
+   *   The match object: { result, fullKeyword }, where `result` is the matching
+   *   result ID and `fullKeyword` is the suggestion.  If there is no match,
+   *   then `result` will be null and `fullKeyword` will not be defined.
    */
   get(query) {
-    let tree = this.tree;
-    let phrase = query.trim();
-    // The result object for the given phrase.
-    let result = null;
-    // The keyword that matched completed up untill the next space.
-    let fullKeyword = "";
-    // Whether we matched any phrases within an iteration so
-    // we know when to terminate.
-    let matched = false;
-    /*eslint no-labels: ["error", { "allowLoop": true }]*/
-    loop: while (true) {
-      matched = false;
-      for (const [key, child] of tree.entries()) {
-        if (key == RESULT_KEY) {
-          continue;
+    query = query.trimStart();
+    let match = this._getMatch(query, this.tree, "");
+    if (!match) {
+      return { result: null };
+    }
+
+    let result = UrlbarQuickSuggest._results.get(match.resultID);
+    if (!result) {
+      return { result: null };
+    }
+
+    let longerPhrase;
+    let trimmedQuery = query.trim();
+    let queryWords = trimmedQuery.split(" ");
+
+    // We need to determine a suggestion for the result (called `fullKeyword` in
+    // the returned object).  The data doesn't include suggestions, so we'll
+    // need to make our own based on the keyword phrases in the result.  First,
+    // find a decent matching phrase.  We'll use two heuristics:
+    //
+    // (1) Find the first phrase that has more words than the query.  Use its
+    //     first `queryWords.length` words as the suggestion.  e.g., if the
+    //     query is "moz" and `result.keywords` is ["moz", "mozi", "mozil",
+    //     "mozill", "mozilla", "mozilla firefox"], pick "mozilla firefox", pop
+    //     off the "firefox" and use "mozilla" as the suggestion.
+    // (2) If there isn't any phrase with more words, then pick the longest
+    //     phrase.  e.g., pick "mozilla" in the previous example (assuming the
+    //     "mozilla firefox" phrase isn't there).
+    for (let phrase of result.keywords) {
+      if (phrase.startsWith(query)) {
+        let trimmedPhrase = phrase.trim();
+        let phraseWords = trimmedPhrase.split(" ");
+        // As an exception to (1), if the query ends with a space, then look for
+        // phrases with one more word so that the suggestion includes a word
+        // following the space.
+        let extra = query.endsWith(" ") ? 1 : 0;
+        let len = queryWords.length + extra;
+        if (len < phraseWords.length) {
+          // We found a phrase with more words.
+          return {
+            result: match.resultID,
+            fullKeyword: phraseWords.slice(0, len).join(" "),
+          };
         }
-        // We need to check if key starts with phrase because we
-        // may have flattened the key and so .get("hel") will need
-        // to match index "hello", we will only flatten this way if
-        // the result matches.
-        if (!result && (phrase.startsWith(key) || key.startsWith(phrase))) {
-          matched = true;
-          phrase = phrase.slice(key.length);
-          if (!phrase.length) {
-            result = child.get(RESULT_KEY) || null;
-            if (!result) {
-              return { result };
-            }
-          }
+        if (
+          query.length < phrase.length &&
+          (!longerPhrase || longerPhrase.length < trimmedPhrase.length)
+        ) {
+          // We found a longer phrase with the same number of words.
+          longerPhrase = trimmedPhrase;
         }
-        if (result || matched) {
-          fullKeyword += key;
-          // If we find a space or we reach the end of the tree.
+      }
+    }
+    return {
+      result: match.resultID,
+      fullKeyword: longerPhrase || trimmedQuery,
+    };
+  }
+
+  /**
+   * Recursively looks up a match in the tree.
+   *
+   * @param {string} query
+   *   The query string.
+   * @param {Map} node
+   *   The node to start the search at.
+   * @param {string} phrase
+   *   The current phrase key path through the tree.
+   * @returns {object}
+   *   The match object: { resultID }, or null if no match was found.
+   */
+  _getMatch(query, node, phrase) {
+    for (const [key, child] of node.entries()) {
+      if (key == RESULT_KEY) {
+        continue;
+      }
+      let newPhrase = phrase + key;
+      let len = Math.min(newPhrase.length, query.length);
+      if (newPhrase.substring(0, len) == query.substring(0, len)) {
+        // The new phrase is a prefix of the query or vice versa.
+        let resultID = child.get(RESULT_KEY);
+        if (resultID !== undefined) {
+          // This child has a result.  Look it up to see if its keyword phrases
+          // match the query.  If it has a phrase that starts with the query,
+          // then it potentially matches, but we don't want to match when the
+          // query is shorter than every phrase, so also check that the query
+          // starts with some phrase.  For example, if the query is "m" and the
+          // phrases are ["moz", "mozilla"], we don't want to match until the
+          // query is at least "moz".
+          let result = UrlbarQuickSuggest._results.get(resultID);
           if (
-            (result && key.includes(" ")) ||
-            (child.size == 1 && child.get(RESULT_KEY))
+            result &&
+            result.keywords.some(p => p.startsWith(query)) &&
+            result.keywords.some(p => query.startsWith(p))
           ) {
-            return { result, fullKeyword: fullKeyword.trim() };
+            return { resultID };
           }
-          tree = child;
-          continue loop;
         }
-      }
-      if (!result) {
-        return { result };
+        // Recurse into descendants and return any match.
+        let match = this._getMatch(query, child, newPhrase);
+        if (match) {
+          return match;
+        }
       }
     }
+    return null;
   }
 
   /*

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

@@ -6,6 +6,7 @@
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   KeywordTree: "resource:///modules/UrlbarQuickSuggest.jsm",
+  UrlbarQuickSuggest: "resource:///modules/UrlbarQuickSuggest.jsm",
 });
 
 let data = [
@@ -17,9 +18,27 @@ let data = [
     term: "helzo bar",
     keywords: ["helzo b", "helzo ba"],
   },
+  {
+    term: "test1",
+    keywords: ["aaa", "aaab", "aaa111", "aaab111"],
+  },
+  {
+    term: "test2",
+    keywords: ["aaa", "aaab", "aaa222", "aaab222"],
+  },
+
+  // These two "test10" and "test11" objects must be in the following order to
+  // test bug 1697678.
+  { term: "test10", keywords: ["kids sn", "kids sneakers"] },
+  { term: "test11", keywords: ["ki", "kin", "kind", "kindl", "kindle"] },
 ];
 
 function basicChecks(tree) {
+  UrlbarQuickSuggest._results = new Map();
+  for (let { term, keywords } of data) {
+    UrlbarQuickSuggest._results.set(term, { keywords: keywords.concat(term) });
+  }
+
   Assert.equal(tree.get("nomatch").result, null);
   Assert.equal(tree.get("he").result, null);
   Assert.equal(tree.get("hel").result, "helzo foo");
@@ -27,10 +46,46 @@ function basicChecks(tree) {
   Assert.equal(tree.get("helzo").result, "helzo foo");
   Assert.equal(tree.get("helzo").fullKeyword, "helzo");
   Assert.equal(tree.get("helzo ").result, "helzo foo");
+  Assert.equal(tree.get("helzo ").fullKeyword, "helzo foo");
   Assert.equal(tree.get("helzo foo").result, "helzo foo");
   Assert.equal(tree.get("helzo b").result, "helzo bar");
   Assert.equal(tree.get("helzo b").fullKeyword, "helzo bar");
   Assert.equal(tree.get("helzo bar").result, "helzo bar");
+
+  Assert.equal(tree.get("f").result, null);
+  Assert.equal(tree.get("fo").result, null);
+  Assert.equal(tree.get("foo").result, null);
+
+  Assert.equal(tree.get("b").result, null);
+  Assert.equal(tree.get("ba").result, null);
+  Assert.equal(tree.get("bar").result, null);
+
+  Assert.equal(tree.get("aa").result, null);
+  Assert.equal(tree.get("aaa").result, "test2");
+  Assert.equal(tree.get("aaa").fullKeyword, "aaab222");
+  Assert.equal(tree.get("aaab").result, "test2");
+  Assert.equal(tree.get("aaab").fullKeyword, "aaab222");
+  Assert.equal(tree.get("aaab1").result, "test1");
+  Assert.equal(tree.get("aaab1").fullKeyword, "aaab111");
+  Assert.equal(tree.get("aaab2").result, "test2");
+  Assert.equal(tree.get("aaab2").fullKeyword, "aaab222");
+  Assert.equal(tree.get("aaa1").result, "test1");
+  Assert.equal(tree.get("aaa1").fullKeyword, "aaa111");
+  Assert.equal(tree.get("aaa2").result, "test2");
+  Assert.equal(tree.get("aaa2").fullKeyword, "aaa222");
+
+  Assert.equal(tree.get("ki").result, "test11");
+  Assert.equal(tree.get("ki").fullKeyword, "kindle");
+  Assert.equal(tree.get("kin").result, "test11");
+  Assert.equal(tree.get("kin").fullKeyword, "kindle");
+  Assert.equal(tree.get("kind").result, "test11");
+  Assert.equal(tree.get("kind").fullKeyword, "kindle");
+  Assert.equal(tree.get("kid").result, null);
+  Assert.equal(tree.get("kids").result, null);
+  Assert.equal(tree.get("kids ").result, null);
+  Assert.equal(tree.get("kids s").result, null);
+  Assert.equal(tree.get("kids sn").result, "test10");
+  Assert.equal(tree.get("kids sn").fullKeyword, "kids sneakers");
 }
 
 function createTree() {
@@ -60,12 +115,35 @@ add_task(async function test_flatten() {
 
   Assert.deepEqual(
     {
+      k: {
+        i: {
+          "^": "test11",
+          "ds s": { n: { "^": "test10", eaker: { s: { "^": "test10" } } } },
+          ndle: { "^": "test11" },
+        },
+      },
       he: {
         lzo: {
           "^": "helzo foo",
           " ": { foo: { "^": "helzo foo" }, bar: { "^": "helzo bar" } },
         },
       },
+      aa: {
+        a: {
+          "11": { "1": { "^": "test1" } },
+          "22": { "2": { "^": "test2" } },
+          "^": "test2",
+          b: {
+            "11": { "1": { "^": "test1" } },
+            "22": { "2": { "^": "test2" } },
+            "^": "test2",
+          },
+        },
+      },
+      test: {
+        "1": { "0": { "^": "test10" }, "1": { "^": "test11" }, "^": "test1" },
+        "2": { "^": "test2" },
+      },
     },
     tree.toJSONObject()
   );