Fenix: [Bug] Adding/removing top sites (frecent too) causes a flash

[jonalmeida]

Steps to reproduce

1. Add/remove a top site.
2. Observe the top sites list.
3. Enable frecent top sites.
4. Visit a new site.
5. Go back to the home screen.
6. Observe the top sites list.

Expected behavior

• At 2, a top site item should animate in or out (depending on add/remove).
• At 5, the new frecent top site items should animate in.

Actual behavior

• At 2, all the top sites disappear and reappear with the new change.
• At 2, all the top sites disappear and reappear with the new change.

Device information

• Android device: Android 9, Pixel 4
• Fenix version: Nightly 25/08/2020 - this happens before the new top sites feature landed, but is more prominently visible with frecent top sites turned on.

Removing a pinned top site:

Adding a frecent top site:

[jonalmeida]

Updating the ticket from my last investigation: when adding break points in the DiffUtils calls.

When you start with three top sites, remove one, then remove a second, the first one that was removed shows up before being removed with the second.

Somewhere (I haven't figured that out yet), there is an old cache which is not coming from the storage layer.

[jonalmeida]

I think I have an idea what the bug is. We have two adapters that have caused confusion: TopSitesAdapter and TopSitesPagerAdapter - the latter is the inner adapter that holds the actual top sites but doesn't using DiffUtils to distinguish which top sites to update, so therefore the call the notifyDataSetChanged in it will update the entire list of top sites.

[jonalmeida]

Adding an adapter diff check with DiffUtils seems to solve a different bug. We're still seeing the flash happening.

I'm going to try next to see if I can modify the DiffUtils check to be more specific with which top site screen.

Ideally, we reduce the nested RecyclerViews and write our own custom LayoutManager to draw a the layout as we need it, but this is a much larger change.

[jonalmeida]

The fix for this issue is that in the comparator for top sites in the SessionControlAdapter tells us to always refresh all the top sites instead of updating the individual items. This is the main adapter that propagates home screen state changes.

We also need to add a diff utils comparator for the inner top sites adapter as well since that was missing too.

[jonalmeida]

Since we've enabled frecent top sites in beta (yes?), adding the uplift tag if there is a new RC built.

[liuche]

@jonalmeida @gabrielluong when this has gone through QA and has baked for a few days, please open a PR against v81.1.1 branch. That way it can go out with whatever dot release is happening.

[jonalmeida]

We just landed the fix, let's see how it turns out! 😄

[ebalazs-sv]

This issue is still reproducible on Nightly 200921 06:05 (Build #2015765171) GV 82.0a1 from 9/21 with Motorola Moto G6 (Android 8), Google Pixel 2 (Android 9), Nexus 5 (Android 6.0.1). With the "Show most visited sites" setting enabled, this issue is less visible now:

cc @jonalmeida I will remove the qa:needed label for now.

[jonalmeida]

@ebalazs-sv I've filed https://github.com/mozilla-mobile/fenix/issues/15499 for this after investigating the issue with no success to why this happens. I'm still looking into it but we can close out this issue since the fix works better with frecent top sites (that are enabled by default so are more noticeable).