Closed Bug 1406234 Opened 7 years ago Closed 7 years ago

Set browser.urlbar.maxHistoricalSearchSuggestions value to 0 for 57/photon

Categories

(Firefox :: Address Bar, defect, P1)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 blocking fixed
firefox58 --- fixed

People

(Reporter: javaun, Assigned: past)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

Bug 1406234 - Remove historical search suggestions and move search suggestions on top.
59 bytes, text/x-review-board-request
mak
: review+
Details
Move history results on top again. rs=mak
1.26 KB, patch
Details | Diff | Splinter Review
[beta patch] Remove historical search suggestions. rs=mak
1.36 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
This was a new capability I pushed for in photon. After testing it, I don't think it's ready for 57. The matching is fairly greedy and it increases the likelihood that an irrelevant result will be tacked up in the most important slot.  

Let's disable for 57  by setting:

browser.urlbar.maxHistoricalSearchSuggestions to 0 by default
Priority: -- → P1
Whiteboard: [fxsearch]
Component: Search → Address Bar
We also have to set the urlbar composition for 57. 

We had a conversation today. I came in with a firm position that it should be 
* 10 overall results 
* 4 search results after the autocomplete, then
* 5 history results [0]

My argument is that search is a more prominent use case, and putting it below history doesn't give enough utility to search-heavy users, especially under unification. 

There are also heavy awesome bar users. Some may not mind history being below search suggestions. Some may mind A LOT. But users will be able to go into UI settings and turn off search suggestions in the address bar to have the original awesome bar back (and they can toggle the separate search bar on/off there too).

This is just our starting point for 57. Important to note: we can only have 1 address bar composition for both unified bar and 2 bar users. Hence I say put search first. we'll be doing testing and iteration well beyond 57 for things like result size (i.e. 7 total results), or for giving some new users history up top, or 1 top hit up top. These are things we've tested in pre-release shields but do not have complete understanding and can't make a generalizable answer at this time. This isn't final, it's our first step.


NI phlsa, shorlander, and wselman for visual input. They may want a slightly different combo for different visual balance or other effect

-------------------
[0] - The pref value to do 4 search then 5 history is: 
 "browser.urlbar.matchBuckets  = suggestion:4,general:5"
(In reply to Javaun Moradi [:javaun] from comment #1)
> There are also heavy awesome bar users. Some may not mind history being
> below search suggestions. Some may mind A LOT. But users will be able to go
> into UI settings and turn off search suggestions in the address bar to have
> the original awesome bar back (and they can toggle the separate search bar
> on/off there too).

I personally fear we'll break muscle memory of our users, and as such I'd suggest we keep at least 1 (or 2) history result at the top. This will benefit both users who prefer the old behavior and users who want search close to the top.
The alternative of disabling search suggestions seems to throw the baby out with the bath water, sure the user could, but that means it'll be mandatory to use 2 bars for those users.

> This is just our starting point for 57. Important to note: we can only have
> 1 address bar composition for both unified bar and 2 bar users.

Fwiw, in code we could switch the composition based on the layout.
Note that we also have a plan in bug 1386548 that will pretty much transform the address bar in the search bar, when the "?" cgar is typed or the keyboard shortcut to focus the search bar is used. That is another interesting path for search in Address Bar.
Assignee: nobody → past
Status: NEW → ASSIGNED
[Tracking Requested - why for this release]:
status-firefox57: --- → affected
This bug tracks unified search awesome bar result composition pref flip for 57 + removal of historical search suggestion. The plan is to request uplift to beta 57 by 10/20
tracking-firefox57: --- → ?
mak, I would definitely be interested to vary the composition based on bar layout (i.e. unified vs. 2). Right now, for users with a heavy awesome bar history workflow, they would have to turn off search suggestions to get back to their original awesome Bar.
The risk I see is that those users will disable search suggestions, and we'll never be able to re-enable them, since we don't know if they disabled them for privacy or just for annoyance. Thus, I fear we're giving up the unified experience for them.
Comment on attachment 8918302 [details]
Bug 1406234 - Remove historical search suggestions and move search suggestions on top.

https://reviewboard.mozilla.org/r/189130/#review195360

Technically, provided we pass all the tests, it looks good.

I still have my functionality concerns, but I already expressed those :)

::: browser/modules/test/browser/browser_UsageTelemetry_urlbar.js:110
(Diff revision 3)
>  
>    // Clear historical search suggestions to avoid interference from previous
>    // tests.
>    await SpecialPowers.pushPrefEnv({"set": [["browser.urlbar.maxHistoricalSearchSuggestions", 0]]});
>  
> +  // Use the default matching bucket configuration.

Did you check on Try that this is the only failure? I'd suggest to do a full xpcshell + firefox-ui-functional + mochitest-browser-chrome and use --rebuild 3 to re run the test and ensure we are not introducing a ton of new intermittents.
Attachment #8918302 - Flags: review?(mak77) → review+
That is a good idea, thanks. Turns out that everything was fine except for browser_searchEngine_behaviors.js:

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

However that seems to fail anyways on artifact builds as I get it to fail without my patch applied. I will file a separate bug to fix this.
sounds similar to bug 1408730
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d352a380a67d
Remove historical search suggestions and move search suggestions on top. r=mak
Mak, Phlsa is back today and I laid out your concerns on breaking muscle memory for existing (2 bar) users. 2 things we should consider here. 

1. (Fix for 57): If this is a concern, do we leave existing users (who also keep the search bar) with history first.
2. Longer term (beyond 57) do we enable users in UI settings to choose between history first/search first.
Filed bug 1406234 for discussion:
Wrong bug! Filed bug 1409450 for discussion on whether to add a user choice for history first or search suggestions first in address bar.
https://hg.mozilla.org/mozilla-central/rev/d352a380a67d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Blocks: 1409810
Mak, Past, don't uplift this one to 57 yet. We're going to figure out how to let existing users keep their bar how it is. But we need to assess our implementation options first. The default may go one way (or another) and then we'll solve some of this with off-trains tech (Shield or GoFaster). 

For now, leave this as is and we'll get clarity this week. Regardless of which browser.urlbar.matchBuckets option we land on, we want to turn off maxHistoricalSearchSuggestions
Flags: needinfo?(past)
Flags: needinfo?(mak77)
We'll wait.
Flags: needinfo?(past)
Flags: needinfo?(mak77)
(In reply to Panos Astithas [:past] (please ni?) from comment #13)
> That is a good idea, thanks. Turns out that everything was fine except for
> browser_searchEngine_behaviors.js:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0e55a6acd845153ebda9b24d74051828e1b3fe86
> 
> However that seems to fail anyways on artifact builds as I get it to fail
> without my patch applied. I will file a separate bug to fix this.

Filed bug 1410005.
Reopening this. 

Mak, we're going to leave the Awesome Bar composition exactly as it is, which means history results go first. However, we still want to turn off historical search suggestions until we can test it more. 

Two requirements:

1. Make history first. Revert to prior state. That means undoing/deleting this pref we set in commment 1:
"browser.urlbar.matchBuckets  = suggestion:4,general:5" <-- UNDO THAT

2. We still want to turn off historical searches:

browser.urlbar.maxHistoricalSearchSuggestions to 0 by default


=============
(The longer version for those curious. New installs will get unified bars in 57, and we can use Shield to selectively give some of them a search-first experience. This allows us to continue to iterate w/o breaking existing users' workflows.)
Status: RESOLVED → REOPENED
Flags: needinfo?(mak77)
Resolution: FIXED → ---
Thanks everyone for re-evaluating the change, I know it's a very complex problem. We'll continue with the technical work to improve both the history and search experiences.
Panos, rs=me on a pref flipping patch.
Flags: needinfo?(mak77)
MozReview-Commit-ID: 6NhmsVdM9Nm
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fddb6059f1ce
Move history results on top again. rs=mak
Comment on attachment 8921196 [details] [diff] [review]
[beta patch] Remove historical search suggestions. rs=mak

Approval Request Comment
[Feature/Bug causing the regression]: bug 1181644
[User impact if declined]: The matching is fairly greedy and it increases the likelihood that an irrelevant result will be tacked up in the most important slot.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes, this is essentially reverting this pref to the value it had until 8/25
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not at all
[Why is the change risky/not risky?]: it is just a pref flip and a return to the prior default at that
[String changes made/needed]: none
Attachment #8921196 - Flags: approval-mozilla-beta?
Comment on attachment 8921196 [details] [diff] [review]
[beta patch] Remove historical search suggestions. rs=mak

Fixes a blocking bug, pref flip, beta57+
Attachment #8921196 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/fddb6059f1ce
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
(In reply to Panos Astithas [:past] (please ni?) from comment #28)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes, this is essentially reverting
> this pref to the value it had until 8/25
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on past's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
I've managed this issue in Firefox 58.0a1 (2017-10-05) (64-bit) from Manjaro Linux (64 Bit) 

This Bug is now verified as fixed on Latest Firefox Beta 58.0b6 (64-bit) (Build ID: 20171123161455)

and Latest Firefox 57.0 (64-bit) (Build ID: 20171115203031)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20171122]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: