Closed Bug 1443229 Opened 2 years ago Closed 2 years ago

Address bar shows search suggestions even when "Show search suggestions in address bar results" is unchecked

Categories

(Firefox :: Address Bar, defect)

53 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox58 --- unaffected
firefox59 blocking verified
firefox60 + verified

People

(Reporter: cpeterson, Assigned: jaws)

References

Details

Attachments

(4 files)

I see a lot of mysterious behavior from the "Show search suggestions in address bar results" feature in Beta 59 and Nightly 60. This bug might be related to bug 1438180.

* In a couple of my test user profiles, the address bar shows search suggestions even though I unchecked both the "Show search suggestions in address bar results" and "Show search suggestions ahead of browsing history in address bar results" checkboxes. 

* In my Windows laptop's user profile, I can check and uncheck the "Show search suggestions ahead of browsing history in address bar results" checkbox even when the "Show search suggestions in address bar results" checkbox is unchecked. I would expect the "Show search suggestions ahead of browsing history in address bar results" checkbox to be greyed-out.

* In my MacBook Pro's user profile, the "Show search suggestions ahead of browsing history in address bar results" checkbox is *always* grayed-out, so I can never check or uncheck it.

I don't see any obvious non-default pref values in about:support. I have a clean user profile that reproduces the problem. I can share the profile with anyone who wants it. I don't think this profile has any private user information in it, but I'm still reluctant to attach it to this public bug.
I bisected this regression to the following pushlog, which includes the fix for `browser.urlbar.suggest.searches` pref bug 1438180:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1162b0f53b85870409c6ffcf9075e43321da5588&tochange=ba1d6f897a923b561bd9458f77bd3f7f63b99573
[Tracking Requested - why for this release]:
This is really not good and sounds more severe than the original issue. It's late here and I'm still not 100% recovered from being ill this weekend. If anyone is able to investigate this before tomorrow morning Europe time that would be good. I wouldn't be opposed to backing out bug 1438180 from 59 if we don't have better options, though it'd be helpful if we understood the issue at least slightly better before doing that.

Drew / Jared, any chance you have time to poke at this?

Ryan, where are we with the release and is it too late to do something about this?
Flags: needinfo?(ryanvm)
Flags: needinfo?(jaws)
Flags: needinfo?(adw)
We're building Fx59rc1 today. There's a decent chance we'll end up needing another RC build later this week, though, so adding Liz to the thread so it's on her radar.
Flags: needinfo?(ryanvm) → needinfo?(lhenry)
(In reply to Chris Peterson [:cpeterson] from comment #0)
> I don't see any obvious non-default pref values in about:support. I have a
> clean user profile that reproduces the problem. I can share the profile with
> anyone who wants it. I don't think this profile has any private user
> information in it, but I'm still reluctant to attach it to this public bug.

Hm, but in about:config, what are the pref values for browser.search.suggest.enabled and browser.urlbar.suggest.searches ?
Flags: needinfo?(cpeterson)
Marking this as a blocker till I understand the issues better. 
We could land a backout now but it will likely miss the RC build.
Flags: needinfo?(lhenry)
I'm looking in to this now.
Flags: needinfo?(jaws)
Attached image Screenshot.jpg
(In reply to :Gijs (under the weather; responses will be slow) from comment #4)
> Hm, but in about:config, what are the pref values for
> browser.search.suggest.enabled and browser.urlbar.suggest.searches ?

browser.search.suggest.enabled = true (which is expected because "Provide search suggestions" is checked)

browser.urlbar.suggest.searches = true (which is NOT expected because "Show search suggestions in address bar results" is unchecked). Checking and unchecking "Show search suggestions in address bar results" in my test profile does NOT toggle the pref value in about:config.

If I manually set the browser.urlbar.suggest.searches pref = false, then the address bar stops showing search suggestions, as expected. But then the "Show search suggestions ahead of browsing history in address bar results" checkbox is grayed-out, regardless of whether "Show search suggestions in address bar results" is checked or not.

Here is a screenshot of two windows from the same Firefox Beta browser session demonstrating how the checkbox UI state is not consistent with the pref value or the address bar behavior.
Flags: needinfo?(cpeterson)
There is no "command" event listener for the checkbox anymore.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8956220 [details]
Bug 1443229 - Add a 'command' event listener for the urlbar search suggestions checkbox.

https://reviewboard.mozilla.org/r/225130/#review231102

::: browser/components/preferences/in-content/search.js:69
(Diff revision 1)
>      let updateSuggestionCheckboxes = this._updateSuggestionCheckboxes.bind(this);
>      suggestsPref.on("change", updateSuggestionCheckboxes);
>      urlbarSuggestsPref.on("change", updateSuggestionCheckboxes);
> +    let urlbarSuggests = document.getElementById("urlBarSuggestion");
> +    urlbarSuggests.addEventListener("command", () => {
> +      urlbarSuggestsPref.value = !urlbarSuggestsPref.value;

Shouldn't we read the checkbox's `checked` property here instead?

::: browser/components/preferences/in-content/tests/browser_searchsuggestions.js:35
(Diff revision 2)
> +     "Checkbox should be back to initial value after clicking it");
> +  is(Services.prefs.getBoolPref(URLBAR_SUGGEST_PREF_NAME), urlbarBox.checked,
> +     "Pref should match checkbox");
>  
> +  Services.prefs.setBoolPref(SUGGEST_PREF_NAME, false);
> +  ok(!urlbarBox.checked, "Checkbox should be now be unchecked");

double be

::: browser/components/preferences/in-content/tests/browser_searchsuggestions.js:58
(Diff revision 2)
>  add_task(async function() {
> -  Services.prefs.setBoolPref("browser.search.suggest.enabled", original);
> +  Services.prefs.setBoolPref(SUGGEST_PREF_NAME, original);
>  });

We already do this in a cleanup function. Why do we need to do it here, too? :-\
Attachment #8956220 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Chris Peterson [:cpeterson] from comment #0)
> * In my MacBook Pro's user profile, the "Show search suggestions ahead of
> browsing history in address bar results" checkbox is *always* grayed-out, so
> I can never check or uncheck it.

I'm assuming this is because in about:config in this profile, `browser.urlbar.suggest.searches' is actually false, and so because ticking the 'Show search search suggestions in in address bar results' checkbox doesn't alter the actual value of the pref, the other pref's checkbox's disabled state doesn't update either.
(In reply to :Gijs (under the weather; responses will be slow) from comment #4)
> Hm, but in about:config, what are the pref values for
> browser.search.suggest.enabled and browser.urlbar.suggest.searches ?

I don't think there is anything Windows or Mac specific in this bug. I see the same behavior and `browser.search.suggest.enabled` = true and `browser.urlbar.suggest.searches` = true pref values on both Windows and Mac.

In comment 0, I wrote:

> * In my MacBook Pro's user profile, the "Show search suggestions ahead of browsing history in address bar results" checkbox is *always* grayed-out, so I can never check or uncheck it.

That happened when I manually set the `browser.search.suggest.enabled` pref = false (because the "Show search suggestions in address bar results" checkbox was not toggling the pref value). I can reproduce the same behavior on Windows and Mac.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09978f543c01
Add a 'command' event listener for the urlbar search suggestions checkbox. r=Gijs
Flags: needinfo?(adw)
https://hg.mozilla.org/mozilla-central/rev/09978f543c01
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
This patch made today's nightly, but the result doesn't seem to work well for me. :-(

The first time I click the checkbox, the checkbox's value changed but the disabled state of the dependent pref is not altered.
The second time I click the checkbox, the checkbox's value stays the same and the disabled state of the dependent pref changes. :-\
Comment on attachment 8956475 [details]
Bug 1443229 - follow-up: use correct checkbox state,

https://reviewboard.mozilla.org/r/225386/#review231294
Attachment #8956475 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a10eb278c3b2
follow-up: use correct checkbox state, r=jaws
Fixed on 59 per discussion with lizzard by backing out bug 1438180 on mozilla-release. Tentatively marking fixed, I think Liz said she may consider taking both patches as ride-alongs for a dot-release.

https://hg.mozilla.org/releases/mozilla-release/rev/c98eb35cfd6d181f11211f191c04c14efb548e49
In case we want this for a dot-release, this + relanding bug 1438180 would work.
Attachment #8956601 - Flags: review+
Verified fixed in Nightly 60.0a1 (2018-03-06).
Status: RESOLVED → VERIFIED
I have managed to reproduce this issue using Firefox 60.0a1 (BuildId:20180305220117).

This issue is verified fixed using Firefox 59.0 (BuildId:20180307211018) on Windows 10 64bit, macOS 10.13.3 and Ubuntu 16.04 64bit.
Gijs, can you file a new bug for the issues in comment 23, so we can keep things straight with the tracking flags for 59, post-release?
Flags: needinfo?(gijskruitbosch+bugs)
See Also: → 1444158
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Gijs, can you file a new bug for the issues in comment 23, so we can keep
> things straight with the tracking flags for 59, post-release?

Filed 1444158.
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.