1348020 - [RTL] Tabs trays that show a grid of tabs scroll on their own each time a tab changes or is closed or added

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Itiel]

STR:
1. Install latest Nightly, Hebrew locale
2. Enable compact tabs from settings
3. Open 14+ New tabs (so you'd be able to scroll in the tabs tray)
4. In the first or second tab open any webpage (preferably a heavy one that takes time to load, such as www.walla.co.il), and before it finished loading quickly open the tabs tray
5. Wait for the webpage to load

AR:
The moment the tab finishes loading the tab list is scrolling on it's own a bit, 

ER:
It shouldn't.

I couldn't reproduce it without enabling the Compact tabs feature, and with Firefox being in LTR locale.

Comment 1

[Tom Klein]

This happens in RTL mode with any tabs tray that shows tabs in a grid (so tablet, phone in landscape mode, or phone in portrait mode with "Compact tabs" on), and looks like it's going to be pretty annoying.  I'm seeing the scroll (on emulators) even with sites like twitter, which loads relatively quickly, but the scroll happens when the thumbnail gets updated, which takes a little longer.

I'm able to reproduce on a minimal app with a very basic RecyclerView using a GridLayoutManager and enough items to require scrolling: everytime I call notifyItemChanged on the adapter, even if the position I'm notifying about isn't in view, the RecyclerView scrolls.  So this looks to be an Android bug (which is surprising since this seems like such a basic operation...that makes me nervous, but on the other hand my sample app can't get much more basic - there are no calls to scroll anything anywhere, for starters, and the scroll doesn't happen in LTR mode in spite of their being 0 references to RTL in the app).  I'll work on submitting a bug/searching for existing bugs.

In the meantime: everytime a tab changes (selected, unselected, thumbnail, title, audio playing) we're going to get a scroll we don't want.  If you close a tab you get a selected and an unselected, so that's two scroll jumps (I think) which I'm seeing move the next selected tab all the way off screen on a tablet :(

It wouldn't be nice, but we might have to just do item view updates by hand in RTL mode if we want to avoid this.

Comment 2

[Tom Klein]

I couldn't find any matching bugs, so I filed a new one:
https://code.google.com/p/android/issues/detail?id=339632

Comment 3

[Tom Klein]

I fixed (i.e. worked around) the issue with notifyItemChanged, and that worked, but then found that we were still doing extra scrolling on a tab close and a tab undo-close.  I was able to reproduce both using the simple app from the comment 2 bug report.

There's been no news on the android bug report, but I tested the simple app using API 24 and 25 and found that all three issues seem to be fixed there.  My understanding is that we're going to skip 24 and 25 and (hopefully? :-) jump straight to 26?  But that's probably several releases away.

As I mentioned I have a work around for notifyItemChanged, so we could land just that and fix some of the issues, but I don't know how to work around calling notifyItemInserted and notifyItemRemoved, so it seems our options are to:
a) live with it until we move to 26;
b) try to patch RecyclerView, or whichever of its helper components is failing, and use that until we move to 26. Of course depending on how complex the patch is and how much changed in between our version and the version that was patched, this could carry its own risks.

My feeling is that this is annoying enough (see more STR below) that we should try for b), unless my timeline on moving to 26 is off. Thoughts?

I didn't find any bugs that fit with the notifyItemChanged issue I originally reported, but I can look again - maybe it got reported under notifyItemInserted or notifyItemRemoved.  I guess I could also just walk back through the RecyclerView (and friends) change history looking for RTL-related patches.

[By the way, since I submitted the bug in comment 2, apparently things have changed and you can't even view a bug report without logging in :/]

STR for the close and add issues:
1) Set the android language to an RTL language (or use an RTL locale build I guess).
2) Use a tabs tray that shows tabs in a grid: tablet, phone in landscape, or phone in portrait with "compact tabs" on.
3) Add enough tabs to require scrolling in the tray.
4) Close tab 1 (or any tab for which there are still tabs offscreen to scroll below it), and then undo the close. [Tab 0 scroll behavior is different, don't try that.]

ER: All but the first tab moves, but the tabs tray doesn't scroll at all.
AR: when you close the tab the whole tabs tray scrolls down, and when you undo the close it scrolls down some more.  In the compact tabs case the new selected tab is already scrolled off screen after the close.

Comment 4

[Tom Klein]

I've searched the issue database and github repo, and compared "rtl" (the string) references in the android-7.0.0_r33 versions of RecyclerView, GridLayoutManager, and LinearLayoutManager to our versions and didn't find anything obvious. I just last night asked on the bug I reported if they know which bug or commit fixed this (oh yeah: the bug was confirmed in 23.4.0 and is now marked obsolete since the issue is fixed in the current support library version).

https://github.com/android/platform_frameworks_support/commit/95017f70973e4cbc9b7be1142e2cf887af4f4a48 seemed like a possibility, but the issue here is fixed in 24 and, unless I'm misunderstanding the meaning of the tags on that commit, that commit isn't applied in any 7.0.* versions.

In trying to apply that patch anyway (just because) I discovered that it probably won't be so simple as patching a single RecyclerView-related file even if I do find the right patch: for example, in this case, if I use a patched version of GridLayoutManager (in org.mozilla.gecko.tabs), then I also need a patched version of LinearLayoutManager since GridLayoutManager uses package-private members of its base LLM, and then LLM uses package-private members of RV, which uses package-private members of several other classes, and so on.  So as of now it looks like we'd wind up needing to replicate most of a newer version of the android.support.v7.widget package in order to fix this (unless we get lucky and find the fix and it's in a public method that only refers to public members, which seems unlikely to me for this bug).  I suppose we could try that (though it's also possible that the newer widget classes may use other parts of the more recent API that we don't have in 23, or even change our current RV behavior in some way...).

The other option which I hadn't thought of earlier is to call notifyDataSetChanged() in the adapter when we add or remove an item (instead of using the more specific notifyItemInserted and notifyItemRemoved). That doesn't suffer from the scrolling bug, but it does mean we lose the remove and add animations: instead the new tabs just blink into their new positions without any movement on screen. That's what used to happen (I think) on a close undo prior to RV, so I guess we wouldn't be losing much there (though it would be different from current LTR), but we've always done a remove animation, so we'd probably want to try to restore that. I hacked in the pre-RV version [1] where we did it by hand, and it doesn't seem that far off (ironically it animates like it might just be RTL broken :) but that's fairly involved code I'm not eager to bring back, and it will take more testing before it's clear if it will really mesh with the way RV does things.  Combined with the workaround for notifyItemChanged, it's going to be pretty hacky, but I guess the hacks would (fingers crossed) only be around for a few releases.

Max (and/or Sebastian), before I spend more time on this: do you have any thoughts on a) priority in terms of getting this fixed before we switch to API 26 / b) any preference between the two proposed possible solutions / c) can you see any other solution I'm missing?

[1] https://hg.mozilla.org/mozilla-central/file/28cab19aa7f1/mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java#l361

Comment 5

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.