Closed Bug 1423247 Opened 3 years ago Closed 3 years ago

Couple Awesome Bar Composition (matchBuckets pref) to user's Address bar state (unified vs. 2 bar)

Categories

(Firefox :: Address Bar, defect, P1)

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

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix

People

(Reporter: javaun, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

Bug 1423247 - Couple Awesome Bar Composition (matchBuckets pref) to user's Address bar state (unified vs. 2 bar).
59 bytes, text/x-review-board-request
mak
: review+
Details
Beta 58 patch
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)
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.
Target Milestone: --- → Firefox 58
Version: 57 Branch → 58 Branch
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee: nobody → adw
Status: NEW → ASSIGNED
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.
Let's see if that fixes the TV failure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78c05939beda
Sigh, looks like that didn't run the TV tests.  Here's another:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=768c5dd2ca63
OK, that last push looks good.
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+
[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: --- → ?
Thanks, this addresses your comments.  I'll autoland this now.
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
https://hg.mozilla.org/mozilla-central/rev/3325c31e498c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: Firefox 58 → Firefox 59
Attached patch Beta 58 patch — — Splinter Review 
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 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+
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 → ---
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
Closing per comment 20. Bug 1426216 is what we'll do instead.
Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
Resolution: --- → WONTFIX
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.