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

VERIFIED FIXED in Firefox 64

Status

()

defect
P1
normal
VERIFIED FIXED
8 months ago
8 months ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
Firefox 65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox64 verified, firefox65 verified)

Details

(Whiteboard: [fxsearch], )

Attachments

(2 attachments)

Assignee

Description

8 months ago
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.
Flags: needinfo?(mverdi)
Assignee

Updated

8 months ago
Summary: Show only autosuggest results when search only engine alias is typed in the address bar → Search aliases: @engine text should remain in the urlbar when highlighting search suggestion results, and modified suggestions should search with the @ engine
(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.
Flags: needinfo?(mverdi)
Assignee

Comment 2

8 months ago
(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
Assignee

Comment 3

8 months ago
* 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 7

8 months ago
Pushed by dwillcoxon@mozilla.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b185d07420f
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee

Comment 10

8 months ago
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".
Flags: qe-verify+
Assignee

Comment 11

8 months ago
Posted patch Beta/64 patchSplinter Review
[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
Attachment #9020386 - Flags: approval-mozilla-beta?
Flags: in-testsuite+
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.
Attachment #9020386 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.