Closed Bug 846569 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox28 --- verified

People

(Reporter: aryx, Assigned: Margaret)

References

Details

Attachments

(1 file)

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.
Duplicate of this bug: 896190
Duplicate of this bug: 914550
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.
Blocks: 842609
Flags: needinfo?(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.
Flags: needinfo?(sriram)
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?
(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
(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.
(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.
Assignee: nobody → margaret.leibovic
Attached patch patchSplinter Review
This splits out the style logic from the selection logic.
Attachment #829012 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 829012 [details] [diff] [review]
patch

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

Looks good.
Attachment #829012 - Flags: review?(lucasr.at.mozilla) → review+
Status: NEW → ASSIGNED
Whiteboard: [fixed in fx-team][qa+]
Target Milestone: --- → Firefox 28
https://hg.mozilla.org/mozilla-central/rev/ea9995f3f858
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team][qa+] → [qa+]
Status: RESOLVED → VERIFIED
Whiteboard: [qa+]
Depends on: 839885
You need to log in before you can comment on or make changes to this bug.