Current tab sometimes not visible in Tabs Tray on tablet

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mhaigh, Assigned: mhaigh)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 42
All
Android
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Summary: Current tab not selected in Tabs Tray on tablet → Current tab sometimes not visible in Tabs Tray on tablet
(Assignee)

Comment 1

4 years ago
Created attachment 8624750 [details] [diff] [review]
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+
Blocks: 1024426
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)
(Assignee)

Comment 4

4 years ago
Good catch - I'll check this out.
Flags: needinfo?(mhaigh)
(Assignee)

Comment 5

4 years ago
Created attachment 8625798 [details] [diff] [review]
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.
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/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 (..) {
Attachment #8625798 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/7341164aa82a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Target Milestone: Firefox 41 → Firefox 42
You need to log in before you can comment on or make changes to this bug.