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

VERIFIED FIXED in Firefox 28

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: aryx, Assigned: Margaret)

Tracking

Trunk
Firefox 28
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified)

Details

Attachments

(1 attachment)

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.

Updated

5 years ago
Duplicate of this bug: 896190

Updated

5 years ago
Duplicate of this bug: 914550
(Assignee)

Comment 3

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

Comment 6

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

Comment 8

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

Comment 9

5 years ago
Created attachment 829012 [details] [diff] [review]
patch

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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team][qa+] → [qa+]

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox28: --- → verified
Whiteboard: [qa+]
(Assignee)

Updated

5 years ago
Depends on: 839885
You need to log in before you can comment on or make changes to this bug.