Closed Bug 1331736 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Version:
53 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: alexical, Assigned: alexical)

References

Details

(Whiteboard: [fce-active-legacy][fxsearch])

Attachments

(1 file)

Bug 1331736 - Use selected search suggestion with one-offs
59 bytes, text/x-review-board-request
mak
: review+
Details 
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.
Assignee: nobody → dothayer
Whiteboard: [fce-active]
We were discussing also about this in bug 1295458.
I think it makes a lot of sense to special case Search suggestions.
Priority: -- → P3
Whiteboard: [fce-active] → [fce-active][fxsearch]
Priority: P3 → P1
just to be sure, Doug, do you still plan to work on this?
Flags: needinfo?(dothayer)
Yeah, I should be able to get something in for this pretty soon.
Flags: needinfo?(dothayer)
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.
Attachment #8875394 - Flags: review?(mak77)
Status: NEW → ASSIGNED
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
Attachment #8875394 - Flags: review?(mak77) → review+
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f759d63d25b9
Use selected search suggestion with one-offs r=mak
https://hg.mozilla.org/mozilla-central/rev/f759d63d25b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
This will ride the trains from 55.
status-firefox53: affected → wontfix
status-firefox54: --- → wontfix
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
QA Whiteboard: [bugday-20170614]
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]
Verified as fixed using Nightly 55.0a1 (Build ID: 20170612030208) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
Simona, why did you mark this as verified disabled? It is actually enabled.
Flags: needinfo?(simona.marcu)
(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.
status-firefox55: verified disabled → verified
Flags: needinfo?(simona.marcu)
Whiteboard: [fce-active][fxsearch] → [fce-active-legacy][fxsearch]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: