Closed Bug 1359363 Opened 3 years ago Closed 1 year ago

Search engine favicon flicker/blinks/flashes after pressing "Show search suggestions" option in "Search" settings

Categories

(Firefox for Android :: Settings and Preferences, defect, P3)

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
fennec + ---
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox-esr68 70+ verified
firefox53 --- unaffected
firefox54 --- wontfix
firefox55 - wontfix
firefox56 - wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- verified

People

(Reporter: sflorean, Assigned: brad.arant)

References

Details

(Keywords: regression, Whiteboard: [fennec68.2?])

Attachments

(1 file)

Environment: 
Device: Samsung Galaxy Note 4 (Android 5.0.1);
Build: Nightly 55.0a1 (2017-04-23);

Steps to reproduce:
1. Launch Fennec and go to Menu -> Settings -> Search;
2. Turn on/off Show Search suggestions 3-4 times;
3. Observe the search engines.

Expected results:
There is a visible flicker when "show search suggestions" are enabled/disabled.

Actual result:
Enabled/disabled of "show search suggestions" doesn't affect the installed search engines.

Notes:
Video: https://youtu.be/7GNTFZxA2dA

Regression window:
Last good build: 03-02
First bad build: 03-03

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e91de6fb2b3dce9c932428265b0fdb630ea470d7&tochange=9732cd019a8b94c49a275661320c1b742635a3d6
tracking-fennec: --- → ?
tracking-fennec: ? → 55+
Priority: -- → P2
Since this is 55+ for fennec, marking fix-optional for 54.
editing flags due to Bug 1370184
tracking-fennec: 55+ → +
Wesley, I'm a bit hesitant to track this for 55 given this has been filed 2 months ago and there doesn't seem to be much progress yet.  Is this still on track for a fix in early beta55?
Flags: needinfo?(whuang)
I'll track this for 56 for now. The effect sounds minor. If we get a patch we could still uplift to 55.
Very likely the team won't have bandwidth to make it in 55. 
56 is more practical.
Flags: needinfo?(whuang)
Looking at this again it seems like a fairly minor regression and I don't think release management needs to track it - we can leave it to the Fennec team to fix when they have time.
[triage] Bulk edit from title: this is a non-critical issue. Please remove priority if you wish this to be re-triaged.
Priority: P2 → P3
Has Regression Range: --- → yes
Has STR: --- → yes
Summary: Search engines flicker when search suggestions are turn on/off → Search engine favicon flicker/blinks/flashes after pressing "Show search suggestions" option in "Search" settings
Reproducible in the latest version of NIghtly 65.0a1 (2018-10-25) with OnePlus 5T (Android 8.1.0).
Marking fix-optional for 64. We could still take a patch for 65, and if it's verified and doesn't seem risky, could still take fixes for 64 as well.
Happy to take a patch in nightly; if it seems low risk enough please feel free to request uplift to 65 beta.

Removing regression keyword since it has been around for many releases.

Keywords: regression
Assignee: nobody → brad.arant

Easily visible on Samsung Note 8. The issue is a ListView object that is being refreshed upon every single update to the Gecko component to ensure that the list is reflective of the current state in the Gecko module.
I am developing a method to examine the newly retrieved list and only update the changed elements in the ListView similar to the recycler view. In essence emulating a ViewHolder so that upon re-display the ImageView will not need to re-fetch the image resource and thereby reduce or eliminate the visual artifacts caused by a completely reloaded list (flickering).

There is a call to dispatch the list of search engines when this checkbox is clicked, as follows:

    } else if (PREFS_SEARCH_SUGGESTIONS_ENABLED.equals(prefName)) {
        // Tell Gecko to transmit the current search engine data again, so
        // BrowserSearch is notified immediately about the new enabled state.
        EventDispatcher.getInstance().dispatch("SearchEngines:GetVisible", null);
    }

This is causing the ListView to reload in the object hierarchy. I commented out the EventDispatcher so that it does dispatch the message and the flickering goes away and the search bar works the same as if I left this in there. Given that the preferences are being update and the BrowserSearch reads the preferences is this dispatch absolutely necessary? Comment indicates that the dispatch is needed to update the BrowserSearch but works even without it.

Is this dispatch absolutely necessary?

I will generate a test apk and submit to the testing group to see if there is any effects not handled by removing this else if group and it's contents.

Is this dispatch absolutely necessary?

Yes, it is, otherwise I wouldn't have added it.
Outside of the special case of the post-installation-prompt to enable or disable search suggestions, BrowserSearch takes the current state of search suggestions (enabled/disabled) only from the search engine data transmitted by Gecko.
I.e. the full cycle goes

  1. The preferences screen writes the settings value into Gecko preferences.
  2. Eventually, some browser.js-based search engine code reads that value and includes it in an update message sent to the Android app front-end, which also includes the complete current search engine data.
  3. BrowserSearch receives that message and among other things, also updates its "suggestions enabled" state from that message.

Because the intention was for bug 1342718 to be fixed as quickly as possible and to be uplifted as well, the simplest solution was to add a step "1a. Re-request a search engine data update from browser.js" instead of adding some additional code that would allow turning suggestions on/off directly on the Android app side without a full search engine data update.

When the dispatch response is received I added logic to include the DiffUtil array comparison functionality to apply only the changes to the list preventing the reloading of the detail items thereby eliminating the flicker.

Appears to work smoothly. Ready for review and testing.

Even if old, this is still a regression, so adding back the keyword for consistency.

Keywords: regression
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62e1b5ea9cab
Apply diffutil to search list updates for smoother ui experience.;r=petru

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

I'm thinking we can just leave this fix for GeckoView-based apps and wontfix for Fennec. Sound OK to you, Chris?

Flags: needinfo?(cpeterson)

Both the bug and the patch are Fennec-only.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

I'm thinking we can just leave this fix for GeckoView-based apps and wontfix for Fennec. Sound OK to you, Chris?

Since this bug is about showing the list of search suggestions, I think this bug only affects Fennec, not GeckoView apps. If we don't uplift this fix to ESR 68, then it will never reach Fennec users.

@ Ryan, it would be nice to uplift the fix so we don't lose Brad's work, but OTOH the bug doesn't sound super bad. (It is a P3, after all.) Can we take this fix in ESR 68.2a1 after we ship ESR 68.1 (September 3). That would give us plenty of beta bake time.

@ Brad, is there a specific list of Fennec bugs you're working through? I'll follow up with Ashley Thomas (the new Product Manager for Fennec) to make sure we prioritize the most important bugs.

Flags: needinfo?(ryanvm)
Flags: needinfo?(cpeterson)
Flags: needinfo?(brad.arant)
Whiteboard: [fennec68.2?]

Yeah, let's punt on this until 68.2 at this point.

Flags: needinfo?(ryanvm)

In response to Comment 23.

I am working from the list provided to me to find actionable items. If you have priorities then I would be glad to work whatever list you can provide.

I am currently educating myself on the video functions required in Bug 1548116.

My List:
https://bugzilla.mozilla.org/buglist.cgi?o5=anyexact&j2=OR&o1=substring&n1=1&v5=affected&o4=anyexact&v1=geckoview&v4=affected&f1=status_whiteboard&o3=anyexact&v3=affected&f4=cf_status_firefox67&query_format=advanced&f3=cf_status_firefox66&f2=OP&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&f5=cf_status_firefox68&component=Activity%20Stream&component=Add-on%20Manager&component=Android%20partner%20distribution&component=Android%20Sync&component=Audio%2FVideo&component=Awesomescreen&component=Build%20Config%20%26%20IDE%20Support&component=Custom%20Tabs&component=Data%20Providers&component=Download%20Manager&component=Favicon%20Handling&component=Firefox%20Accounts&component=First%20Run&component=General&component=JimDB&component=Keyboards%20and%20IME&component=Locale%20switching%20and%20selection&component=Logins%2C%20Passwords%20and%20Form%20Fill&component=Metrics&component=Overlays&component=Profile%20Handling&component=Reader%20View&component=Screencasting&component=Session%20Restore&component=Settings%20and%20Preferences&component=Testing&component=Text%20Selection&component=Theme%20and%20Visual%20Design&component=Toolbar&component=Web%20Apps&product=Firefox%20for%20Android&list_id=14857939

Flags: needinfo?(brad.arant)

Hi, verified as fixed with Firefox Nightly 71.0a1 (2019-09-03) with devices:

  • Google Pixel 3 XL (Android 9)
  • Samsung Galaxy Note9 (Android 8.1.0)
  • Sony Xperia Z5 (Android 7.0)

Thanks for verifying, Diana!

Brad, now that Diana has verified your fix and the Fennec ESR 68.2 Beta cycle has begun, you can request uplift to the mozilla-esr68 repo for the Fennec ESR 68.2 (2019-10-22 release date).

Flags: needinfo?(brad.arant)

[Tracking Requested - why for this release]:

Flags: needinfo?(brad.arant)

Comment on attachment 9077405 [details]
Bug 1359363 - Apply diffutil to search list updates for smoother ui experience.;r?VladBaicu

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Resolves annoying flicker in user experience.
  • User impact if declined: UI issue - search icon flicker.
  • Fix Landed on Version: 71.0a1
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Very local to small component and verified by QA team.
  • String or UUID changes made by this patch:
Attachment #9077405 - Flags: approval-mozilla-esr68?

Comment on attachment 9077405 [details]
Bug 1359363 - Apply diffutil to search list updates for smoother ui experience.;r?VladBaicu

Fixes a Fennec flicker bug. Approved for Fennec 68.2b4.

Attachment #9077405 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

I tested the issue on Beta 68.2b4 using a Samsung Galaxy S8+ (Android 8.0.0) and the issue no longer occurs, I will mark it accordingly.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.