current tab should be visible by default upon opening tab drawer

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: pwd.mozilla, Assigned: Margaret)

Tracking

({regression})

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

Firefox Tracking Flags

(firefox27 unaffected, firefox28 verified, firefox29 verified, firefox30 verified, b2g-v1.3 fixed, fennec28+)

Details

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130208 Firefox/21.0
Build ID: 20130208031053

Steps to reproduce:

Current implementation shows the first tabs where as we should show current tab.
OS: Windows 7 → Android
Hardware: x86 → ARM
This works for me. I have 15 tabs open, select the 13th, close the tab panel, re-open the tab panel and it's currently selected and in view. Do you have any better steps to reproduce, also can you try 20130210?
1: Have a bunch of tabs open
2: Open new tab via awesomescreen
3: Open tab drawer
4: Notice that I'm at the top of the tab drawer rather than at the bottom/having the selected tab in view

This has just been confirmed in 20130210
Still seeing this on 20120223, can you confirm Aaron?
20130223*
Yes, I see this when the newly opened tab is not in view. Also see this with the new landscape horizontal scroller.
Assignee: nobody → sriram
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can reproduce this bug using the steps from comment 2.
Margaret, looks like a potential regression from bug 846569. Could you check?
Flags: needinfo?(margaret.leibovic)
(In reply to Lucas Rocha (:lucasr) from comment #8)
> Margaret, looks like a potential regression from bug 846569. Could you check?

If this was a regression from bug 846569, how was it filed a year ago?

Was it fixed by something else at some point, but then got broken by bug 846569?
Assignee: sriram.mozilla → margaret.leibovic
Flags: needinfo?(margaret.leibovic) → needinfo?(flaviu.cos)
Last good revision: f003c386c77a (2013-11-08)
First bad revision: 9e571ad29946 (2013-11-09)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f003c386c77a&tochange=9e571ad29946
Flags: needinfo?(flaviu.cos)
Regression window for the fix from an year ago:

Last bad revision: 73f0c5b00572 (2013-02-26)
First good revision: e7632ab657e5 (2013-02-27)

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73f0c5b00572&tochange=e7632ab657e5
cade02c975db	Sriram Ramasubramanian — Bug 842609: Scroll to selected tab in tabs ui. [r=mfinkle]

?
tracking-fennec: --- → ?
tracking-fennec: ? → 28+
The patch for bug 846569 changed the logic order in updateSelectedPosition, and that's what appears to have caused this problem.

I'm not familiar with TwoWayView, but I'm not sure if this is expected behavior, or perhaps some unexpected assumptions are baked into setSelectionFromOffset.
Attachment #8379241 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8379241 [details] [diff] [review]
Update the selected tab style before calling setSelection

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

::: mobile/android/base/TabsTray.java
@@ +201,5 @@
>  
>          // Updates the selected position in the list so that it will be scrolled to the right place.
>          private void updateSelectedPosition() {
>              int selected = getPositionForTab(Tabs.getInstance().getSelectedTab());
> +            updateSelectedStyle(selected);

This is smelling like a TwoWayView bug. However, I also noticed that updateSelectedStyle() is going through all adapter items and setting their checked state one by one which is not really necessary and might be causing a excessive amount of requestLayout() calls in the view. The TabsTray view is using choiceMode=single which means we can just setItemChecked(selectedPosition, true) and TwoWayView will do the right thing for us.

I'm fine with landing your patch but, before doing so, could you try just changing updateSelectedStyle() to just call TabsTray.this.setItemChecked(selected, true) to see if that fixes the bug?
Attachment #8379241 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #14)
> Comment on attachment 8379241 [details] [diff] [review]
> Update the selected tab style before calling setSelection
> 
> Review of attachment 8379241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/TabsTray.java
> @@ +201,5 @@
> >  
> >          // Updates the selected position in the list so that it will be scrolled to the right place.
> >          private void updateSelectedPosition() {
> >              int selected = getPositionForTab(Tabs.getInstance().getSelectedTab());
> > +            updateSelectedStyle(selected);
> 
> This is smelling like a TwoWayView bug. However, I also noticed that
> updateSelectedStyle() is going through all adapter items and setting their
> checked state one by one which is not really necessary and might be causing
> a excessive amount of requestLayout() calls in the view. The TabsTray view
> is using choiceMode=single which means we can just
> setItemChecked(selectedPosition, true) and TwoWayView will do the right
> thing for us.
> 
> I'm fine with landing your patch but, before doing so, could you try just
> changing updateSelectedStyle() to just call
> TabsTray.this.setItemChecked(selected, true) to see if that fixes the bug?

Looking back at history, we used to to this, but we changed it at one point:
http://hg.mozilla.org/mozilla-central/rev/fc9b2b7ebfbd

In bug 834525 comment 8, Sriram mentions doing this to fix a focus issue. We can investigate whether or not this would still pose a problem, but I'd rather do that in another bug.
Blocks: 846569
Keywords: regression
Comment on attachment 8379241 [details] [diff] [review]
Update the selected tab style before calling setSelection

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 846569
User impact if declined: tabs tray is not scrolled to current tab when tabs tray is opened
Testing completed (on m-c, etc.): tested locally, landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, reverting logic re-ordering from bug 846569
String or IDL/UUID changes made by this patch: none
Attachment #8379241 - Flags: approval-mozilla-beta?
Attachment #8379241 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/66b8fc136e5e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attachment #8379241 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8379241 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed in builds:
- 28 beta 6;
- 29.0a2 (2014-02-25);
- 30.0a1 (2014-02-25);
Device: Google Nexus 7 (Android 4.4.2).
You need to log in before you can comment on or make changes to this bug.