Closed Bug 1202753 Opened 6 years ago Closed 6 years ago

Regression: tabs tray no longer autoscrolls to active tab when switching to tabs tray

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox43 affected, firefox44 fixed, fennecNightly+)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox43 --- affected
firefox44 --- fixed
fennec Nightly+ ---

People

(Reporter: liuche, Assigned: sebastian)

References

Details

Attachments

(1 file)

This looks like a regression in behavior from tabs tray behavior from before. When switching from single tab view to tabs tray, the active tab should be visible without scrolling.

STR:
1. Open a bunch of tabs, so they exceed what can be viewed in the tabs tray without scrolling.
2. Select and expand first tab.
3. Add a new tab (which becomes the active tab)
4. Switch to tabs tray.

Expected: Current active tab is visible and selected.
Actual: Can't see active tab in the tabs tray, because it's off screen. Disorienting, because your current tab should be visible in the tabs tray.
Summary: Regression: tabs tray no longer scrolls to active tab when switching to tabs tray → Regression: tabs tray no longer autoscrolls to active tab when switching to tabs tray
tracking-fennec: --- → ?
Assignee: nobody → mhaigh
tracking-fennec: ? → Nightly+
Hm - I can't reproduce this.  This is what I see : https://www.youtube.com/watch?v=pzpYmH7K8qg

liuche: What device are you using?  Am I doing something wrong?
Flags: needinfo?(liuche)
I can repro if using the new tab button in the tabs tray.
Yup, just coming here to comment on that - I open new tabs from the tabs tray, not the settings menu.
Flags: needinfo?(liuche)
Assignee: mhaigh → s.kaspari
Status: NEW → ASSIGNED
I also see this every time I tab-queue multiple tabs. Tapping on the "X tabs waiting" notification opens Fennec in the Tabs Tray, and the tray never scrolls down to the newly opened tabs.
Bug 1202753 - Tabs tray (Grid): Scroll to selected tab if it is not visible. r?margaret
Attachment #8678060 - Flags: review?(margaret.leibovic)
Comment on attachment 8678060 [details]
MozReview Request: Bug 1202753 - Tabs tray (Grid): Scroll to selected tab if it is not visible. r?margaret

https://reviewboard.mozilla.org/r/23083/#review20567

I have a few comments to consider, but overall this seems fine.

::: mobile/android/base/tabs/TabsGridLayout.java:175
(Diff revision 1)
> -        if (lastSelectedTabId != currentlySelectedTab.getId()) {
> +        final int position =  currentlySelectedTab != null ? tabsAdapter.getPositionForTab(currentlySelectedTab) : -1;

When can the selected tab be null? If this is unexpected, does that signal a more serious problem? Also, it looks like the logic for `getPositionForTab` already handles being passed in a null tab:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabsLayoutAdapter.java#68

::: mobile/android/base/tabs/TabsGridLayout.java:182
(Diff revision 1)
> +            }

I'm looking at `TabsListLayout` to see if we have similar logic, and I'm having trouble finding it. Is this taken care of in the list view by the logic to update the selected tab state when we show the panel?
Attachment #8678060 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #7)
> When can the selected tab be null? If this is unexpected, does that signal a
> more serious problem?

Good question. IntelliJ was showing warnings because getSelectedTab() is annotated with @Nullable. So I just accepted that and added the null check. The javadoc says: "The selected tab can be null if we're doing a session restore after a crash and Gecko isn't ready yet.".

(In reply to :Margaret Leibovic from comment #7)
> Also, it looks like the logic for `getPositionForTab`
> already handles being passed in a null tab:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/
> TabsLayoutAdapter.java#68

That's nice! I'm going to change that line. :)

(In reply to :Margaret Leibovic from comment #7)
> I'm looking at `TabsListLayout` to see if we have similar logic, and I'm
> having trouble finding it. Is this taken care of in the list view by the
> logic to update the selected tab state when we show the panel?

Yeah. The list updates the selection every time. So just closing and re-opening the tabs tray might scroll the list. That's why it is not reproducible there. I'm really looking forward to a single implementation based on RecyclerView (bug 1116415).
https://hg.mozilla.org/integration/fx-team/rev/a829347b8691b54ab85103a9c391a8d6ce5c2b28
Bug 1202753 - Tabs tray (Grid): Scroll to selected tab if it is not visible. r=margaret
https://hg.mozilla.org/mozilla-central/rev/a829347b8691
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.