Maybe have the tabs tray scroll on every SELECTED instead of only on ADDED

NEW
Assigned to

Status

()

Firefox for Android
General
7 months ago
7 months ago

People

(Reporter: Tom Klein, Assigned: Tom Klein)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
To work around some scrolling issues (which I'll describe in follow up comments) when rewriting the tabs tray to use RecyclerView, I moved the scrollToPosition from SELECTED to ADDED in TabsLayout.onTabChanged ([1] changed to [2]).

Unfortunately at the time I wasn't tuned into tabs queue interactions with the tabs tray, and it turns out there is one case, associated with opening tabs from another app, where we *may* need to scroll on SELECTED and not ADDED:

1) Turn on tab queue.
2) Open more tabs than will fit in the tabs tray at once.
3) Open a link from an app that uses EXTRA_APPLICATION_ID using "Open now" from the overlay on the other app.
4) Open fennec and scroll the newly opened tab out of view.
5) Go back to the app from step 3 and open another link using "Open now".  Because of EXTRA_APPLICATION_ID and "Open now", this link will be opened in the same tab as in step 3 (but see bug 1353226 comment 1 for more discussion and some inconsistencies in this case - opening this second tab in the same tab as step 3 when the tabs queue is turned on should maybe not even be possible).

In that case (and that case only, as far as I know - please let me know of others), we *may* need to scroll to the Step 3 tab when fennec reopens using a SELECTED only, without any associated ADDED [I'll have more to say on that in followup comments]. (The other situation where we sometimes need to scroll to a position when the tabs tray is already open is a tab close-undo, but that always includes an ADDED with the SELECTED.)

[1] https://hg.mozilla.org/mozilla-central/file/28cab19aa7f1/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java#l129
[2] https://dxr.mozilla.org/mozilla-central/rev/9aacfa8081b35bb8ae1a59ce3fd9d7aba57cfc7b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java#113
(Assignee)

Comment 1

7 months ago
Other steps to reproduce with tabs queue off (derp):
1) Turn off tabs queue.
2) Open more tabs than will fit in the tabs tray at once.
3) Open a link from an app that uses EXTRA_APPLICATION_ID.
4) Scroll the tab opened in step 3 off screen.
5) Go back to the app from step 3 and open another link.

Currently bug 1353226 already breaks the step 5) scroll in a different place, as described in bug 1353226 comment 1.  My current thinking on fixing that bug is to send a (new) RESELECTED event instead of resending a SELECTED event if we try to select a tab that's already selected, which would mean we still wouldn't *need* to do a scrollToPosition on every SELECTED in TabsLayout.  Also, depending on the outcome of bug 1350994, we might just always wind up closing the tabs tray when any link is opened from another app, in which case this won't be needed (unless there are still other cases I'm unaware of).

Still, from a purely events-oriented perspective, disregarding the way in which they're currently being used, it does feel like a SELECTED should come with a scrolled-to (an ADDED should as well, but every ADDED already comes with a SELECTED, so the SELECTED would be enough).  Maybe this will be a case of YAGNI, and that's fine, but I'll go ahead and post on what the RecyclerView issue with doing a scroll on every SELECTED is, and then I'll post a solution, and then we can go from there.
(Assignee)

Comment 2

7 months ago
Created attachment 8854309 [details]
tabs tray grid scroll-on-selected issue scenario

Consider the RecyclerView grid layout in the attachment.  Assume there's plenty of room to scroll, but for simplicity's sake, number the visible tabs starting at 0, so the currently selected mozilla tab is tab 5.  When we close tab 5, we first SELECT tab 6, then unselect tab 5, then close tab 5.  That all happens in one call chain on the UI thread, so it all happens before a new layout occurs.

IF we were to do a scrollToPosition on the SELECT, then we would tell RecyclerView to scroll to tab 6 (which is currently not fully in view, so would need to be scrolled down into full view), then we would close tab 5, and then a layout would occur.  What we would hope would be that RecyclerView would notice that we closed tab 5, and so now must actually want to scroll to the new tab 5 instead of what was originally tab 6.  But that's not what it does, it scrolls to the new tab 6, which pushes the now selected new tab 5 halfway off the top of the screen.  (Apparently GridView's setSelection handled this situation correctly.)  This issue doesn't arise with the list layout.
You need to log in before you can comment on or make changes to this bug.