Closed
Bug 1423247
Opened 5 years ago
Closed 5 years ago
Couple Awesome Bar Composition (matchBuckets pref) to user's Address bar state (unified vs. 2 bar)
Categories
(Firefox :: Address Bar, defect, P1)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: javaun, Assigned: adw)
References
Details
(Whiteboard: [fxsearch])
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
mak
:
review+
|
Details |
9.50 KB,
patch
|
Details | Diff | Splinter Review |
In 57, we now have the option of a unified bar or two separate bars (address bar + legacy search bar). Existing profiles kept 2 bars, new installs got unified by default. Users can change to the other figuration at any time in Settings > Search. The default Awesome Bar composition in Fx is 5 history results, then 4 search suggestions. Existing users still see this. For new, unified installs in 57, we use Shield to dynamically alter the Awesome Bar search-first, which is the expectation many new users have. Objective: This work is to formalize that by coupling the Awesome Bar composition prefs to the state of the User's bar configuration. Requirements: The pref that controls Bar unification is: `browser.search.widget.inNavBar` False = unified. True = 2 bars. When users have unified bar, their composition should be set to 4 search results, 5 history. This is achieved by setting: browser.urlbar.matchBuckets = suggestion:4, general:5 (NOTE: this pref does not exist by default in Fx) When users have 2 bar Firefox, they should see 5 history results, then 4 search (if available). This is current state and achieved by setting: browser.urlbar.matchBuckets = general:5 (or browser.urlbar.matchBuckets = general:5, suggestion:4)
Reporter | ||
Comment 1•5 years ago
|
||
The above is highly prescriptive, just to lay out the prefs available. The objective here is to harden what we're already running in Shield. We've been watching this for a few weeks and things are very stable. We have high confidence. We will continue to iterate and test in Shield, but given how things look (and that we have the window to land in Fx 58), let's do that and give ourselves extra measure of stability.
Reporter | ||
Updated•5 years ago
|
Target Milestone: --- → Firefox 58
Version: 57 Branch → 58 Branch
Updated•5 years ago
|
Priority: -- → P1
Assignee | ||
Updated•5 years ago
|
Whiteboard: [fxsearch]
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•5 years ago
|
||
This modifies nsBrowserGlue to set the pref initially and add a CUI observer to keep the pref updated as the searchbar is added and removed from the UI. I changed the default value in UnifiedComplete to place suggestions at the top, since that matches the default behavior of not showing the searchbar. I continued the current policy where the pref does not exist by default. If the pref value is customized -- i.e., the user changed it to something other than the non-unified value -- then I don't touch the pref at all so that I don't override what the user set.
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=158fa081d133
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•5 years ago
|
||
Forgot to hg-add the test. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ded358bdb13e
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•5 years ago
|
||
Let's see if that fixes the TV failure. https://treeherder.mozilla.org/#/jobs?repo=try&revision=78c05939beda
Assignee | ||
Comment 9•5 years ago
|
||
Sigh, looks like that didn't run the TV tests. Here's another: https://treeherder.mozilla.org/#/jobs?repo=try&revision=768c5dd2ca63
Assignee | ||
Comment 10•5 years ago
|
||
OK, that last push looks good.
Comment 11•5 years ago
|
||
mozreview-review |
Comment on attachment 8936724 [details] Bug 1423247 - Couple Awesome Bar Composition (matchBuckets pref) to user's Address bar state (unified vs. 2 bar). https://reviewboard.mozilla.org/r/207460/#review213514 ::: browser/components/nsBrowserGlue.js:2366 (Diff revision 3) > + this._checkWidget(widgetID); > + }, > + onWidgetRemoved: (widgetID, area) => { > + this._checkWidget(widgetID); > + }, > + }); Off-hand it looks like you could: let checkWidget = id => { if (id == ...) this.updatePref(); }; CustomizableUI.addListener({ onWidgetAdded: checkWidget, onWidgetRemoved: checkWidget }); ::: browser/components/tests/browser/browser_urlbar_matchBuckets.js:18 (Diff revision 3) > +// that queues an idle callback. > +add_task(async function waitForIdle() { > + await new Promise(resolve => { > + Services.tm.idleDispatchToMainThread(resolve); > + }); > +}); Sounds like a nice addition for TestUtils.jsm, it already contains some of these wait utils at the top.
Attachment #8936724 -
Flags: review?(mak77) → review+
Comment 12•5 years ago
|
||
[Tracking Requested - why for this release]: per comment 1 product wants it in 58. I gave Sylvestre a heads up about this yesterday.
tracking-firefox58:
--- → ?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•5 years ago
|
||
Thanks, this addresses your comments. I'll autoland this now.
Comment 15•5 years ago
|
||
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3325c31e498c Couple Awesome Bar Composition (matchBuckets pref) to user's Address bar state (unified vs. 2 bar). r=mak
Comment 16•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3325c31e498c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: Firefox 58 → Firefox 59
Assignee | ||
Comment 17•5 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Not a regression [User impact if declined]: Please see comments above [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: Maybe? 1. New profile 2. Type "moz" in the urlbar 3. Verify that "moz"-related search suggestions appear above the Mozilla-related bookmarks that are installed by default 4. Move the searchbar to the navbar 5. Repeat step 2 6. Verify that now the Mozilla-related bookmarks appear above the search suggestions 7. Restart Firefox and do steps 5 and 6 again [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Not technically, no [Why is the change risky/not risky?]: Technically it's a small simple patch that simply changes a hidden preference [String changes made/needed]: None
Attachment #8937193 -
Flags: approval-mozilla-beta?
Comment 18•5 years ago
|
||
Comment on attachment 8937193 [details] [diff] [review] Beta 58 patch This is for product requirement and unify user experience. Beta58+.
Attachment #8937193 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•5 years ago
|
status-firefox58:
--- → affected
Comment 19•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/873f72d1eeb9
Updated•5 years ago
|
Flags: qe-verify+
Reporter | ||
Comment 20•5 years ago
|
||
I screwed up here. The intent was correct, but even as I said in comment 1, this was highly prescriptive. The outcome we need here is that users need to be able to choose whether search or history comes first in the address bar. Users have different workflow styles and we want to accommodate all. The criticism here from UX is that we should not couple address bar composition to number of bars (unified vs. 2) because there is no clear disclosure to the user that changing their bar configuration would also change their composition of results. I spoke with Past and we're going to back this one out of 58 and take a new approach targeting 59. The new approach is in bug 1426216 and we will make a new UI pref allowing users to choose history or search first. We'll also set their default based on a reasonable heuristic (i.e. were they a new install/unified user as of 57)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•5 years ago
|
Comment 21•5 years ago
|
||
Backout by aciure@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd09f7ff978 Backed out changeset 3325c31e498c for taking a new approach targeting Mozilla59 r=backout a=backout on a CLOSED TREE
Comment 22•5 years ago
|
||
Backed out changeset 873f72d1eeb9 (bug 1423247) for taking a new approach targeting Mozilla59 r=backout a=backout https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd09f7ff978a6c20a14f977f21d3e42b440405b https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e4d571577ef19fdf20a6a67635ea1e3d69d59ee8&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Target Milestone: Firefox 59 → ---
Comment 23•5 years ago
|
||
Beta backout: Backed out changeset 873f72d1eeb9 (bug 1423247) for taking a new approach targeting Mozilla59 r=backout a=backout https://hg.mozilla.org/releases/mozilla-beta/rev/422fb348439770b71d08a87f8f8cf800e404b327 https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=94e939c04e269ad8f8c454a1b3b5635c2ccff8c5&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Comment 26•5 years ago
|
||
Closing per comment 20. Bug 1426216 is what we'll do instead.
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → WONTFIX
Updated•5 years ago
|
Comment 27•5 years ago
|
||
Comment on attachment 8937193 [details] [diff] [review] Beta 58 patch this was backed out, clearing beta approval flag.
Attachment #8937193 -
Flags: approval-mozilla-beta+
You need to log in
before you can comment on or make changes to this bug.
Description
•