846569 - Tab list should keep scroll position when closing a tab, currently jumps back to active tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox for Android Nightly 20130228, Android 4.1.2 (stock), Google Nexus S

Tab list should keep scroll position when closing a tab, currently jumps back to active tab

Steps to reproduce:
1. Open 5 tabs.
2. Select the last tab.
3. Open the tab list.
4. Close the first tab by slide.

Actual result:
View jumps back to last tab, for closing the former second tab, you have to scroll up again.

Expected result:
No scroll jump after closing first tab.

Comment 3

[:Margaret Leibovic]

This has been really annoying me and I want to fix it. Doing some debugging in TabsTray, I believe this issue was caused by bug 842609.

I found that not calling setSelection() fixes this problem, but that doesn't seem like a good fix, because the selected position *is* actually changing when tabs higher up in the list are closed. So maybe the right fix for this is in TwoWayView? Sriram, do you have any idea? Basically, if the tabs tray is open and the selected tab isn't changing, we shouldn't be scrolling the list.

Comment 4

[Sriram Ramasubramanian [:sriram]]

The adapter changes and so notifyDatasetChanged() is called. This refreshes the list, which inturn sets the "selected tab". I don't find a better way to do this.

Comment 5

[Richard Newman [:rnewman]]

At the very least we don't need to call setSelection if the removed item is below the current tab, right? 

And otherwise, can we override an implementation somewhere to avoid scrolling to the newly selected item?

Comment 6

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #5)
> At the very least we don't need to call setSelection if the removed item is
> below the current tab, right? 

If we don't call setSelection at all, the styling of the selected tab still looks correct, since the setItemChecked call [1] is what actually takes care of styling the selected item. However, I wonder if there are other consequences if we decide to just not call setSelection when a tab is closed while the tabs tray is open.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#206

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #6)
> (In reply to Richard Newman [:rnewman] from comment #5)
> > At the very least we don't need to call setSelection if the removed item is
> > below the current tab, right? 
> 
> If we don't call setSelection at all, the styling of the selected tab still
> looks correct, since the setItemChecked call [1] is what actually takes care
> of styling the selected item. However, I wonder if there are other
> consequences if we decide to just not call setSelection when a tab is closed
> while the tabs tray is open.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.
> java#206

Selection is a kinda misleading name in the AbsListView world. The 'selection' forces a specific position in the AdapterView to be at the top of the viewport. See:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/TwoWayView.java#3654

The notifyDataSetChanged() call should not reset the scroll state in itself. If it's doing that, it's a bug in TwoWayView. So, the setSelection() call in updateSelectedPosition() is simply forcing the selected tab row to be visible, which might be what we want in some cases e.g. when you show the tabs tray, we want to ensure the selected tab is visible.

Simply removing the setSelection() from updateSelectedPosition() is going to cause regressions. We just need to avoid setSelection() specifically when *removing* a tab.

Comment 8

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> (In reply to :Margaret Leibovic from comment #6)
> > (In reply to Richard Newman [:rnewman] from comment #5)
> > > At the very least we don't need to call setSelection if the removed item is
> > > below the current tab, right? 
> > 
> > If we don't call setSelection at all, the styling of the selected tab still
> > looks correct, since the setItemChecked call [1] is what actually takes care
> > of styling the selected item. However, I wonder if there are other
> > consequences if we decide to just not call setSelection when a tab is closed
> > while the tabs tray is open.
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.
> > java#206
> 
> Selection is a kinda misleading name in the AbsListView world. The
> 'selection' forces a specific position in the AdapterView to be at the top
> of the viewport. See:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/
> TwoWayView.java#3654
> 
> The notifyDataSetChanged() call should not reset the scroll state in itself.
> If it's doing that, it's a bug in TwoWayView. So, the setSelection() call in
> updateSelectedPosition() is simply forcing the selected tab row to be
> visible, which might be what we want in some cases e.g. when you show the
> tabs tray, we want to ensure the selected tab is visible.
> 
> Simply removing the setSelection() from updateSelectedPosition() is going to
> cause regressions. We just need to avoid setSelection() specifically when
> *removing* a tab.

Yes, indeed. Thanks for clarifying what setSelection() is all about, I'll work on a patch to avoid calling setSelection() when removing a tab.

Comment 9

[:Margaret Leibovic]

Attachment: patch

This splits out the style logic from the selection logic.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 829012 [details] [diff] [review]
patch

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

Looks good.

Comment 11

[:Margaret Leibovic]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea9995f3f858