Closed
Bug 839885
Opened 12 years ago
Closed 11 years ago
current tab should be visible by default upon opening tab drawer
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox27 unaffected, firefox28 verified, firefox29 verified, firefox30 verified, b2g-v1.3 fixed, fennec28+)
RESOLVED
FIXED
Firefox 30
People
(Reporter: tech4pwd, Assigned: Margaret)
References
Details
(Keywords: regression)
Attachments
(1 file)
1.31 KB,
patch
|
lucasr
:
review+
Sylvestre
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
OS: Windows 7 → Android
Hardware: x86 → ARM
Comment 1•12 years ago
|
||
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?
Reporter | ||
Comment 2•12 years ago
|
||
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
Reporter | ||
Comment 3•12 years ago
|
||
Still seeing this on 20120223, can you confirm Aaron?
Reporter | ||
Comment 4•12 years ago
|
||
20130223*
Comment 5•12 years ago
|
||
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
Comment 7•11 years ago
|
||
I can reproduce this bug using the steps from comment 2.
status-firefox27:
--- → unaffected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
Comment 8•11 years ago
|
||
Margaret, looks like a potential regression from bug 846569. Could you check?
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 9•11 years ago
|
||
(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)
Comment 10•11 years ago
|
||
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)
Comment 11•11 years ago
|
||
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
Comment 12•11 years ago
|
||
cade02c975db Sriram Ramasubramanian — Bug 842609: Scroll to selected tab in tabs ui. [r=mfinkle]
?
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
tracking-fennec: ? → 28+
Assignee | ||
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
(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.
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Blocks: 846569
Keywords: regression
Assignee | ||
Comment 17•11 years ago
|
||
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?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Updated•11 years ago
|
Attachment #8379241 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•11 years ago
|
||
Updated•11 years ago
|
Attachment #8379241 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•11 years ago
|
||
Comment 21•11 years ago
|
||
Updated•11 years ago
|
status-b2g-v1.3:
--- → fixed
Comment 22•11 years ago
|
||
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).
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•