1500516 - Search aliases: @engine text should remain in the urlbar when highlighting search suggestion results, and modified suggestions should search with the @ engine

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Drew Willcoxon :adw]

See bug 1496814 comment #6 (copied below).  Bruce points out two main problems:

(1) When you type "@engine query" and then highlight a search suggestion result, the text in the urlbar changes to "query suggestion", but we should keep the @engine text: "@engine query suggestion".

(2) When you type "@engine query", highlight a search suggestion result, modify it by continuing to type, and then press the enter key, we do a search using the default engine, but we should search using the @ engine.

(2) seems more like an obvious problem than (1) to me.  But (1) probably follows from (2).  i.e., if it's ambiguous in (2) which engine will be used, then showing @engine in the text would make it clear.

Verdi, any thoughts?

(There's a third problem below of the one-off header being wrong, but I'd like to focus this bug on the main interaction.)

(In reply to Bruce from bug 1496814 comment #6)
> Using the arrow keys to highlight a search suggestion ends up removing the
> search shortcut from the urlbar. Is this intentional? To be more specific:
> 
> STR:
> 1. Type `@google test`
> 2. Use down arrow key to highlight say the second suggestion
> 
> AR: urlbar says `testbook`. If I continue typing at this point, the search
> query is no longer being sent to Google but to my default engine.
> 
> ER: urlbar says `@google testbook` and Google remains "active." This feels
> more intuitive to me and allows[1] for further editing of the search query.
> 
> Also after STR1, the one-off section says `Search for @google test with:`
> followed by the usual list. After STR2, it says `Search for testbook with:`.
> Bug 1498023 will fix this, iiuc.
> 
> 
> [1]Unsure if I'm explaining it correctly so as an example: I would like to
> be able to do the following:
> 
> Goal: Search Google for `telescope price`. Google is not the default engine.
> 
> Expected "lazy" steps:
> i) Type `@google teles`
> ii) Use down arrow to pick the `telescope` suggestion. At this point, urlbar
> should read `@google telescope` and continue to be in "queries being sent to
> Google" mode
> iii) Type out ` p`. First suggestion is `telescope price`. Select it, urlbar
> changes to `@google telescope price`, press enter.

Comment 1

[Verdi [:verdi] Best to slack me]

(In reply to Drew Willcoxon :adw from comment #0)
> See bug 1496814 comment #6 (copied below).  Bruce points out two main
> problems:
> 
> (1) When you type "@engine query" and then highlight a search suggestion
> result, the text in the urlbar changes to "query suggestion", but we should
> keep the @engine text: "@engine query suggestion".
> 
> (2) When you type "@engine query", highlight a search suggestion result,
> modify it by continuing to type, and then press the enter key, we do a
> search using the default engine, but we should search using the @ engine.
> 
> (2) seems more like an obvious problem than (1) to me.  But (1) probably
> follows from (2).  i.e., if it's ambiguous in (2) which engine will be used,
> then showing @engine in the text would make it clear.
> 
> Verdi, any thoughts?
> 

Yes, for (1) we should keep @engine in the bar until/unless the user deletes it or part of it. And for (2) the active engine should always be whatever is specified by @engine and it shouldn't change if the user alters the query. 


> (There's a third problem below of the one-off header being wrong, but I'd
> like to focus this bug on the main interaction.)
> 

Yes please, we also need to fix Bug 1498023 - we don't want the one-off UI any time we're using a search shortcut.

> (In reply to Bruce from bug 1496814 comment #6)
> > [1]Unsure if I'm explaining it correctly so as an example: I would like to
> > be able to do the following:
> > 
> > Goal: Search Google for `telescope price`. Google is not the default engine.
> > 
> > Expected "lazy" steps:
> > i) Type `@google teles`
> > ii) Use down arrow to pick the `telescope` suggestion. At this point, urlbar
> > should read `@google telescope` and continue to be in "queries being sent to
> > Google" mode
> > iii) Type out ` p`. First suggestion is `telescope price`. Select it, urlbar
> > changes to `@google telescope price`, press enter.

Yes - it should work that way.

Comment 2

[Drew Willcoxon :adw]

(In reply to Verdi [:verdi] from comment #1)
> Yes please, we also need to fix Bug 1498023 - we don't want the one-off UI
> any time we're using a search shortcut.

I have a question about this that I'll ni you on over there

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1500516 - Search aliases: @engine text should remain in the urlbar when highlighting search suggestion results, and modified suggestions should search with the @ engine

* Slightly rework the logic that makes `searchSuggestionsCompletePromise` so that it checks `this.hasBehavior("searches")` and `this._inPrivateWindow` earlier so that it can avoid getting the query string and truncating it (along with the pref accesss)
* Get rid of the `input` param to `_addSearchEngineMatch`. It's only used for forcing a trailing space for alias results that don't have a query, but `_addSearchEngineMatch` can detect that case on its own -- no need for an `input` param.
* A slightly unrelated change: I noticed that when the spec shows a search for "@amazon telescopes", the first suggestion is not "telescopes", like it actually is in Firefox, but "telescopes for adults". That makes sense. There's no point in having the first suggestion echo back the heuristic result. It's better not to because (1) there's no visual dupe and (2) you don't have to press the down arrow key twice to get to non-dupe suggestions. So I added some logic to the suggestions fetching to ignore suggestions that are duplicates of the search string. I changed `_searchEngineAliasMatch` to `_searchEngineHeuristicMatch` because of course you can do searches without using an alias, and this new logic needs the query string in that case.
* Slightly rework `_addSearchEngineMatch` to be a little more straightforward
* Fix `head_autocomplete.js` so it intelligently compares moz-action results instead of a simple string comparison (and hope that the object is stringified the same way)

Comment 4

[Drew Willcoxon :adw]

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

Comment 5

[Drew Willcoxon :adw]

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

Comment 6

[Drew Willcoxon :adw]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b185d07420f
Search aliases: @engine text should remain in the urlbar when highlighting search suggestion results, and modified suggestions should search with the @ engine r=mak

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1b185d07420f

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1b185d07420f

Comment 10

[Drew Willcoxon :adw]

STR for QA:

1. Type "@bing test" in the urlbar

2. Verify that the results in the popup are the following -- more or less, the exact suggestions after the first result may vary:

[Bing logo] test -- Search with Bing
[magnifying glass] testout
[magnifying glass] testosterone
[magnifying glass] testing
[magnifying glass] test internet speed
etc.

This result should *not* appear anywhere:
[magnifying glass] test

3. Press the Down arrow key to highlight each result.  Each time a result is highlighted, the text in the urlbar should change to "@bing [suggestion]" for whatever suggestion you've highlighted.

4. Highlight one of the results, type some other characters or word, and then press the Return key.

5. Verify that a Bing search is performed for the search string that you used.  For example, if you highlighted the "testing" result and then typed " foo" so that the whole search string is "testing foo", then Bing should have been searched for "testing foo".

Comment 11

[Drew Willcoxon :adw]

Attachment: Beta/64 patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1496814

User impact if declined: The UX for bug 1496814 and the @search keyword feature in general in the urlbar will be incorrect on 64

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Please see comment 10

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Affects only the @search keyword feature, covered by automated tests and manual testing, and it's early in the cycle

String changes made/needed: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9020386 [details] [diff] [review]
Beta/64 patch

[Triage Comment]
Improves the UX for bug 1496814 and the @search keyword feature shipping in 64. Approved for 64.0b5.

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/38e139673b19

Comment 14

[Maria Berlinger [:mberlinger], Services QA]

I've managed to reproduce this issue on Firefox 63.0b10 using Windows 10 64bit.

This is verified fixed using Firefox 64.0b5 and 65.0a1 (BuildId:20181031223503) on the following OSes: Windows 10 x64, Ubuntu 16.04 x86 and macOS 10.13.6.