Closed Bug 1176250 Opened 6 years ago Closed 6 years ago

Current tab sometimes not visible in Tabs Tray on tablet


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox42 fixed)

Firefox 42
Tracking Status
firefox42 --- fixed


(Reporter: mhaigh, Assigned: mhaigh)




(1 file, 1 obsolete file)

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

Add smoothScrollToPosition(selected) to updateSelectedPosition method in TabsGridLayout
Summary: Current tab not selected in Tabs Tray on tablet → 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.
Attachment #8624750 - Flags: review?(s.kaspari)
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.
Attachment #8624750 - Flags: review?(s.kaspari) → review+
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.
Flags: needinfo?(mhaigh)
Good catch - I'll check this out.
Flags: needinfo?(mhaigh)
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.
Attachment #8624750 - Attachment is obsolete: true
Attachment #8625798 - Flags: review?(s.kaspari)
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/
@@ +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 (..) {
Attachment #8625798 - Flags: review?(s.kaspari) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Target Milestone: Firefox 41 → Firefox 42
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.