Closed Bug 1181173 Opened 9 years ago Closed 9 years ago

Conflicting duplicate checkboxes in locationbar and search.

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: mhoye, Assigned: mossop)

References

Details

(Whiteboard: [UX][unifiedautocomplete][suggestions][fxsearch])

Attachments

(3 files)

Search Suggestion Preferences / Settings Changes - 01
227.43 KB, image/png
Details
strings
2.40 KB, patch
mak
: review+
Details | Diff | Splinter Review
patch
9.88 KB, patch
mak
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
General -> Search -> "provide search suggestions"

and

Privacy -> Location Bar -> "Related Searches from the default search engine"

seem to do the same thing.

If either one is unchecked, I don't see search suggestions in either of the search box or the location bar. This is pretty confusing when you click "change search settings" and it takes you to a checkbox saying that should be working.
Because many users don't want to send any text typed in the locationbar to a third party, but they are fine with having that in the search bar cause it's differently scoped. So a single toggle won't do for them.

Yes, the link to settings is confusing, UX still working on that.
Stephen, what's the UX bug tracking work to make these prefs more understandable?
Flags: needinfo?(shorlander)
I don't know that we have a specific UX bug for this yet.
Flags: needinfo?(shorlander)
Then let's track this for now
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [unifiedautocomplete][suggestions][fxsearch]
I may not have been clear here, but from my perspective the bug is not "these checkboxes are kind of the same", it's that when I uncheck _either one_ of those two checkboxes, I do not get search suggestions in either of the location bar or the search box. If I uncheck location-bar related searches, I don't get search suggestions in either the location bar or the search bar.
Maybe I'm misunderstanding, this is what to expect:
1. the Show suggestions in Search pane only affects search panes (search bar, about:home, about:newTab)
2. the Related Searches... from privacy locationbar only affects the locationbar.
There is no connection between the 2 checkboxes.
They should not be linked, but right now they appear to be.
If I'm the only person who has ever noticed this problem, it seems likely we don't need both of those checkboxes.
(In reply to Mike Hoye [:mhoye] from comment #8)
> If I'm the only person who has ever noticed this problem, it seems likely we
> don't need both of those checkboxes.

I think we do need them both so people can continue to isolate functionality between the two inputs. Just need to figure out what that looks like :)
From a conversation with adw for references' sake: 

14:54 <adw> search suggestions in the ui, including both the search bar and url bar, are handled by this module:
            http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/SearchSuggestionController.jsm#20
14:54 <adw> which obeys browser.search.suggest.enabled
14:54 <adw> so if that pref is false, that module won't return any suggestions even when it's asked to
14:54 <adw> and then in the url bar...
14:55 <adw> the suggest.searches pref is obeyed starting here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#39
14:56 <adw> in the latter link, you can see that small collection of prefs i mentioned earlier for controlling what appears in the url bar results
14:57 <mhoye> huh. OK, thank you.
14:59 <adw> that suggest.searches url pref gets converted into this flag on an autocomplete interface at some point -- it's kind of convoluted --
            http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/mozIPlacesAutoComplete.idl#105
14:59 <mhoye> "kind of"
14:59 <adw> which ultimately gets checked here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#881
Mike and I talked about this a little.  There are a couple of different points:

(1) Why do both browser.search.suggest.enabled and browser.urlbar.suggest.searches exist?  (But that's addressed by comment 9, for example.)

(2) A weird bug: Mike's saying that when he unchecks "Related searches from the default search engine" (browser.urlbar.suggest.searches=false), he no longer sees suggestions in the search bar, which is wrong.  Not sure how that could happen.
FWIW I am seeing the same thing happen.
If you guys set/verify the preferences manually through about:config, instead of using the preferences UI, does the problem still happen?  There's another bug about a race condition in the UI affecting the checkboxes -- but that's only the Location bar checkboxes in the Privacy pane I think.
(In reply to Marco Bonardo [::mak] from comment #6)
> Maybe I'm misunderstanding, this is what to expect:
> 1. the Show suggestions in Search pane only affects search panes (search
> bar, about:home, about:newTab)
> 2. the Related Searches... from privacy locationbar only affects the
> locationbar.
> There is no connection between the 2 checkboxes.

This isn't right, actually the Search pane checkbox affects all suggestions in the browser, even the location bar suggestions.  See comment 10.
based on IRC discussion, we might want the 2 prefs to visually be in a hierarchy, something like (better text needed)

 [ ] Provide Search Suggestions
     [ ] Allow the location bar to retrieve related searches

The only miss would be users looking to disable search suggestions in Privacy / Location Bar where all the other options exist, for them we could provide a link to the preferences Search Pane like "Manage Search Suggestions settings", in the end those are still privacy related.
since we will be moving the prefs around, I think we first need bug 1173939 to land, that should move most of the code to unified complete and allow for a cleaner patch here.
Depends on: 1173939
Rank: 15
Whiteboard: [unifiedautocomplete][suggestions][fxsearch] → [UX][unifiedautocomplete][suggestions][fxsearch]
we need UX for this bug :) - ask Marco for any details if needed.
Flags: needinfo?(shorlander)
if we want to do something, we have only 3 work days left to land the strings before the merge.
we are still missing UX
Flags: needinfo?(sescalante)
(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #19)
> if we want to do something, we have only 3 work days left to land the
> strings before the merge.
> we are still missing UX

I am not totally happy with this approach because it adds more confusion to our already confusing prefs. But it mostly aligns with the solution we came up with a few weeks ago.

1) A global suggest pref in the Search pane — "Provide search suggestions"
2) A sub-pref for just Location Bar searches — "Show search suggestions in location bar results"
3) A link from Privacy back to the Search pane in case someone looks there first — "Change preferences for search engine suggestions…"

I think we should follow-up on this and reevaluate the taxonomy of these prefs.
Flags: needinfo?(shorlander) → needinfo?(mak77)
I somehow missed that you already outlined this in comment 16 :)
ok, we just need to land the strings for now. I wonder if Dave has time to do that.
Flags: needinfo?(mak77) → needinfo?(dtownsend)
Attached patch strings — — Splinter Review 
(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #22)
> ok, we just need to land the strings for now. I wonder if Dave has time to
> do that.

Here they are, for whoever wants to review them first. Access keys in the privacy pane are getting pretty difficult to find.
Flags: needinfo?(dtownsend)
Attachment #8644596 - Flags: review?(mak77)
Attachment #8644596 - Flags: review?(adw)
Comment on attachment 8644596 [details] [diff] [review]
strings

Review of attachment 8644596 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, please remember to add leave-open keyword when lading
Attachment #8644596 - Flags: review?(mak77)
Attachment #8644596 - Flags: review?(adw)
Attachment #8644596 - Flags: review+
Keywords: leave-open
Flags: needinfo?(sescalante)
Assignee: nobody → dtownsend
Comment on attachment 8644146 [details]
Search Suggestion Preferences / Settings Changes - 01

If the location bar checkbox is ticked when you untick the global search suggestions box the location bar box becomes disabled, should it also become unticked? When you tick the global box again should the location bar box become ticked again?
Flags: needinfo?(shorlander)
(In reply to Dave Townsend [:mossop] from comment #27)
> When you tick the global box again should the location bar box
> become ticked again?

I'm pretty sure this would be considered a privacy hit since it could override the previous user's choice. If we need to untick the location bar pref (stephen will tell), we need to mirror its previous value in a separate pref and then set that back, imo.
(In reply to Dave Townsend [:mossop] from comment #27)
> Comment on attachment 8644146 [details]
> Search Suggestion Preferences / Settings Changes - 01
> 
> If the location bar checkbox is ticked when you untick the global search
> suggestions box the location bar box becomes disabled, should it also become
> unticked? When you tick the global box again should the location bar box
> become ticked again?

The location bar specific sub-option should retain its state when enabling/disabling the global.

1) Both checkboxes are checked:
   A) User unchecks global option = global option is disabled, sub-option is disabled but *not* unchecked
   B) User checks global at a later time = global option is enabled, sub-option is set back to previous checked/enabled state

2) Global is checked; location bar is unchecked:
   A) User unchecks global option = global option is disabled, sub-option remains disabled
   B) User checks global at a later time = global option is enabled, sub-option remains disabled
Flags: needinfo?(shorlander)
Attached patch patch — — Splinter Review 
Good, that matches what I already implemented.
Attachment #8648106 - Flags: review?(mak77)
Comment on attachment 8648106 [details] [diff] [review]
patch

Review of attachment 8648106 [details] [diff] [review]:
-----------------------------------------------------------------

did we stop supporting legacy preferences dialog now that in-content shipped? Bug 1140495 looks blocked from some time. I see you are only handling in-content prefs here, I'm fine with that if the plan to remove the legacy ones is proceeding.

::: browser/components/preferences/in-content/search.js
@@ +45,5 @@
>      Services.obs.addObserver(this, "browser-search-engine-modified", false);
>      window.addEventListener("unload", () => {
>        Services.obs.removeObserver(this, "browser-search-engine-modified", false);
>      });
> +

we must ensure autocomplete is running when this pref can be touched, see _initAutocomplete in http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#50
That's because autocomplete keeps synchronization of autocomplete prefs (at least until we finally remove browser.autocomplete.enabled)
so we also need to init autocomplete here, in search.js init function (didn't check it exists but I assume so)

@@ +52,5 @@
> +
> +    let updateSuggestsCheckbox = () => {
> +      urlbarSuggests.disabled = !suggestsPref.value;
> +    }
> +    suggestsPref = document.getElementById("browser.search.suggest.enabled")

missing "let", and even if it's the same code-wise, I'd prefer if it would be defined before the handler, for readability.

::: browser/components/preferences/in-content/tests/browser_searchsuggestions.js
@@ +7,5 @@
> +  yield openPreferencesViaOpenPreferencesAPI("search", undefined, { leaveOpen: true });
> +
> +  let doc = gBrowser.selectedBrowser.contentDocument;
> +  let urlbarBox = doc.getElementById("urlBarSuggestion");
> +  is(urlbarBox.disabled, false, "Checkbox should be enabled");

nit: might use ok(!urlbarBox.disabled... and ok(urlbarBox.disabled... below

@@ +34,5 @@
> +  gBrowser.removeCurrentTab();
> +});
> +
> +add_task(function*() {
> +  Services.prefs.setBoolPref("browser.search.suggest.enabled", original);

I guess this may stay in a registerCleanupFunction call at the top
Attachment #8648106 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #31)
> Comment on attachment 8648106 [details] [diff] [review]
> patch
> 
> Review of attachment 8648106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> did we stop supporting legacy preferences dialog now that in-content
> shipped? Bug 1140495 looks blocked from some time. I see you are only
> handling in-content prefs here, I'm fine with that if the plan to remove the
> legacy ones is proceeding.

I've been told that we don't need to support them anymore.
Comment on attachment 8648106 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Search suggestions
[User impact if declined]: If search suggestions are turned on users will see confusing UI in preferences for controlling it
[Describe test coverage new/current, TreeHerder]: On m-c with automated tests.
[Risks and why]: This is a simple UI change with low risk.
[String/UUID change made/needed]: No strings changed, this is revealing strings that were previously unused (prelanded) in the UI though
Attachment #8648106 - Flags: approval-mozilla-aurora?
Comment on attachment 8648106 [details] [diff] [review]
patch

Sure, let's take it. Thanks
Attachment #8648106 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
we forgot to remove leave-open, this is FIXED!
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Verified as fixed using Dev Edition 43.0a2 2015-09-25.
Status: RESOLVED → VERIFIED
Sorry to ask here, but I think it might be the best place.

In order to update/fix the Sumo article [1] mentioning the "Provide search suggestions" and "Show search suggestions in location bar results" options for 43 as well as current 42 at this point, that article recently got updated. However, it was a bit hard to find out about their exact behavior, and the current info is based on what I read here. Could anyone please check if the info is correct? (See the article’s history and use the version selector on the left if needed.)

When OK, I also wondered if the second option could need some clarification by adding "engine" (e.g. "Show search engine suggestions in …" or "Show suggestions provided by search engines in …") or similar/better to distinguish the Bookmarks/History results and the online search engine’s suggestions a little more. (Per comment 0, it used to include "default search engine".) I know it’s just called Search suggestions elsewhere like on [2], but I’d consider any search suggestion to be either from offline sources or from the selected search provider included. Anyhow, if the 2nd option is only valid for the location bar’s search engine results and not globally like option 1 appears to be, that may help. Users may now think option 2 is a general search option including Bookmarks and History for the location bar instead and not enirely comprehend the privacy issue involved.

Since the offline (Bookmarks/History) searches still/always work when disabling the first option, which seems incorrect reading comment 15 (at least when considering "suggestions" there to include bookmarks/history and search _engine_ suggestions), the first option may also raise some questions on what that’s valid for to some users, so maybe clarifying that by adding that it only applies to search engines for the Search box, about:newtab and about:home would be a good thing to do as well.

In short: I can imagine both options are unclear to users on what they apply to (i.e. any search, search engines, or both?), especially when the first had a different meaning before option 2 was added if that’s the case. Any ideas?

[1] https://support.mozilla.org/en-US/kb/awesome-bar-search-firefox-bookmarks-history-tabs#w_changing-your-location-bar-settings
[2] https://support.mozilla.org/en-US/kb/search-suggestions-firefox
Hi Ton, thanks for asking.  The article is correct.

(In reply to Ton from comment #40)
> Anyhow, if the 2nd option is only valid for the location bar’s
> search engine results and not globally like option 1 appears to
> be

That's right.

To me, both of these checkboxes are in the Search pref pane, which once you look at it is clearly about search engines, not local history and bookmarks, so I don't think the options' wording needs any clarification.  On the other hand it couldn't hurt.  But I'll needinfo Stephen for a more authoritative opinion.  He came up with the wording in comment 20.
Flags: needinfo?(shorlander)
Ton, one thing I would change:

> Add a check mark next to Provide search suggestions there to
> enable search suggestions from your preferred search engine for
> the Search bar, home page or New Tab page,

I think you should say "and New Tab page", not "or", since suggestions appear in all of those places, not just some of them.
I agree with Drew, the fact is in the Search pane and the fact it's a child of the other Search suggestions checkbox should clarify the context enough.
Drew, Marco, thanks very much for the feedback. I applied the small "or" to "and" edit to the article as requested.

Regarding both checkboxes, I agree about the other options applying to search _engines’_ suggestions too and see that now, though basically all options in the Search pane currently do. However and in general, options functionality should probably be clear from a nearby header or description rather than from similar options. 

The "Search" pane title also implies these settings refer to any search, where those from search engines and especially their _input_ suggestiuons are just a part of IMO. Please keep in mind that "search suggestions" (as in "search results" from the URL bar) in Firefox have applied to offline searches only for years, which is likely what confused me and may confuse others.

So in fact, the word "search engine suggestions" in the Privacy pane currently is the only place indicating these options apply to search engine communication (hence residing under Privacy I suppose) while not everyone passes by them before entering the Search panel. All together I think it wouldn’t hurt to consider some future rewording, where "Provide search suggestions" may be the most confusing option - thinking of the 3 items it only applies to (Search bar etc.), though that’s where the support link/article comes in.

I’ll stay out of this now, though I’d be curious to hear about UX opinions and user feedback. Maybe changing wordings should be postponed until the "Location bar" section in the Privacy pane will be moved to the Search pane (if that ever happens) to emphasize their difference. ;)
Flags: needinfo?(shorlander)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: