978741 - History tab is highlighted while Top sites tab is displayed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Flaviu Cos]

Environment: 
Device: Google Nexus 7 (Android 4.4.2);
Build: Nightly 30.0a1 (2014-03-03).

Steps to reproduce:
1. Launch Firefox;
2. Go to a website e.g. google.com;
3. Tap on the url bar;
4. Tap on the history tab;
5. Tap on android back button to go back to the opened website;
6. Tap on the url bar.

Expected result:
Top sites tab is opened and highlighted.

Actual result:
Top sites tab is opened but history tab is highlighted.

Notes:
The bug is also reproducible on Asus Transformer Tab (Android 4.0.3);
Please check the video: http://www.youtube.com/watch?v=J6OZg7VUkxY

Comment 1

[Flaviu Cos]

Last good revision: 58eca03214a6 (2014-02-28)
First bad revision: 8abc76dedec2 (2014-03-01)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58eca03214a6&tochange=8abc76dedec2

Comment 2

[Aaron Train [:aaronmt]]

This is reproducible in the tablet UI only.

Comment 3

[Aaron Train [:aaronmt]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d59f2795cc40&tochange=2ffb87a2c0cf

4bc6027f5638	Josh Dover — Bug 966047 - Hide banner when there are no panels enabled. r=mleibovic

?

Comment 4

[Aaron Train [:aaronmt]]

Attachment: screenshot.png

On startup on my Nexus 7 (2013), w/Nightly (03/03), top-sites is not highlighted

Comment 5

[Aaron Train [:aaronmt]]

Attachment: screenshot2.png

and then when performing the steps in comment #1, this is what the user will see

Comment 6

[:Margaret Leibovic]

Josh, did you ever test your patch on a tablet? I mentioned in bug 966047 comment 11 that this would impact the tab strip on tablets, we should figure out what's going on here, and potentially revert this change to just set the home banner enabled (now "active") state, rather than calling the page change listener.

Comment 7

[Josh Dover [:jdover]]

Attachment: patch

Built on try and this fixed the issue.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8384915 [details] [diff] [review]
patch

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

::: mobile/android/base/home/HomePager.java
@@ +216,5 @@
>      public void setCurrentItem(int item, boolean smoothScroll) {
>          super.setCurrentItem(item, smoothScroll);
>  
>          // Android doesn't call this when there is only one item
> +        if (getAdapter().getCount() == 1) {

We should also add a check here to make sure mHomeBanner isn't null. We should also update the comment, since it was originally written about the page change listener.

Comment 9

[Josh Dover [:jdover]]

Attachment: patch

Comment 10

[Aaron Train [:aaronmt]]

Ever find out yesterday why this wasn't reproducible on self made builds?

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/fx-team/rev/9ee8634f70dc

Comment 12

[:Margaret Leibovic]

Also make sure to request uplift approval!

Comment 13

[Josh Dover [:jdover]]

Comment on attachment 8384945 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 966047
User impact if declined: Hub tab strip on tablet will not highlight the correct page.
Testing completed (on m-c, etc.): landed on m-c 03/04
Risk to taking this patch (and alternatives if risky): It continues not to work, due to some strange difference between local builds and nightly builds. 
String or IDL/UUID changes made by this patch: none.

Comment 14

[Josh Dover [:jdover]]

(In reply to Aaron Train [:aaronmt] from comment #10)
> Ever find out yesterday why this wasn't reproducible on self made builds?

Sadly no. I would checkout the same versions and as the various nightly builds and run a clobber and full build and it would work correctly, but the corresponding nightly didn't. I suspect some issue with some sort of optimization?

Comment 15

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/9ee8634f70dc

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8384945 [details] [diff] [review]
patch

Josh, I updated the status flag for 29 as affected since I guess it is the reality (otherwise, you would not request an uplift)

Comment 17

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb67f9c0da24

Comment 18

[(PTO June 19-July 7) Ryan VanderMeulen]

Backed out from Aurora for Android bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/b88a05e76c11

https://tbpl.mozilla.org/php/getParsedLog.php?id=35664361&tree=Mozilla-Aurora

Comment 19

[:Margaret Leibovic]

I'm backing this out of m-c, since it didn't actually work:
https://hg.mozilla.org/integration/fx-team/rev/2c0d7218a005

I also backed out bug 966047 from m-c and aurora, so in the end, this issue was actually FIXED by those backouts.

Comment 20

[Flaviu Cos]

Verified as fixed in builds:
- 29.0a2 (2014-03-06);
- 30.0a1 (2014-03-06);
Device: Lenovo Yoga Tab 10 (Android 4.2.2).