Pref/Search Engine Suggestions UI can get out of sync

VERIFIED FIXED in Firefox 44

Status

()

Firefox for Android
General
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: liuche, Assigned: psd, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 45
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 verified, firefox45 verified, fennec44+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Created attachment 8664495 [details]
Screenshot: Search Suggestions + Prompt

We don't seem to refresh UI for search suggestions when we change the pref, so we'll get inconsistent UI.

STR:
1. New fennec where opt-in search suggestions UI hasn't been dismissed. (Search suggestions off by default.)
2. Make some searches.
3. Enable search suggestions through settings UI.
4. Go back to awesomescreen and type searches

Expected: Search suggestions, no opt-in prompt
Actual: Search suggestions, as well as opt-in prompt

Updated

2 years ago
tracking-fennec: --- → ?

Updated

2 years ago
Assignee: nobody → ally
Might be a dupe of Bug 1206639, Bug 1206628. Sync up with mcomella.
Status: NEW → ASSIGNED
tracking-fennec: ? → 44+
Summary: Pref/Search Suggestions UI can get out of sync → Pref/Search Engine Suggestions UI can get out of sync
After a quick read through Bug 1206639, Bug 1206628, I think it might be a dupe. What do you think?
Flags: needinfo?(michael.l.comella)
Like bug 1206639 & bug 1206628, I think is a pre-existing bug uncovered by bug 1186683, which no longer creates BrowserSearch each time it is shown, meaning we never refreshed the opt-in prompt's visibility correctly (it should probably be done in onStart, or one of those methods). Prabhjyot was interested in working on the regressions so I'll assign here too.

Prabhjyot, since you're also assigned to the other regressions, let me know if this is too much and I'll try to find someone else to help out! There is about a month and a half to the next merge though so we have time.
Assignee: ally → prabhjyotsingh95
Blocks: 1186683
Flags: needinfo?(michael.l.comella)
Prabhjyot, we know that the Search/Awesomescreen code is unintuitive, so please let me know if you have any questions or want feedback.

I look forward to your patch :)
Mentor: ally@mozilla.com
(Assignee)

Comment 5

2 years ago
Created attachment 8672357 [details] [diff] [review]
1207340.patch

This works, as in, it removes the prompt-in when I search for something after enabling suggestions. However, I have noticed that sometimes there is a delay in the removal of the ViewStub, this could be a problem in slower devices. Any ideas that could help?
Flags: needinfo?(ally)
Attachment #8672357 - Flags: review?(ally)
Attachment #8672357 - Flags: feedback?(ally)
(Assignee)

Updated

2 years ago
Attachment #8672357 - Flags: review?(ally)
(Assignee)

Updated

2 years ago
Depends on: 1206628
(Assignee)

Comment 6

2 years ago
The uploaded patch must be applied on top of the patch uploaded on bug 1206628.
by the by, you need only set either NI or feedback? or review? to get someone's attention on bugzilla. :)
Flags: needinfo?(ally)
Comment on attachment 8672357 [details] [diff] [review]
1207340.patch

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

My chief concern here is that we might be papering over the problem. What other approaches have you tried?

It's also not clear to me how this patch functionally relies on Bug 1206628 to work. Or did you mean simply that this patch bitrots if not applied on top of that patch?

::: mobile/android/base/home/BrowserSearch.java
@@ +649,5 @@
>              updateSearchEngineBar();
>  
>              // Show suggestions opt-in prompt only if suggestions are not enabled yet,
>              // user hasn't been prompted and we're not on a private browsing tab.
> +            // Else remove the opt-in prompt if it has been inflated in the view

nit: you don't need the "Else" in the comment. The rest of the sentence is sufficient. You might want to explain in a sentence why we're removing the prompt if it has already been inflated though.
Attachment #8672357 - Flags: feedback?(ally)
(Assignee)

Updated

2 years ago
Attachment #8672357 - Flags: review?(michael.l.comella)
Comment on attachment 8672357 [details] [diff] [review]
1207340.patch

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

This works, but I see the prompt flicker on before it disappears. Can we do better?

Perhaps this check should occur in onResume? It feels strange that we change suggestions opt-in visibility in setSearchEngines – should we move that too?

::: mobile/android/base/home/BrowserSearch.java
@@ +649,5 @@
>              updateSearchEngineBar();
>  
>              // Show suggestions opt-in prompt only if suggestions are not enabled yet,
>              // user hasn't been prompted and we're not on a private browsing tab.
> +            // Else remove the opt-in prompt if it has been inflated in the view

I agree w/ Ally – can you explain why we need to remove the prompt?

@@ +654,4 @@
>              if (!mSuggestionsEnabled && !suggestionsPrompted && mSuggestClient != null) {
>                  showSuggestionsOptIn();
>              }
> +            else {

nit: } else {
Attachment #8672357 - Flags: review?(michael.l.comella) → feedback+
(Assignee)

Comment 10

2 years ago
Hey mcomella!
When I tried to check if we want to remove the prompt in onHiddenChanged, I realised that the browser fragment knows if the search has been enabled only when it receives and parses the JSON from gecko([1]) and hence we have to show prompt only after setting the search engines.

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#574
re comment 10, we spoke on irc:

<mcomella> I think if we call for the update [of the search engines] when we change the preference, it would be good and the preload [bug 927692] will take care of the case for first run – does that make sense?
hey psd, how's this going?
Flags: needinfo?(prabhjyotsingh95)
(Assignee)

Comment 13

2 years ago
Created attachment 8683320 [details] [diff] [review]
1207340-4.patch

Hey!
This is what I had in mind and tried to do: add a new listener to the checkboxpreference (which changes suggestions enabled) in GeckoPreferences.java which then calls, asks for an update from Gecko, i.e: SearchEngines:GetVisible, which updates and data and removes the prompt if necessary.

I am stuck at a runtime error (Not able to addPreferences before super.onCreate), while trying to add the listener in GeckoPreferences.java, but more than that, I have some doubts about this algorithm, to solve this.
Flags: needinfo?(prabhjyotsingh95)
(Assignee)

Comment 14

2 years ago
So,
After discussion on IRC, we have concluded that removing the flicker by adding in listeners or trying some other solutions seem overly complicated and not worth it.
I'll upload a patch (which flickers :( ) but fixes this and bug 1216826.
(Assignee)

Comment 15

2 years ago
Created attachment 8684333 [details] [diff] [review]
1207340-5.patch
Attachment #8672357 - Attachment is obsolete: true
Attachment #8683320 - Attachment is obsolete: true
Attachment #8684333 - Flags: review?(michael.l.comella)
Comment on attachment 8684333 [details] [diff] [review]
1207340-5.patch

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

Looks good to me! Thanks Prabhjyot!

Make sure you update the commit message from the try text.
Attachment #8684333 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 19

2 years ago
ah! Realized my mistake! working on correcting it!
Keywords: checkin-needed
Hi, this failed to apply:

patching file mobile/android/base/home/BrowserSearch.java
Hunk #2 FAILED at 666
1 out of 3 hunks FAILED -- saving rejects to file mobile/android/base/home/BrowserSearch.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 1207340-5.patch
Flags: needinfo?(prabhjyotsingh95)
Keywords: checkin-needed
(Assignee)

Comment 23

2 years ago
Created attachment 8687123 [details] [diff] [review]
1207340-6.patch

rebased!
Attachment #8684333 - Attachment is obsolete: true
Flags: needinfo?(prabhjyotsingh95)
Prabhjyot, can you flag this for uplift to Aurora?
Flags: needinfo?(prabhjyotsingh95)
(Assignee)

Comment 27

2 years ago
Comment on attachment 8687123 [details] [diff] [review]
1207340-6.patch

Approval Request Comment
[Feature/regressing bug #]:bug 1186683
[User impact if declined]: User shown prompt even after switching on suggestions
[Describe test coverage new/current, TreeHerder]: Tested manually, visual testing required
[Risks and why]: Low risk - it is a simple change, removing the suggestions prompt when it should not be present
[String/UUID change made/needed]:None
Flags: needinfo?(prabhjyotsingh95)
Attachment #8687123 - Flags: approval-mozilla-aurora?

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d70d6466d7eb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Chenxia, could you please verify this issue is fixed as expected on the latest Nightly build? Thanks.
Flags: needinfo?(liuche)
Comment on attachment 8687123 [details] [diff] [review]
1207340-6.patch

This has been on Nightly for a few days now, seems safe to uplift to Aurora44.
Attachment #8687123 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ritu, this works for me on the most recent Nightly. I also see the flash that mcomella mentioned in comment #9, but it sounds like we are okay with that.
Flags: needinfo?(liuche)

Comment 32

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ac88e18ff69
status-firefox44: affected → fixed
With search suggestions disabled, make a search. Search Suggestion opt-in prompt is displayed: "Would you like to turn on search suggestions?". Enable search suggestions and make a new search.  The Search Suggestion opt-in prompt is not displayed anymore:
Verified as fixes using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 45.0a1 (2015-11-22)
status-firefox45: fixed → verified
With search suggestions disabled, make a search. Search Suggestion opt-in prompt is displayed: "Would you like to turn on search suggestions?". Enable search suggestions from settings and make a new search.  The Search Suggestion opt-in prompt is not displayed anymore:
Verified as fixes using:
Device: Nexus 6 (Android 5.1.1)
Build: Firefox for Android 44.0a2 (2015-11-25)
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
You need to log in before you can comment on or make changes to this bug.