1176107 - Search suggestions in awesomebar: highlighting a suggestion doesn't place it into the input, and selecting a suggestion doesn't actually search with it

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Drew Willcoxon :adw]

Attachment: patch

When you highlight a suggestion, it's not put into the input.  Instead whatever you typed to trigger the suggestion remains in the input.

And when you click a suggestion, what you end up searching for is the original input, not the clicked suggestion.

Comment 1

[Drew Willcoxon :adw]

Attachment: patch with updated test

Needed to update the test.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8624880 [details] [diff] [review]
patch with updated test

Review of attachment 8624880 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/UnifiedComplete.js
@@ +1025,5 @@
>  
>    _addSearchEngineMatch(match, query, suggestion) {
>      let actionURLParams = {
>        engineName: match.engineName,
> +      input: suggestion || this._originalSearchString,

after moving over this result, do we properly restore the original user search text in the textfield? I am currently still building and I couldn't verify that, but off-hand looks like we might be doing that.
If that's true, looks like it might not be trivial to "autofill" the remaining part of the suggestion on selection through the search provider, we might have to actually add a visual hack for that to the binding...
Btw, I'll check this once I get a build.

@@ +1026,5 @@
>    _addSearchEngineMatch(match, query, suggestion) {
>      let actionURLParams = {
>        engineName: match.engineName,
> +      input: suggestion || this._originalSearchString,
> +      searchQuery: suggestion || query,

setting searchQuery like this will break hilighting the user typed text, we use searchQuery exactly for that... 

the right fix might be using searchSuggestion where currently we use searchQuery, when searchSuggestion is defined clearly. very likely in urlbarBindings.xml.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

ok, nevermind the input part, it seems to be working fine and the user can still go back to the first entry that has the original text. The only problem is the hilight due to changing searchQuery.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch v2

Well, here's a patch that does what you suggested, but it's not exactly right.  Whatever I type in the input appears as a substring in all the displayed suggestions, even if a suggestion does not actually contain what I typed.  For example, when I type "foox", I get these:

fooxnews
fooxsports
fooxcatcher

But the actual suggestions are:

fox news
fox sports
foxcatcher

And when I highlight each suggestion, the correct suggestion is put into the input.

We can't assume that what you typed actually appears in each suggestion.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8626393 [details] [diff] [review]
patch v2

Review of attachment 8626393 [details] [diff] [review]:
-----------------------------------------------------------------

ok, so, we must highlight the term only if it appears in the suggested string and don't hilight in the other case.
Ideally we could use a levenshtein distance algorithm to figure which of the terms matches, but I feel like that would go too far, let's keep it as a possible future improvement.

In the end looks like the code in autocomplete.xml that does the hilight in the suggestions case is bogus, specifically this code:
              let suggestedPart = "";
              if (searchSuggestion) {
                suggestedPart = searchSuggestion.substr(searchQuery.length);
              }

              title = this._generateEmphasisPairs(`%1$S${suggestedPart} %2$S`, [
                                                    [searchQuery, "match"],
                                                    [engineStr, "selected"],
                                                  ]);
from searchQuery we need to separate a matchPart and a suggestedPart, and pass matchPart as "match". in case searchSuggestion doesn't contain searchQuery, then matchPart will be an empty string and suggestedPart will be searchSuggestion. I hope we don't render an empty string as a match...

Can we add a test where for "foo" we suggest "fox" (or something similar)? since this patch shows the code can corrupt the string, it would be nice to have a test for that.

::: browser/base/content/urlbarBindings.xml
@@ +791,5 @@
> +            "input",
> +            "searchQuery",
> +            "searchSuggestion",
> +          ];
> +          for (let key of keys) {

nit: I'd just inline the array in the for

Comment 7

[Drew Willcoxon :adw]

Attachment: patch v3

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8626864 [details] [diff] [review]
patch v3

[Hit Return and submitted the patch before I meant to...]

Ah, thanks.  This patch seems to work well.

In order to get the results for "$ foo" to be properly styled with emphasis, I had to modify _maybeAddSearchSuggestionMatch to pass _searchTokens.join(" ") instead of _originalSearchString to _addSearchEngineMatch.  Because otherwise "$" is in the searchQuery string, so none of the results contain the searchQuery.  Actually, should _addSearchEngineMatch always pass _searchTokens.join(" ") as `input` instead of _originalSearchString?

This also cleans up the test a little by automatically resetting the search suggestion function that the test server uses after each test, on cleanup.

And this adds a test for where the original search query string is not in the suggestions.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Drew Willcoxon :adw from comment #8)
> Actually, should _addSearchEngineMatch always pass
> _searchTokens.join(" ") as `input` instead of _originalSearchString?

Probably yes, see bug 1177895 that I just filed (even if that likely affects the fallback heuristic and won't be fixed by doing this change)

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8626864 [details] [diff] [review]
patch v3

Review of attachment 8626864 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't have the time to test this latest version as the previous one since I'm traveling, off-hand it looks good and I assume you did the testing already.

::: toolkit/components/places/UnifiedComplete.js
@@ +1191,5 @@
>        let [match, suggestion] = this._searchSuggestionController.consume();
>        if (suggestion) {
> +        // Don't include the restrict token, if present.
> +        let searchString = this._searchTokens.join(" ");
> +        this._addSearchEngineMatch(match, searchString, suggestion);

So I _addSearchEngineMatch can generally use searchTokens, but in _matchSearchEngineAlias where we actually have to slice from searchTokens...

So probably for now we'll just have to pass like you do here and not directly change _addSearchEngineMatch

::: toolkit/content/widgets/autocomplete.xml
@@ +1680,5 @@
> +              pairs.push([engineStr, "selected"]);
> +              let interpStr = "";
> +              for (let i = 1; i <= pairs.length; i++) {
> +                interpStr += `%${i}$S`;
> +              }

nit: you could maybe
let interpStr = pairs.map(......).join("");

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Marco Bonardo [::mak] from comment #10)
> So I _addSearchEngineMatch can generally use searchTokens, but in
> _matchSearchEngineAlias where we actually have to slice from searchTokens...


not sure what happened here, I meant to type:

> So, in general _addSearchEngineMatch could just use searchTokens, but not in
> _matchSearchEngineAlias, where we actually have to slice from searchTokens...

Comment 12

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/b015ec383bc8

(In reply to Marco Bonardo [::mak] from comment #10)
> nit: you could maybe
> let interpStr = pairs.map(......).join("");

Hmm, I like map(), but in this case we're not mapping items in the array but their indexes, which isn't a great use of map() I think.  But I changed it anyway. :-)

Comment 13

[Drew Willcoxon :adw]

Ideally we wouldn't have to build that string at all and could just pass the array of pairs.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b015ec383bc8