Closed Bug 1270068 (jingle-lights) Opened 5 years ago Closed 5 years ago

Top sites tiles have improper graphic behavior when the order changes

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox46 unaffected, firefox47 unaffected, firefox48 unaffected, firefox49 verified, fennec49+, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- verified
fennec 49+ ---
firefox50 --- verified

People

(Reporter: u549602, Assigned: sebastian)

References

Details

Attachments

(1 file)

Environment: Nightly
Device: Samsung Galaxy S7 Edge (Android 6.0.1);
Build: Nightly 49.0a1 (2016-05-03);

Steps to reproduce:
1. Open Nightly and Sync 
2. Go to Top sites panel and open a link in order to change top visited order


Actual result:
Improper graphic reordering displayed 

For further details please check:  https://www.youtube.com/watch?v=9_0KdhRz2xw&feature=youtu.be
LOL.

We need to make sure that this does not happen with the new top sites version (bug 1052933).
Blocks: 1052933
Also CC Grisha: From the video it looks like it's not necessarily the coloring that is broken but we are actually reordering the top sites multiple times. Do you have an idea what's causing this?
Component: Graphics, Panning and Zooming → Awesomescreen
You can try this with 3/4 sites that were visited and observe the ordering behavior, indeed the top sites are re-ordered several times before reaching the correct history/access based positions. This after opening one of them in the background
Alias: jingle-lights
tracking-fennec: --- → ?
Flags: needinfo?(gkruglov)
In today's nightly, I see ordering change just once - after the page finishes loading in the background, but thumbnails/favicons blink multiple times, switching from a generic looking one (M for mail.), to the favicon one. Sometimes they end up being stuck in a wrong state (e.g. show proper favicon momentarily, but final result is the generic icon).

From what I can tell, icons blink twice after tab finishes loading. I see the same behaviour on the History panel, if you long-press and open history items in a new tab.

When pages finish loading, we update local meta about them - specifically page title and then the favicon. And in the process we bump the "last modified" timestamp. Perhaps that's causing some issues with rendering, or something else related to updating history meta? I'll take a closer look into it.
Assignee: nobody → gkruglov
tracking-fennec: ? → 49+
I saw the tiles/favicons fighting and sometimes even the wrong tiles being re-used for other sites. I don't think it is worth fixing the tile problem while we are rewriting the panel (bug 1052933) - assuming the database part is okay (comment 4) - as long this only affects 49.
I've noticed flickering issues even when opening the home panels. It seems like we're re-querying or re-drawing or something. 

Grisha, we need to address these issues.
Spent some time looking into this issue. To narrow down potential causes, I've changed underlying top sites query to return a static, unchanging set of history items in a constant order.
jingle-lights behaviour as described below still happened!

Looking a bit closer, even with the unchanging data behind it, TopSitesLoader's loadCursor is called multiple times, and TopSitesGridAdapter's bindView is being called a few times for each item in the grid, in order - and a lot of the time TopSitesGridItemView that is passed in and cursor's position are "misaligned" - which results in view's updateState doing a bunch of work - such as generating fancy default icons on a background thread, posting them back to UI thread, etc.

It's almost always "one off" - e.g. bindView is called with a view at position 2, and cursor will be pointing to data for position 3 - so we regenerate the view resulting in a visual "blink". Sometimes we end up with correct icons/thumbnails, and sometimes not.

http://pastebin.com/Cqkzs210 contains some debug logging I've put in place, which should help deciphering the above. Logs are showing what happens when an item from the top sites grid is opened in a new tab.

Other notes: switching orientation enough times will end up with entirely blank top-sites grid tiles - sometimes they get fixed with more orientation switches, and sometimes not. When that happens, if a thumbnail was present for one of the grid items, it pretty much always gets displayed for a few other grid items as well.

Sebastian, do you know what could be happening here?
Flags: needinfo?(gkruglov) → needinfo?(s.kaspari)
Flags: needinfo?(gkruglov)
(In reply to :Grisha Kruglov from comment #7)
> Sebastian, do you know what could be happening here?

I'll debug it a bit. I can't remember seeing this before, so I wonder if this is triggered by the changes for the new default favicons or if they make the issue just more visible.
Assignee: gkruglov → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
A big problem here is that the value of the _id column is always 0. And that's what the default implementation of CursorAdapter will return from getItemId(). At the same time CursorAdapter returns true from hasStableIds() (Normally that should be the case for most database cursors). This leads to the ListView thinking it can recycle views with matching ids and as every item has the id 0 it's pretty random which existing view ends up where after a requery.

This seems to be fixed after writing a custom implementation of getItemId(). On the positive side this might even improve the performance of updates because we now correctly recycle views.
Comment on attachment 8760722 [details]
Bug 1270068 - TopSitesGridAdapter: Implement getItemId() so that Android can recycle views appropriately.

https://reviewboard.mozilla.org/r/58206/#review55114

Nice, thanks for tracking it down!

::: mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java:595
(Diff revision 1)
> +            // We are trying to return stable ids so that Android can recycle views appropriately:
> +            // * If we have a history id then we return it
> +            // * If we only have a bookmark id then we negate it and return it. We negate it in order
> +            //   to avoid clashing/conflicting with history ids.
> +
> +            Cursor cursor = getCursor();

nit: final

::: mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java:598
(Diff revision 1)
> +            //   to avoid clashing/conflicting with history ids.
> +
> +            Cursor cursor = getCursor();
> +            cursor.moveToPosition(position);
> +
> +            final long historyId = cursor.getLong(cursor.getColumnIndex(TopSites.HISTORY_ID));

nit: getColumnIndexOrThrow

::: mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java:603
(Diff revision 1)
> +            final long historyId = cursor.getLong(cursor.getColumnIndex(TopSites.HISTORY_ID));
> +            if (historyId != 0) {
> +                return historyId;
> +            }
> +
> +            final long bookmarkId = cursor.getLong(cursor.getColumnIndex(TopSites.BOOKMARK_ID));

nit: getColumnIndexOrThrow
Attachment #8760722 - Flags: review?(gkruglov) → review+
https://hg.mozilla.org/integration/fx-team/rev/3cf6f6e8022b9759ed2d914ad16f5a754de00366
Bug 1270068 - TopSitesGridAdapter: Implement getItemId() so that Android can recycle views appropriately. r=grisha
Flags: needinfo?(gkruglov)
Comment on attachment 8760722 [details]
Bug 1270068 - TopSitesGridAdapter: Implement getItemId() so that Android can recycle views appropriately.

Approval Request Comment

[Feature/regressing bug #]: We introduced new, generated default favicons in bug 1228680 (49.0).

[User impact if declined]: This can best be seen in the video linked in comment 0. Jingle lights!

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: Risk seems to be low. In the worst case I can see us recycling views wrong but that's what we have been doing before this patch all the time.

[String/UUID change made/needed]: -
Attachment #8760722 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3cf6f6e8022b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Verified as fixed on the latest Nightly build 50.0a1 (2016-06-14).
This was tested on a Xiaomi mi i4(Android 5.0.2) and on a Samsung Galaxy S6 Edge(Android 5.1.1)
Comment on attachment 8760722 [details]
Bug 1270068 - TopSitesGridAdapter: Implement getItemId() so that Android can recycle views appropriately.

Fix for regression in 49, ok to uplift to aurora.
Attachment #8760722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on latest Aurora 50(2016-09-14) with Galaxy Note 5(6.0.1).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.