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

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Chris Peterson [:cpeterson]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[:Gijs (he/him)]

[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?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 4

[:Gijs (he/him)]

(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 ?

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I'm looking in to this now.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

There is no "command" event listener for the checkbox anymore.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1443229 - Add a 'command' event listener for the urlbar search suggestions checkbox.

This was inadvertently removed when the preference attribute was removed to clean up a race condition.

Review commit: https://reviewboard.mozilla.org/r/225130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225130/

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8956220 [details]
Bug 1443229 - Add a 'command' event listener for the urlbar search suggestions checkbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225130/diff/1-2/

Comment 11

[:Gijs (he/him)]

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? :-\

Comment 12

[:Gijs (he/him)]

(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.

Comment 13

[Chris Peterson [:cpeterson]]

(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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8956220 [details]
Bug 1443229 - Add a 'command' event listener for the urlbar search suggestions checkbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225130/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09978f543c01
Add a 'command' event listener for the urlbar search suggestions checkbox. r=Gijs

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/09978f543c01

Comment 17

[:Gijs (he/him)]

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 18

[:Gijs (he/him)]

Attachment: Bug 1443229 - follow-up: use correct checkbox state,

It seems `doCommand` runs through a different codepath than just clicking the checkbox,
and as a result the outcome of the command handler is different that way.

This aligns the automated test closer to what happens when you 'manually'
click the checkbox, and fixes the bug in the command handler.

Review commit: https://reviewboard.mozilla.org/r/225386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225386/

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8956475 [details]
Bug 1443229 - follow-up: use correct checkbox state,

https://reviewboard.mozilla.org/r/225386/#review231294

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a10eb278c3b2
follow-up: use correct checkbox state, r=jaws

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/a10eb278c3b2

Comment 22

[:Gijs (he/him)]

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

Comment 23

[:Gijs (he/him)]

Attachment: Patches combined for release

In case we want this for a dot-release, this + relanding bug 1438180 would work.

Comment 24

[Chris Peterson [:cpeterson]]

Verified fixed in Nightly 60.0a1 (2018-03-06).

Comment 25

[Emil Ghitta, QA [:emilghitta]]

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.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 27

[:Gijs (he/him)]

(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.