Closed Bug 1223983 Opened 4 years ago Closed 4 years ago

Disabling search suggestion settings leaves suggestion on screen

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox44 --- verified
firefox45 --- verified
firefox46 --- verified
b2g-v2.5 --- fixed
fennec 44+ ---

People

(Reporter: mcomella, Assigned: ahunt)

References

Details

Attachments

(2 files, 2 obsolete files)

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?
Flags: needinfo?(ally)
tracking-fennec: --- → ?
(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)
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.
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.
Assignee: nobody → ally
tracking-fennec: ? → 44+
Depends on: 1200367
Flags: needinfo?(michael.l.comella)
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.
Flags: needinfo?(michael.l.comella)
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.
Flags: needinfo?(ally)
logging suggests the pref state for saved search is as expected but the 0th history item is not getting hidden.
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.
(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.
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.
Assignee: a.m.naaktgeboren → ahunt
Flags: needinfo?(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.
Flags: needinfo?(ahunt)
Attachment #8699259 - Flags: review?(michael.l.comella)
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/
Attachment #8699259 - Attachment description: MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. → MozReview Request: Bug 1223983 - Hide recycled search items when suggestions/history are disabled too. r=mcomella
Attachment #8699781 - Attachment is obsolete: true
Attachment #8699781 - Flags: review?(liuche)
Blocks: 1232651
Blocks: 1234534
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.
Attachment #8699259 - Flags: review?(michael.l.comella)
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/
Attachment #8699259 - Flags: review?(michael.l.comella)
Blocks: 1235456
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/
Attachment #8699259 - Flags: review?(michael.l.comella) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/66361f4ff785
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(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
ahunt, can you request uplift for this patch?
Flags: needinfo?(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
Flags: needinfo?(ahunt)
Attachment #8699259 - Flags: approval-mozilla-aurora?
Attached patch hiderecycleditems_beta.patch (obsolete) — Splinter Review
(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
Attachment #8705227 - Flags: approval-mozilla-beta?
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
Attachment #8705227 - Attachment is obsolete: true
Attachment #8705227 - Flags: approval-mozilla-beta?
Attachment #8705229 - Flags: approval-mozilla-beta?
Comment on attachment 8705229 [details] [diff] [review]
hiderecycleditems_beta.patch

This is a key scenario, taking the fix to beta44.
Attachment #8705229 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8699259 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on the latest Beta build(44.0b7),on an Samsung Galaxy S6 Edge, Android 5.1.1
Also, verified as fixed on the latest Aurora build (45.0a2 - 2016-01-07), on an Samsung Galaxy S6 Edge, Android 5.1.1
You need to log in before you can comment on or make changes to this bug.