Wordpress-android: IA Reader: tabs bar not retaining scroll position under certain circumstances

[develric]

Expected behavior

Tablayout should retain the scroll position when user switch away from the reader screen and comes back.

Actual behavior

The scroll position is not retained and the selected tab risk to go out of current visualization.

Steps to reproduce the behavior

1. Select an item off the right side of the tab bar. It becomes centred, as you’d expect:

1. Switch to the Notifications section of the app and then back to Reader.
2. Notice that the tab is now off the right side of the screen again; the bar has scrolled back to its default position:

Tested on [Pixel 3 emu], Android [API 28], WPAndroid [alpha-200]

[renanferrari]

The most reasonable hypothesis for why this behavior occurs I've found so far is that it seems to happen when a tab is selected programmatically before the TabLayout have been measured and drawn.

Looking into the Reader code I found that the tabs themselves aren't directly selected anywhere, but since the TabLayout is setup using the setupWithViewPager method, I also looked for times when a _page_ is selected programmatically, which led me to this line, inside the setCurrentFilter method:

https://github.com/wordpress-mobile/WordPress-Android/blob/30d15a881b4951d2b6a11345384f98fe51a50d00/WordPress/src/main/java/org/wordpress/android/ui/FilteredRecyclerView.java#L106

---

My first approach to further validate this was to see if I could identify at which point in the ReaderPostListFragment lifecycle the setCurrentFilter method was being called so we could then try to move it to some point later, in order to prevent it from being called before the TabLayout was ready.

I believe that would have been the most elegant solution here, but unfortunately I was not able to go too far with it. I did manage to find that most of the calls to setCurrentFilter can be traced back directly or indirectly to code at ReaderPostListFragment.onCreateView, but there are many async callbacks and conditions along the way that made it hard for me to debug this more precisely, specially since I’m not very familiar with the code base.

---

Since that didn't work out so well, my second approach was to look into more "brute force" solutions. Taking a look at related StackOverflow questions (1|2), I saw there were two commonly suggested alternatives:

• Delaying the tab selection using Handler.postDelayed().
• Using a ViewTreeObserver to make sure the tab is selected only after the TabLayout is ready.

These are hackish solutions, but from what I've tested, they both kinda work.

However, it's worth noting that they don't really solve the source of the issue, which is answering "why isn't the tab bar retaining its scroll position in the first place". Instead, they just answer "why the tab bar isn't correctly scrolling to its selected position". This distinction is important because it can result in a small, but noticeable visual discrepancy (showing the selected tab as soon as the screen is shown vs scrolling to the selected tab after showing the screen).

---

For now, here's what I have in mind for the next steps:

1. We pick one of those naive fixes and make it work ASAP.
2. I'll keep exploring other possible causes and solutions.
   
   
   
   • I'm not 100% convinced that the hypothesis I've worked with so far is really the problem here. For instance, there's a related open issue on the AndroidX tracker which I think it's worth looking into.
   
   
   • I also want to listen to suggestions from people more familiar with the codebase and maybe revisit that first approach with them.
   
   
3. Later on, coming up with a more solid plan on what actually needs to be reworked.

[develric]

Thanks for the thorough research on this @renanferrari ! 🙇‍♂️

tab is selected programmatically before the TabLayout have been measured and drawn

I also agree with this. As discussed a bit on slack I wonder if it could be related to the fact that currently we basically re-use the same RecyclerView in the pages (and this is something we will probably want to change passing to Fragment(State)PagerAdapter in an eventual rework as stated in phase 3 of your plan). Maybe could be good to make a quick test returning dummy (different) views for different pages to see if we get the same wrong behavior, not a solution of course, but maybe we can understand something more if it does not take much time to setup.

...ReaderPostListFragment lifecycle...

Could not check myself, but maybe could make sense to check in the NotificationsListFragment if (a part the PagerAdapter/FragmentPagerAdapter thing) we do something different/in different lifecycle places.

Also the plan sounds good to me, I think I would spend another bit more time to investigate the above before to go with one of the hacky way. If we do not come to a conclusion with it then we can check how good/bad the final result can be with the hacky ways to take a decision (we need to consider also (and not sure if we have a way tbh) how many users currently will have all the 4 tabs visible (so no issue), and how many will have some tabs out of screen for having more than the 4 standards tabs like Automattic users (but not only) have).

[renanferrari]

TL;DR: Both the ViewPager and the FilteredPagerAdapter are unnecessary to achieve the current behavior, so I've removed them both. The ViewPager is probably going to be necessary again sooner than later to deliver remaining functionalities like the swipe gesture, but that would require a bigger rework on the current implementation of FilteredRecyclerView. For now, I went ahead with a temporary solution that is not as hacky as the ones proposed before, which allows us to solve the scroll position bug, close this issue and move this more comprehensive discussion to another place.

---

Maybe could be good to make a quick test returning dummy (different) views for different pages to see if we get the same wrong behavior

After doing the suggested test above, I realized that the ViewPager being used inside FilteredRecyclerView was "behind" the RecyclerView in the view hierarchy:

https://github.com/wordpress-mobile/WordPress-Android/blob/d330001a7f92b5bc3caa1adae9c2daaf28b16495/WordPress/src/main/res/layout/filtered_list_component.xml#L56-L74

In other words, all the work being done by the FilteredPagerAdapter was actually not being used at all, which led me to the conclusion that both that and the ViewPager are currently not contributing to the solution nor the problem – in fact, they're unnecessary to achieve the current behavior.

The FilteredRecyclerView class has a custom logic to swap between "views" which was meant to be used with a Spinner. Trying to reuse this custom logic with a ViewPager has been problematic because the ViewPager itself serves the same purpose as the custom logic (i.e. they both exist to swap between views), so there are lots of overlapping functionalities, but at the same time they differ drastically in how they are implemented.

An alternative approach would be to either:

1. Get rid of the custom logic, keep the ViewPager and integrate it with both the TabLayout and the Spinner, or;
2. Get rid of the ViewPager, keep the custom logic and integrate it with both the TabLayout and the Spinner.

With Option 1 we could take advantage of the great integration between the ViewPager and TabLayout (which even solves the scroll position problem out of the box). On the downside, this would probably need a bigger rework of the entire FilteredRecyclerView class (maybe even creating another abstraction at the same level).

Option 2 is a lot simpler and wouldn't need lots of changes. On the other hand, we give up the advantages that ViewPager would bring us, including the swipe gesture and the smooth integration with Fragments.

In order to objectively close this issue and move this more comprehensive discussion to another place, I preferred to continue with option 2 for now, leaving option 1 for another moment, as we'll probably need to implement the remaining features for this screen really soon (the swipe gesture, for example).

That being said, I've implemented a temporary solution which removes both the ViewPager and the FilteredPagerAdapter and solves the scroll position bug by adding an OnLayoutChangeListener to the TabLayout to listen for updates in its layout and then forcing it to update its scroll position by using its setScrollPosition method. This implementation is on this branch and I should make a PR for it soon.