1176250 - Current tab sometimes not visible in Tabs Tray on tablet

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

Steps to reproduce:
1, add a load of tabs (25)
2, open tabs tray, notice last tab selected and visible
3, close tabs tray
4, scroll tab strip to first tab and select it
5, open tabs tray notice selected tab not visible

Fix:
Add smoothScrollToPosition(selected) to updateSelectedPosition method in TabsGridLayout

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Current tab sometimes not visible in Tabs Tray on tablet

As suggested this calls the smoothScrollToPosition within updateSelectedPosition, resulting in the above use case working as expected.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8624750 [details] [diff] [review]
Current tab sometimes not visible in Tabs Tray on tablet

Review of attachment 8624750 [details] [diff] [review]:
-----------------------------------------------------------------

The patch itself looks good to me. However this has one side effect. So when you have all these tabs open and the last one is selected. Now open the tabs panel and close one of the first tabs. Previously you stayed where you are, now you always scroll to the bottom - to the selected tab.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Setting NI just to get your opinion on the side effect. It can get annoying if you want to close a couple of tabs at the top but you always scroll back to the bottom.

Comment 4

[Martyn Haigh (:mhaigh)]

Good catch - I'll check this out.

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Current tab sometimes not visible in Tabs Tray on tablet

Alright - this is a new approach.  

Currently we clear out the data in the adapter when the Tabs Tray is hidden, and then refresh it when the tabs tray is shown - because of this the changes in the last patch resulted in us always resetting the scrollstate.  What we actually want to do is only change the scroll state when the selected tab has changed.  

In the hide method we call unregisterOnTabsChangedListener which means that we never get informed of selected tab changes when the tabs tray isn't shown.  So this patch saves the id of the selected tab and compares against this when we show the tabs tray next.  We only change the scroll position of the tabs tray if the selected tab has changed.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8625798 [details] [diff] [review]
Current tab sometimes not visible in Tabs Tray on tablet

Review of attachment 8625798 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with nit/comment.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +57,5 @@
>      final private boolean mIsPrivate;
>  
>      private final TabsLayoutAdapter mTabsAdapter;
>      private final int mColumnWidth;
> +    private int selectedTabId;

I wonder if we should make this more explicit and name it something like "lastSelectedTabId" because we only update it in hide() and not when selecting a tab.

@@ +193,5 @@
>          Tabs.registerOnTabsChangedListener(this);
>          refreshTabsData();
> +
> +        Tab currentSelectedTab = Tabs.getInstance().getSelectedTab();
> +        if(selectedTabId != currentSelectedTab.getId()) {

nit: space: if (..) {

Comment 7

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76fb9fb61720

Comment 8

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/7341164aa82a

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7341164aa82a