Closed
Bug 1500516
Opened 6 years ago
Closed 6 years ago
Search aliases: @engine text should remain in the urlbar when highlighting search suggestion results, and modified suggestions should search with the @ engine
Categories
(Firefox :: Address Bar, defect, P1)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 65
People
(Reporter: adw, Assigned: adw)
References
()
Details
(Whiteboard: [fxsearch])
Attachments
(2 files)
46 bytes,
text/x-phabricator-request
|
Details | Review | |
30.23 KB,
patch
|
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•6 years 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
Comment 1•6 years ago
|
||
(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•6 years 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•6 years 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)
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years 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•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Comment 9•6 years ago
|
||
bugherder |
Assignee | ||
Comment 10•6 years 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•6 years ago
|
||
[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?
Updated•6 years ago
|
Flags: in-testsuite+
Comment 12•6 years ago
|
||
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+
Comment 13•6 years ago
|
||
bugherder uplift |
Comment 14•6 years ago
|
||
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.
Description
•