Current tab is not selected in tab tray on gingerbread devices

RESOLVED FIXED in Firefox 44

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Flaviu Cos, Assigned: mhaigh)

Tracking

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

Firefox Tracking Flags

(firefox40 unaffected, firefox41 unaffected, firefox42 unaffected, firefox43 affected, firefox44 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Environment: 
Device: Samsung Galaxy R (Android 2.3.4);
Build: Nightly 43.0a1 (2015-09-09);

Steps to reproduce:
1. Open a few tabs;
2. Open tab tray.

Expected result:
The current tab is selected in the tab tray.

Actual result:
The current tab is not selected in the tab tray.
Martyn?
Blocks: 1158277
Flags: needinfo?(mhaigh)
(Assignee)

Updated

3 years ago
Assignee: nobody → mhaigh
Flags: needinfo?(mhaigh)
(Assignee)

Comment 2

3 years ago
I can reproduce.  I have an idea what it may be.  This should only affect the gridview, which will soon be behind a nightly flag.
(Assignee)

Comment 4

3 years ago
Created attachment 8659301 [details]
MozReview Request: Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r?mcomella

Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r?mcomella
Attachment #8659301 - Flags: review?(michael.l.comella)
Comment on attachment 8659301 [details]
MozReview Request: Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r?mcomella

https://reviewboard.mozilla.org/r/18789/#review16921

If it works, wfm.

If you have any doubts, check that your recent Nightly flag changes don't effect the new-tabs-tray disabled state too.

::: mobile/android/base/tabs/TabsGridLayout.java:266
(Diff revision 1)
> -            if (AppConstants.Versions.feature11Plus) {
> +        if (AppConstants.Versions.feature11Plus) {

Can we just run the bottom algorithm on all views instead of having the branch here? I'd like to reduce the number of code paths if possible.

::: mobile/android/base/tabs/TabsGridLayout.java:267
(Diff revision 1)
> +            for (int i = 0; i < tabsAdapter.getCount(); i++) {

I think you want `getChildCount` instead of `tabsAdapter.getCount` – it's safer because otherwise you're assuming we have a view for every type, which we might not when we're recycling views.

::: mobile/android/base/tabs/TabsGridLayout.java:273
(Diff revision 1)
> +            for (int i = getFirstVisiblePosition(); i < tabsAdapter.getCount(); i++) {

Similarly, `tabsAdapter.getCount()` -> `getLastVisiblePosition`?

::: mobile/android/base/tabs/TabsGridLayout.java:280
(Diff revision 1)
> +                            ((TabsLayoutItemView) getViewForTab(tab)).setChecked(checked);

Store `getViewForTab` in a temporary variable to avoid calling it twice.
Attachment #8659301 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 6

3 years ago
> 
> ::: mobile/android/base/tabs/TabsGridLayout.java:266
> (Diff revision 1)
> > -            if (AppConstants.Versions.feature11Plus) {
> > +        if (AppConstants.Versions.feature11Plus) {
> 
> Can we just run the bottom algorithm on all views instead of having the
> branch here? I'd like to reduce the number of code paths if possible.
> 

Unfortunately just having setChecked doesn't work in my Lollipop test device, so looks like we'll have to keep both code paths in.
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/a7090080e87144f1f0cf92494c130f2f846eafdc
Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r=mcomella
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/812b25892cb6951f20aa754e47f089144f95e79b
Bug 1203122 - Current tab is not selected in tab tray on gingerbread devices; r=mcomella
(Assignee)

Comment 9

3 years ago
Actually I was wrong - I've stripped it down to one loop with a conditional statement within for 11plus devices
(Assignee)

Updated

3 years ago
Blocks: 1207201
https://hg.mozilla.org/mozilla-central/rev/812b25892cb6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Reporter)

Comment 11

3 years ago
Verified as fixed in build 44.0a1 2015-09-25;
Device: Samsung Galaxy R (Android 2.3.4).
status-firefox44: fixed → verified
You need to log in before you can comment on or make changes to this bug.