1223983 - Disabling search suggestion settings leaves suggestion on screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

On a clean profile:
1) Hit toolbar
2) Search for "sup" (click the Yahoo search widget rather than autocompleting to sumo)
3) Click url bar
4) Type "s"
5) Enable search suggestions & confirm "sup" is shown as a search history item
6) Hit back & go to Settings -> Search -> Disable search suggestions & search history
7) Click url bar
8) Type "s"

Expected: The "sup" appears and the search term
Actual: the network search suggestions still appear (bug 1210534 doesn't appear to fix it in this case)

9) Type "u" for "su"
10) Go back and go to Settings -> Search and disable "Show search history"
11) Click the url bar
12) Type "s" (same problem as before), then type "d"

Expected: "sup" goes away
Actual: "sup" is present

It seems the search history is always present. I've also hit weird other states when testing for this bug.

I remember this all working correctly when we only had the single search suggestions preference – due to the complexity, perhaps it's better to just have one preference in the initial release?

Ally, any ideas?

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #0)
> 6) Hit back & go to Settings -> Search -> Disable search suggestions &
> search history

I believe this is supposed to be "Disable just network search suggestions" (i.e. not history)

Comment 2

[Allison Naaktgeboren :ally]

I'm really sorry, but I haven't been able to reproduce this. We talked at about this at the dev-lounge today more about what you're seeing in more detail (and clarifying step 6 vs 10), but on my nexus 9 all behaves as expected as I toggle between the 4 combinations of settings and the sup/su examples. I'll try the samsung galaxy next and whatever the worst phone pdx has to offer later today.

The two prefs are set independently of each other, in different classes, and the 4 different combinations of the 2 settings each have a different code path in updateSuggestions(). So I don't think its likely that they're stomping on each other. The prefs aren't updating as quickly as we need is possible. UpdateSuggestions() is called just about every onLayout().

For the saved searches, we check the prefs system (GeckoSharedPrefs) every time in updateSuggestions() in SearchEngineRow. It is the case that if the system cannot find the pref for whatever reason, we default to on for that one. That doesn't match the behavior mentioned on call with toggling it off and still showing.  

The network suggestions/search engine suggestions pref is a little murkier, controlled by mSuggestionsEnabled in BrowserSearch. The value comes out of a json blob in setSearchEngines() or mSuggestionsEnabled is set directly if the user turns on the suggestions from the prompt. mSuggestionsEnabled is passed directly to updateSuggestions().

How are you exiting the settings panel? Maybe the problem is that the settings changes aren't getting saved?

Leaving NI on myself for followup on the 2 other phones.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

via triage, I didn't see this before bug 1200367 so tracking the release that landed in.

Haven't read comment 2 yet but I'll work with Ally to see if we can't reproduce it again.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self to read comment 2.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I repro'd again on my local build on my GS4:
  https://www.youtube.com/watch?v=xBVWZ4cUbGg&feature=youtu.be

After step 8, the network search suggestions appear only for the first character (like bug 1210534). Then one search history result stays on the screen – perhaps we're not correctly recycling views? Then, I re-enable network search suggestions where the search history View stays on the screen (even though the preference is still disabled).

I'm using the hardware back button to leave settings & the awesomescreen. Notably this device has a hardware menu button which I've also been using.

I can reproduce on my Nexus 9, however, the first expected/actual (after step 8) is not reproducible when I click the 3-dot menu directly from the BrowserSearch screen to open Settings. The second is still reproducible. On phones the 3-dot menu button is not visible which could partially explain the discrepancy.

Comment 6

[Allison Naaktgeboren :ally]

The recording is really helpful.

Are you on the latest build? I was using the 3dot menu and the back arrow in settings to navigate from search to settings and back on today's fxteam.

Do you actually turn on search engine suggestions via the doorhanger? It isn't clear from the movie, but the setting is checked the first time you visit the settings panel. 

For disabling search engine suggestions, the unbolded search term is correctly 's'. It looks like the setting update hasn't reach BrowserSearch there yet. Visiting the search settings page always triggers setSearchEngines() now, though the call is not always immediate. I did see some behavior like this on slow/old devices, along with outdated network suggestions from latency. That it doesn't reproduce on your nexus 9 or mine suggests that latency alone may be to blame for that one.

For the issue after you disable saved searches, I notice that the search term in the saved search 'sd' but 'sup' shown with an unbolded section of 's'.

If I go really fast on my nexus 9, I can get the search term to be 'supppp' or 'supper' but the search result displayed is 'sup' with 'su' unbolded as the search term. I still can't reproduce your original test case. I think this one is starting to feel like a recycler. I've also noticed that 

I think we are looking at two issues, one of which is probably actionable, one of which is not.

Comment 7

[Allison Naaktgeboren :ally]

logging suggests the pref state for saved search is as expected but the 0th history item is not getting hidden.

Comment 8

[Allison Naaktgeboren :ally]

I think this probably came in with the original search history suggestion patch back in the bad old days. The recycler logic was probably wrong from the get-go but the original implementation had only one type of suggestion and its number was hardcoded in the load at 3, so it would not have shown up. Then when I added the 2nd type of suggestions they were controlled by one pref, so it was an all or nothing deal.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Allison Naaktgeboren :ally from comment #6)
> Are you on the latest build?

Yes.

> Do you actually turn on search engine suggestions via the doorhanger?

Yes.

Comment 10

[:Margaret Leibovic]

ahunt, can you take a look at this bug? mcomella should be able to help you if you're unclear about how to get started.

Comment 11

[Andrzej Hunt :ahunt]

This only happens for me if I "exit" url editing by pressing the back button twice (step 6) but not if I exit with "X" button on the right of the url bar. It also only happens on the phone I'm working with (nexus 4) but not the tablet (nexus 7).

I've got a preliminary patch which seems to fix things (see next comment), but I'm not fully confident about it yet, so will try to ask mcomella about it.

Comment 12

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella

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

Comment 13

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1233602 - Don't depend on search suggestions to show search history r=liuche

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

Comment 14

[Andrzej Hunt :ahunt]

Comment on attachment 8699259 [details]
MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella

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

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8699259 [details]
MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella

https://reviewboard.mozilla.org/r/28215/#review25921

Spoke in person – this doesn't fix the second STR I listed in comment 0 so I'm clearing review until that's fixed up.

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8699259 [details]
MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella

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

Comment 17

[Andrzej Hunt :ahunt]

Comment on attachment 8699259 [details]
MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella

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

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8699259 [details]
MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella

https://reviewboard.mozilla.org/r/28215/#review25965

Nice.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/66361f4ff785

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66361f4ff785

Comment 21

[Teodora Vermesan (:TeoVermesan)]

(In reply to Michael Comella (:mcomella) from comment #0)
> 1) Hit toolbar
> 2) Search for "sup"
> 3) Click url bar
> 4) Type "s"
> 5) Enable search suggestions & confirm "sup" is shown as a search history item
> 6) Hit back & go to Settings -> Search -> Disable search suggestions
> 7) Click url bar
> 8) Type "s"
 
=> The "sup" appears and the search term

> 9) Type "u" for "su"
> 10) Go back and go to Settings -> Search and disable "Show search history"
> 11) Click the url bar
> 12) Type "s", then type "d"

=> "sup" goes away
 
Verified as fixed using: 
Device: Samsung Galaxy S6 Edge (Android 5.0)
Build: Firefox for Android 46.0a1 (2016-01-06)

I was able to reproduce the issue on 44 Beta 6

Comment 22

[:Margaret Leibovic]

ahunt, can you request uplift for this patch?

Comment 23

[Andrzej Hunt :ahunt]

Comment on attachment 8699259 [details]
MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella

Approval Request Comment
[Feature/regressing bug #]: Bug 1200367 - add a separate search history pref
[User impact if declined]: User is shown incorrect or outdated search suggestions.
[Describe test coverage new/current, TreeHerder]: Manual testing, verified in trunk.
[Risks and why]: Low risk: we enable recycling of search suggestions for all four configurations, rather than just two configurations.
[String/UUID change made/needed]: none

Comment 24

[Andrzej Hunt :ahunt]

Attachment: hiderecycleditems_beta.patch

(Separate patch for beta due to path changes - it is otherwise identical to the original patch)

Approval Request Comment
[Feature/regressing bug #]: Bug 1200367 - add a separate search history pref
[User impact if declined]: User is shown incorrect or outdated search suggestions.
[Describe test coverage new/current, TreeHerder]: Manual testing, verified in trunk.
[Risks and why]: Low risk: we enable recycling of search suggestions for all four configurations, rather than just two configurations.
[String/UUID change made/needed]: none

Comment 25

[Andrzej Hunt :ahunt]

Attachment: hiderecycleditems_beta.patch

Oops, I uploaded an outdated patch for the beta backport - this one is correct.

Approval Request Comment
[Feature/regressing bug #]: Bug 1200367 - add a separate search history pref
[User impact if declined]: User is shown incorrect or outdated search suggestions.
[Describe test coverage new/current, TreeHerder]: Manual testing, verified in trunk.
[Risks and why]: Low risk: we enable recycling of search suggestions for all four configurations, rather than just two configurations.
[String/UUID change made/needed]: none

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8705229 [details] [diff] [review]
hiderecycleditems_beta.patch

This is a key scenario, taking the fix to beta44.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a1b6f09bccf

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d7aeae4b773

Comment 29

[u549602]

Verified as fixed on the latest Beta build(44.0b7),on an Samsung Galaxy S6 Edge, Android 5.1.1

Comment 30

[u549602]

Also, verified as fixed on the latest Aurora build (45.0a2 - 2016-01-07), on an Samsung Galaxy S6 Edge, Android 5.1.1

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2d7aeae4b773