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

RESOLVED FIXED in Firefox 41

Status

()

P1
normal
Rank:
3
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
Firefox 41
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [suggestions][fxsearch])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8624536 [details] [diff] [review]
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.
Attachment #8624536 - Flags: review?(mak77)
(Assignee)

Updated

3 years ago
Blocks: 1162140
No longer blocks: 959594
(Assignee)

Comment 1

3 years ago
Created attachment 8624880 [details] [diff] [review]
patch with updated test

Needed to update the test.
Attachment #8624536 - Attachment is obsolete: true
Attachment #8624536 - Flags: review?(mak77)
Attachment #8624880 - Flags: review?(mak77)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1176441
Flags: firefox-backlog+
Whiteboard: [suggestions][fxsearch]

Updated

3 years ago
Rank: 3
Priority: -- → P1
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.
Attachment #8624880 - Flags: review?(mak77) → review-
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.
(Assignee)

Comment 5

3 years ago
Created attachment 8626393 [details] [diff] [review]
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.
Attachment #8626393 - Flags: review?(mak77)
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
Attachment #8626393 - Flags: review?(mak77)
(Assignee)

Comment 7

3 years ago
Created attachment 8626864 [details] [diff] [review]
patch v3
Attachment #8626864 - Flags: review?(mak77)
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8624880 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8626393 - Attachment is obsolete: true
(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 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("");
Attachment #8626864 - Flags: review?(mak77) → review+
(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...

Updated

3 years ago
Depends on: 1178050
(Assignee)

Comment 12

3 years ago
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. :-)
(Assignee)

Comment 13

3 years ago
Ideally we wouldn't have to build that string at all and could just pass the array of pairs.
(Assignee)

Updated

3 years ago
No longer depends on: 1178050
https://hg.mozilla.org/mozilla-central/rev/b015ec383bc8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(Assignee)

Updated

3 years ago
Depends on: 1179031
You need to log in before you can comment on or make changes to this bug.