1176437 - Fallback "Search with" first result should not appear when pressing Down in an empty awesomebar (i.e., search string is an empty string)

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Drew Willcoxon :adw]

Attachment: patch

If you press the down arrow key while the awesomebar is empty, the results include the usual "Search with" first item, except it's empty because there is no search string.  When that item is highlighted, you get:

<magnifying glass> - Search with Google

And when it's not highlighted:

<magnifying glass>

Seems like _matchHeuristicFallback should not fall back to _matchCurrentSearchEngine when !this._originalSearchString.

Comment 1

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

Comment on attachment 8624964 [details] [diff] [review]
patch

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

This doesn't completely solve the problem, now the "Search With" item doesn't appear anymore, but selecting the first item doesn't do anything yet :(
Did you plan to manage that elsewhere?
Regardless, this needs a test, and I'm sure we have at least a couple tests that will fail with this patch.

Comment 4

[Drew Willcoxon :adw]

Attachment: proposal v2

How's this approach?  The popup's onResultsAdded looks for a special "initialresult" style in the first result.  If it has it, then it selects it.  Otherwise it doesn't.  UnifiedComplete adds initialresult to its first result's style when it's one of the heuristic matches.

It works in my testing, but you're right that automated tests need to be updated, which I'll do if this is OK.

Comment 5

[Drew Willcoxon :adw]

Or we could add a new nsIAutoCompleteController method like canInitiallySelectFirstResult().  That would save having to update a bunch of UnifiedComplete tests that check for styling (at the expense of updating other nsIAutoCompleteController implementations, but there aren't many of those).

Comment 6

[Drew Willcoxon :adw]

I think maybe I meant nsIAutoCompleteResult instead of nsIAutoCompleteController.  I see that there's nsIAutoCompleteResult.defaultIndex, "Index of the default item that should be entered if none is selected," which sounds like what we want here... but it's being used for autofill looks like?

And speaking of autofill, the latest patch messes it up, but your feedback would still be helpful.

Comment 7

[Drew Willcoxon :adw]

Look at this weird conditional in UnifiedComplete.js by the way:

1509   get _shouldAutofill() {
1510     // First of all, check for the autoFill pref.
1511     if (!Prefs.autofill)
1512       return false;
1513 
1514     if (!this._searchTokens.length == 1)
1515       return false;

Should be != 1 probably?

Comment 8

[Drew Willcoxon :adw]

Attachment: proposal v3

This uses defaultIndex instead of the initialresult style from the previous patch.  It adds nsIAutoCompleteController.defaultIndex.  Would you ever want the initially selected result to be different from the autofilled result (if autofill is enabled)?  Doesn't seem like it, so this makes them the same.

I modified _addMatch to set defaultIndex to 0 if UnifiedComplete is building the heuristic result.  That's the only place now that UnifiedComplete sets defaultIndex (except initially, to -1).

The nsAutoCompleteController::CompleteValue change is kind of unrelated, but it's to prevent this problem that confused me while working on this:

1. Start typing a previously typed URL in the awesomebar so that it's autofilled
2. Press the Esc key twice to delete the text
3. Press the Down key
4. Result: The URL that was autofilled in step 1 is autofilled again, and the first result is an empty "Search with"

The empty "Search with" result definitely isn't right, but it doesn't seem right to me either to autofill the URL again.  I didn't type anything.

Comment 9

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

Comment on attachment 8627988 [details] [diff] [review]
proposal v3

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

The patch looks mostly good, but I didn't have enough time to test it so I need some more time for that.

I also want to verify some things:
1. getNextIndex may cause us issues if we don't have a pre-selection? I guess the user might be unable to escape the popup after he enters it. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1481
2. the completeValue change requires a bit more attention on my side
3. could setting defaultValue for non autofill stuff break the user entered text?

I don't have enough time today to verify all of these, so I'll have to delay to tomorrow/monday.

::: toolkit/components/places/UnifiedComplete.js
@@ +795,5 @@
>      // |hasFirstResult| is used to keep track of whether we've obtained such a
>      // result yet, so we can skip further heuristics and not add any additional
>      // special results.
>      let hasFirstResult = false;
> +    this._addingHeuristicMatch = true;

Could you please initialize this in the Search constructor with a comment explaining what's it?
I know it's unneeded cause we don't reuse searches, but still would make things clearer.

Also, I wonder if we could merge these 2 booleans, a this._waitingHeuristicFirstMatch, set to true in the constructor, and to false once done.

@@ +869,2 @@
>          yield this._matchHeuristicFallback();
> +        this._addingHeuristicMatch = false;

I think we should completely refactor this fallback. The only reason we can't run urlQuery and we need a prediction, is that the query is using AUTOCOMPLETE_MATCH function. And it's using AUTOCOMPLETE_MATCH cause we strip "http", "https", "ftp", "www", so it looked easier to just scan the whole table.
We can rather do a mixup of the current predict query and an actual proper query. For example, supposing we are searching for "mozilla.org/a", we might build a query like 

SELECT :query_type, h.url, NULL, f.url AS favicon_url,
       foreign_count > 0 AS bookmarked,
       NULL, NULL, NULL, NULL, NULL, NULL, h.frecency
FROM moz_places h
LEFT JOIN moz_favicons f ON h.favicon_id = f.id
WHERE (rev_host = "gro.allizom." OR rev_host = "gro.allizom.www.")
AND h.frecency <> 0
AND fixup_url(h.url) BETWEEN "mozilla.org/a" AND "mozilla.org/a" || X'FFFF'
${conditions}
ORDER BY h.frecency DESC, h.id DESC
LIMIT 1

(this requires changing fixup_url and a few other details).

I'm going to file a bug to do exactly this, since it will make the code much easier to follow.
in the meanwhile your workaround here sounds ok.

Comment 10

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

(In reply to Marco Bonardo [::mak] from comment #9)
> 1. getNextIndex may cause us issues if we don't have a pre-selection? I
> guess the user might be unable to escape the popup after he enters it.
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> urlbarBindings.xml#1481

This is indeed a problem, if you mouse down from the empty locationbar, you enter the popup and then you can't escape anymore.
I think we can tweak getNextIndex so that it allows the selection to escape the popup if defaultIndex == -1 (compare the override code with the original code in autocomplete.xml)

> 3. could setting defaultValue for non autofill stuff break the user entered
> text?

Off-hand this seems to be working fine.

Comment 11

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

(In reply to Marco Bonardo [::mak] from comment #9)
> 2. the completeValue change requires a bit more attention on my side

I actually think this bug should be fixed elsewhere, in particular, looks like it's either MaybeCompletePlaceholder that needs some sort of fix or mPlaceholderCompletionString must be .Truncate()d somewhere and it's not.
I suspect the latter, maybe RevertTextValue() should do that.

Comment 12

[Drew Willcoxon :adw]

Attachment: WIP v4

This factors out the heuristic matching and addresses your comments except for the one about setting _waitingHeuristicFirstMatch in the constructor, since factoring out the heuristic matching better addresses that I think.

I'd like feedback on this part:

(In reply to Drew Willcoxon :adw from comment #8)
> Created attachment 8627988 [details] [diff] [review]
> proposal v3
> 
> This uses defaultIndex instead of the initialresult style from the previous
> patch.  It adds nsIAutoCompleteController.defaultIndex.  Would you ever want
> the initially selected result to be different from the autofilled result (if
> autofill is enabled)?  Doesn't seem like it, so this makes them the same.

Actually yes, when you type a bookmark keyword, the first result should be the keyword heuristic result, and it should be selected, but we shouldn't autofill.  test_keywords.js actually tests that.  So overloading defaultIndex like that won't work.

This patch goes back to using a result style ("heuristic"), on the first result only, to tell the front-end whether to select it.  Not sure whether it would be better to add an attribute on nsIAutoCompleteController, like initiallySelectedIndex.  What do you think?  It would be simple to change.

Comment 13

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

If I read this correctly, this is not removing the pointless Search with entry, right?
What about we just hide the first entry visually if the panel is open in this 'empty' mode? In the end the action is correct, it's just the first entry that is wrong.
It should (ideally) be a much simpler hack

Comment 14

[Drew Willcoxon :adw]

It does remove the first search entry, due to the this._originalSearchString term in the final fall-back conditional in _matchFirstHeuristicResult.  So there won't be a searchengine first result when the search string is empty.

Hmm, I'm not sure I agree with you.  The action is not correct IMO.  The action is searchengine, but there is no search string, so how can you do a search?  And more broadly, the user hasn't typed anything, so how can there be any action at all?  So I do think that not adding the heuristic match in this case is the right thing to do.

Comment 15

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

Sorry, I meant:
the action executed when pressing enter (absolutely nothing!) is correct, but the first action row says "Search with", just because it's the final fallback.
Hiding the first row may reach the expected result with just a different hack. My fear is that most unified complete code has been built thinking we always had a first selected row and not having it may expose bugs.
Also, consider in future we might always want to hide the action row and merge (append) its information into the input field itself (based on discussion with shorlander)

Btw, I will evaluate your approach better, I was just trying to have a fallback alternative in case it could be needed.

Comment 16

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

Comment on attachment 8636305 [details] [diff] [review]
WIP v4

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

I think we have some test expecting the Search with in the empty search, did you run tests yet? I'd be surprised if none of them would fail.

Note I didn't test this deeply, yet.

::: toolkit/components/places/UnifiedComplete.js
@@ +1250,5 @@
>        this._maybeRestyleSearchMatch(match);
>      }
>  
> +    if (this._addingHeuristicFirstMatch) {
> +      match.style += " heuristic";

I know we called these heuristic internally, but from the outside it's not saying much... what about "preselected" or "default"? The former would clarify its usage, at least.

Comment 17

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

(In reply to Marco Bonardo [::mak] from comment #16)
> I know we called these heuristic internally, but from the outside it's not
> saying much... what about "preselected" or "default"? The former would
> clarify its usage, at least.

nevermind. I changed my mind, since, as I said, sooner or later we might want to hide this row, "heuristic" is generic enough.

Comment 18

[Drew Willcoxon :adw]

Attachment: for-review patch v1

This updates the unifiedcomplete xpcshell tests.  Not sure yet whether any browser chrome tests are affected, but here's a try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66057d68488a

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 8642690 [details] [diff] [review]
for-review patch v1

Yeah, there are some failing browser tests.  I'll fix them and ask for review on the new patch.

Comment 20

[Drew Willcoxon :adw]

Attachment: for-review patch v2

Needed to remove "heuristic" from `types` in _adjustAcItem and update browser_action_keyword.js.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e957b01933

Comment 21

[Drew Willcoxon :adw]

Comment on attachment 8643171 [details] [diff] [review]
for-review patch v2

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

I'm still open to adding an attribute/method on some interface instead of including "heuristic" in the style, if you think that would be better.  Wouldn't have to update all these tests then I think?

::: toolkit/components/places/UnifiedComplete.js
@@ +829,5 @@
>      }
>      queries.push(this._searchQuery);
>  
> +    // Add the first heuristic result, if any.  Set _addingHeuristicFirstMatch
> +    // to true so that when the result is added, defaultIndex can be set to 0.

Need to update this comment.

Comment 22

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

(In reply to Drew Willcoxon :adw from comment #21)
> I'm still open to adding an attribute/method on some interface instead of
> including "heuristic" in the style, if you think that would be better. 
> Wouldn't have to update all these tests then I think?

Were you thinking to something like nsIAutoCompleteSimpleResult::setHasHeuristicFirstMatch?
This is very specific to our implementation, maybe a result should have a generic params container that consumers can fill?
Then we'd also need a getResultAt exposed from the controller since urlbarbindings has only access to the controller

Comment 23

[Drew Willcoxon :adw]

I was thinking nsIAutoCompleteController.selectFirstResult (or a better name).  The only reason defaultIndex didn't work is because I was overloading it to mean two things.  So then I chose to add a style thinking it would be less invasive than modifying idl but maybe it's not.

What do you think?

Comment 24

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #12)
> This patch goes back to using a result style ("heuristic"), on the first
> result only, to tell the front-end whether to select it.  Not sure whether
> it would be better to add an attribute on nsIAutoCompleteController, like
> initiallySelectedIndex.  What do you think?  It would be simple to change.

Comment 25

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

The search (UnifiedComplete) doesn't have access to the controller though, so how is it supposed to set that?
The search has access to the result, the view has access to the controller, controller has access to both. That's why I was suggesting the result interface.

Comment 26

[Drew Willcoxon :adw]

Yeah, it would have to go through the result, like defaultIndex.  Maybe the controller interface wouldn't need to be updated at all.

But that's just details.  The important question is do you think that would be better than using a style?  I don't have an opinion, so I'm inclined to stick with the latest patch here, but you're the reviewer. :-)

Comment 27

[Drew Willcoxon :adw]

If we're sticking with the style, I'll need to update this too I think:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#1393

So that it doesn't choose "heuristic".  Would be better to pass the whole `types` string to the panel and let it decide how to create the label.

Comment 28

[Drew Willcoxon :adw]

Think I'd like to alter that label getter completely for bug 1190366 actually.

Comment 29

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

I think using style is fine.
Cause otherwise the only thing I could suggest would be to add params to each match, that would be a space separated string, and then add getParamsAt... in the end it would just be an additional style field. This would make sense if style should be a single worded string, but looks like we already injected action into it, so the eggs are broken already.
Let's try to make the code easier to maintain in case in future we add more styles.

Comment 30

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

Comment on attachment 8643171 [details] [diff] [review]
for-review patch v2

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

in addition to the code you found in comment 21 and comment 27, you should also update:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#766

  this.mController.getStyleAt(this.popup.selectedIndex) == "autofill") {

::: browser/base/content/test/general/browser_action_keyword.js
@@ +38,5 @@
>    let result = yield promise_first_result("keyword something");
>    isnot(result, null, "Expect a keyword result");
>  
> +  let types = result.getAttribute("type").split(/\s+/).sort();
> +  is(types.toString(), "action,heuristic,keyword", "Expect correct `type` attribute");

the test aims to check action and keyword are there, so I'd make it check only those 2, others should be ignored.

::: browser/base/content/urlbarBindings.xml
@@ +1506,5 @@
>  
> +          // Do not allow the selection to be removed if UnifiedComplete is
> +          // enabled and the popup's first result is a heuristic result.
> +          let controller =
> +            this.view.QueryInterface(Ci.nsIAutoCompleteController);

I suspect you might use this.input.mController

@@ +1507,5 @@
> +          // Do not allow the selection to be removed if UnifiedComplete is
> +          // enabled and the popup's first result is a heuristic result.
> +          let controller =
> +            this.view.QueryInterface(Ci.nsIAutoCompleteController);
> +          let styles = controller.getStyleAt(0).split(" ");

I guess it might be sane to check we have at least one match.

@@ +1647,5 @@
>              if (!Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete"))
>                return;
>  
> +            // If nothing is selected yet, select the first result if the
> +            // controller allows it.

comment needs an update, it's not the controller allowing the selection.

::: toolkit/content/widgets/autocomplete.xml
@@ +1688,5 @@
>            this._titleOverflowEllipsis.hidden = false;
>  
>            let types = new Set(type.split(/\s+/));
>            let initialTypes = new Set(types);
> +          types.delete("heuristic");

Please create a boolean var
let hasAction = types.has("action");
then immediately remove both heuristic and action here so all the removal code is in one place. Please also move here the comment that is currently before the action delete and update it.

Comment 31

[Drew Willcoxon :adw]

Attachment: for-review patch v3

(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #30)
> ::: toolkit/content/widgets/autocomplete.xml
> @@ +1688,5 @@
> >            this._titleOverflowEllipsis.hidden = false;
> >  
> >            let types = new Set(type.split(/\s+/));
> >            let initialTypes = new Set(types);
> > +          types.delete("heuristic");
> 
> Please create a boolean var
> let hasAction = types.has("action");
> then immediately remove both heuristic and action here so all the removal
> code is in one place. Please also move here the comment that is currently
> before the action delete and update it.

There are a couple of other types that are also removed, autofill and search, so I removed them here too.  And since there's already an initialTypes set, I used that instead of separate booleans for each.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ba009f142d

Comment 32

[Drew Willcoxon :adw]

Typo:

> diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
> --- a/browser/base/content/urlbarBindings.xml
> +++ b/browser/base/content/urlbarBindings.xml
> @@ -1624,17 +1624,17 @@ file, You can obtain one at http://mozil
>  
>        <method name="createResultLabel">
>          <parameter name="aTitle"/>
>          <parameter name="aUrl"/>
>          <parameter name="aTypes"/>
>          <body>
>            <![CDATA[
>              let label = aTitle + " " + aUrl;
> -            let type = types.find(v => v != "action" && v != "heuristic");
> +            let type = aTypes.find(v => v != "action" && v != "heuristic");
>              try {
>                // Some types intentionally do not map to strings, which is not
>                // an error.
>                label += " " + this._bundle.GetStringFromName(type + "ResultLabel");
>              } catch (e) {}
>  
>              return label;
>            ]]>

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41bb007ebf5f

Comment 33

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

Comment on attachment 8644521 [details] [diff] [review]
for-review patch v3

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

Some things to fix yet, but nothing blocking

::: browser/base/content/test/general/browser_action_keyword.js
@@ +39,5 @@
>    isnot(result, null, "Expect a keyword result");
>  
> +  let types = result.getAttribute("type").split(/\s+/);
> +  Assert.ok(types.includes("action"));
> +  Assert.ok(types.includes("keyword"));

includes on Arrays is only available in Nightly for now, please keep using .contains()

See bug 1070767

::: browser/base/content/urlbarBindings.xml
@@ +762,5 @@
>  
>            // Trim popup selected values, but never trim results coming from
>            // autofill.
> +          let styles =
> +            this.popup.selectedIndex < 0 ? [] :

why < 0? afaik it can only be -1 and the previous == -1 check was better imo

@@ +764,5 @@
>            // autofill.
> +          let styles =
> +            this.popup.selectedIndex < 0 ? [] :
> +            this.mController.getStyleAt(this.popup.selectedIndex).split(/\s+/);
> +          if (styles.includes("autofill")) {

ditto for includes.

Looks like this is also changing the if condition. Before it was true when there was no selection or style included autofill.
now it's true only if style includes autofill...
This sounds like an unwanted change off-hand

@@ +1511,5 @@
> +          let styles =
> +            this.input.mController.matchCount == 0 ? [] :
> +            this.input.mController.getStyleAt(0).split(" ");
> +          if (!Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete") ||
> +              !styles.includes("heuristic")) {

ditto on includes

@@ +1647,5 @@
>              if (!Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete"))
>                return;
>  
> +            // If nothing is selected yet, select the first result if it allows
> +            // it (by being a "heuristic" result).

'if it is a pre-selected "heuristic" result. (See UnifiedComplete.js)'

@@ +1652,3 @@
>              if (this._matchCount > 0 && this.selectedIndex == -1) {
> +              let styles = this.input.mController.getStyleAt(0).split(" ");
> +              if (styles.includes("heuristic")) {

ditto on includes

Comment 34

[Drew Willcoxon :adw]

Thanks Marco.

(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #33)
> ::: browser/base/content/test/general/browser_action_keyword.js
> @@ +39,5 @@
> >    isnot(result, null, "Expect a keyword result");
> >  
> > +  let types = result.getAttribute("type").split(/\s+/);
> > +  Assert.ok(types.includes("action"));
> > +  Assert.ok(types.includes("keyword"));
> 
> includes on Arrays is only available in Nightly for now, please keep using
> .contains()

There's no array.contains() I don't think.  I'll use indexOf().

Comment 35

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

ah yes contains was removed in bug 1075059 and then was decided to introduce includes, but only in Nightly (very confusing).

Comment 36

[Drew Willcoxon :adw]

Attachment: patch v4

I'll land this if try is green.  It should be.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b57d0b02413

Comment 37

[Pulsebot]

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

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/907891e1c211