1331736 - Alt+down in location bar searches for original search term rather than selected search term

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Alex Thayer [:alexical] (she/her)]

I noticed this while working on bug 1330462, and I think it should be somewhat easy to fix, provided we know what it should do.

Steps:

- Type "corgis" in the location bar
- Hit the down key until you have a search suggestion selected, like "corgis near me"
- Hit alt+down, which will let you cycle through one-off search engine buttons, until you're on, say, Bing
- Notice that on the highlighted search suggestion line, it says something like "corgis near me -- Search with Bing"
- Hit return, and notice that you searched Bing for "corgis", and not "corgis near me"

The same steps in the search bar will result in searching Bind for "corgis near me." It seems we missed this bit when adapting the one-off buttons for the location bar. I feel like the best course of action is just to make the location bar match the behavior of the search bar in this regard, no? What seems to be getting in the way is this getter in urlbarBindings.xml:

```
      <property name="oneOffSearchQuery">
        <getter><![CDATA[
          // this.textValue may be an autofilled string.  Search only with the
          // portion that the user typed, if any, by preferring the autocomplete
          // controller's searchString (including handleEnterInstance.searchString).
          return (this.handleEnterInstance && this.handleEnterInstance.searchString) ||
                 this.mController.searchString ||
                 this.textValue;
        ]]></getter>
      </property>
```

I think we can work around this fairly easily so we keep the previous behavior so long as the user hasn't scrolled through the search suggestions.

Comment 1

[Marco Bonardo [:mak]]

We were discussing also about this in bug 1295458.
I think it makes a lot of sense to special case Search suggestions.

Comment 3

[Marco Bonardo [:mak]]

just to be sure, Doug, do you still plan to work on this?

Comment 4

[Alex Thayer [:alexical] (she/her)]

Yeah, I should be able to get something in for this pretty soon.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1331736 - Use selected search suggestion with one-offs

Currently in the location bar we don't match the search bar's
behavior of allowing the user to use their currently selected
search suggestion with a one-off search engine. This is due to
logic which is intended to not search for autocompleted text,
instead favoring only what the user has actually typed. This patch
adds an exception to that logic for search suggestions.

Review commit: https://reviewboard.mozilla.org/r/146836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146836/

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8875394 [details]
Bug 1331736 - Use selected search suggestion with one-offs

https://reviewboard.mozilla.org/r/146836/#review151240

::: browser/base/content/test/urlbar/browser_urlbarOneOffs_searchSuggestions.js:53
(Diff revision 1)
> +                                   `http://mochi.test:8888/?terms=foobar`);
> +  EventUtils.synthesizeKey("VK_RETURN", {})
> +  await resultsPromise;
> +
> +  gBrowser.removeTab(gBrowser.selectedTab);
> +});

it would be great to also test a mouse click, so select the search suggestion with the keyboard, then click on the one-off

::: browser/base/content/urlbarBindings.xml:676
(Diff revision 1)
>  
>        <property name="oneOffSearchQuery">
>          <getter><![CDATA[
> +          if (this._isSearchSuggestion) {
> +            return this.textValue;
> +          }

what I don't like about this approach is that it requires adding to the toolkit implementation stuff specific to the urlbar. Unfortunately we did that often, if possible we should try to limit though.

Additionally, _isSearchSuggestion in this case would also be set to true when we completeselectedindex any other entry, like a url for example, not just search suggestions. That'd be wrong.

There's another way to detect if the current value is a search suggestion though, that is using this.value instead of this.textValue.
For search suggestions this.value will be, for example:
moz-action:searchengine,{"engineName":"Google","input":"porcupine%20tree","searchQuery":"porcu","searchSuggestion":"porcupine%20tree"}
you can use this._parseActionUrl(this.value) to get a parsed object, if the object is not nullcheck if action.type is searchengine, and then you can use action.input that should always be the right value to search for.

Comment 7

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8875394 [details]
Bug 1331736 - Use selected search suggestion with one-offs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146836/diff/1-2/

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8875394 [details]
Bug 1331736 - Use selected search suggestion with one-offs

https://reviewboard.mozilla.org/r/146836/#review151754

r=me with a green(ish) Try run.

::: browser/base/content/test/urlbar/browser_urlbarOneOffs_searchSuggestions.js:11
(Diff revision 2)
> +          ["browser.urlbar.suggest.searches", true]],
> +  });
> +  let engine = await promiseNewSearchEngine(TEST_ENGINE_BASENAME);
> +  let oldCurrentEngine = Services.search.currentEngine;
> +  Services.search.moveEngine(engine, 0);
> +  Services.search.currentEngine = engine;

Just in case, should you clear history before running the test? a previous test may have opened a page containing "foo".

::: browser/base/content/test/urlbar/browser_urlbarOneOffs_searchSuggestions.js:25
(Diff revision 2)
> +});
> +
> +// Presses the Return key when a one-off is selected after selecting a search
> +// suggestion.
> +add_task(async function oneOffReturnAfterSuggestion() {
> +  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);

it would be better (to avoid possible intermittents) if you'd use let tab = await BrowserTestUtils.openNewForegroundTab and await BrowserTestUtils.removeTab(tab);

::: browser/base/content/test/urlbar/browser_urlbarOneOffs_searchSuggestions.js:57
(Diff revision 2)
> +  await PlacesTestUtils.clearHistory();
> +});
> +
> +// Clicks a one-off engine after selecting a search suggestion.
> +add_task(async function oneOffClickAfterSuggestion() {
> +  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);

ditto

Comment 9

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8875394 [details]
Bug 1331736 - Use selected search suggestion with one-offs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146836/diff/2-3/

Comment 10

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8875394 [details]
Bug 1331736 - Use selected search suggestion with one-offs

https://reviewboard.mozilla.org/r/146836/#review151754

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f759d63d25b9
Use selected search suggestion with one-offs r=mak

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Sylvestre Ledru [:Sylvestre]]

This will ride the trains from 55.

Comment 14

[Md. Majedul islam]

I have reproduced this bug with Nightly 53.0a1 (2017-01-17) on Ubuntu 16.04, 64 bit!

The Bug's fix is now verified on Latest Nightly 56.0a1.

Build ID 	20170613100218

User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 15

[Tanvir Rahman]

I have reproduced this bug with Nightly 53.0a1 (2017-01-17) on Windows 10, 64-bit.

The fix is now verified on latest Nightly 56.0a1
 
Build ID 	20170613030203
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170614]

Comment 16

[Simona Badau, Desktop QA]

Verified as fixed using Nightly 55.0a1 (Build ID: 20170612030208) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.12.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Simona, why did you mark this as verified disabled? It is actually enabled.

Comment 18

[Simona Badau, Desktop QA]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #17)
> Simona, why did you mark this as verified disabled? It is actually enabled.

Sorry Panos, it was by mistake. Set the right tracking flag. Thanks for the notice.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Thank you!