Closed Bug 874036 Opened 8 years ago Closed 8 years ago

Tab counter displays incorrectly in some circumstances

Categories

(Firefox for Android :: General, defect)

24 Branch
ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox22 --- unaffected
firefox23 + verified
firefox24 + verified
fennec 24+ ---

People

(Reporter: capella, Assigned: capella)

References

Details

(Keywords: reproducible, Whiteboard: [testday-20130726])

Attachments

(1 file, 3 obsolete files)

STR on my GS3 4.1.2:

open FF, navigate to http://arstechnica.com/ (mobile version of site)
Scroll down through article list so awesomebar scrolls out of sight
Long click an article and open in new tab
Tap hardware menu twice to re-expose awesomebar and tab counter
Click tab counter to open tabstray
Swipe closed the current / first tab (http://arstechnica.com/)
Tap on single remaining listitem

Observe that tab counter now indicates "2" tabs open, where only "1" is actually open.
I assume Fx23 and Fx24 are affected? What about Fx22?
Assignee: nobody → bnicholson
Fx22 doesn't do the awesomebar auto-hide thing in the version I downloaded from Play (labeled Beta) and seems to work anyhow
Removing the else / return here fixes the problem, new code for me but logically seems safe

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabCounter.java#76
So, I was browsing ycombinator in nightly this morning, and after touching to open in new tab, the tab counter still said 3, instead of 4.

I waited for page to load, opened tab bar a few times, switched back and forth between tabs, still didn't increment.

I opened another new tab, counter jumped from 3 to 5.

Would this also be fixed by the proposed patch?
If this is only on 23, it looks like it's probably a regression from bug 863828 or bug 865228.

(In reply to Mark Capella [:capella] from comment #3)
> Removing the else / return here fixes the problem, new code for me but
> logically seems safe
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabCounter.
> java#76

Thanks for investigating. So it looks that we should only hit that else case if count == mCount, in which case we wouldn't be changing the number anyway. We should add some logging to see what the numbers are when this is happening.

It also looks like the logic in here could use some auditing:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#753

I think it's bad that we have a setCount method on TabCounter, but that we're also manually calling setCurrentText here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#782
Did some logging already, in the STR example i gave I get


D/XYZZY   (12432): TabCounter.java setCount() hears a call, mCount is: 0 count is: 0
D/XYZZY   (12432): TabCounter.java setCount() returns, as mCount == count
D/XYZZY   (12432): BrowserToolbar.java onTabChanged() hears a RESTORED message
D/XYZZY   (12432): BrowserToolbar.java updateTabCount() hears a call: 1

D/XYZZY   (12432): BrowserToolbar.java fromAwesomeBarSearch() hears a call
D/XYZZY   (12432): BrowserToolbar.java updateTabCountAndAnimate() hears a call: 1
D/XYZZY   (12432): TabCounter.java setCount() hears a call, mCount is: 0 count is: 1
D/XYZZY   (12432): TabCounter.java setCount() mCount < count

D/XYZZY   (12432): BrowserToolbar.java updateTabCountAndAnimate() hears a call: 2
D/XYZZY   (12432): BrowserToolbar.java updateTabCountAndAnimate() returns as isVisible() is not true
D/XYZZY   (12432): BrowserToolbar.java updateTabCount() hears a call: 2

D/XYZZY   (12432): Tabs.java closeTab() hears a call
D/XYZZY   (12432): Tabs.java removeTab() hears a call
D/XYZZY   (12432): Tabs.java removeTab() found a tab to remove: 0
D/XYZZY   (12432): Tabs.java closeTab() uses passed-in nextTab: 1
D/XYZZY   (12432): BrowserToolbar.java onTabChanged() hears a CLOSED message
D/XYZZY   (12432): BrowserToolbar.java updateTabCountAndAnimate() hears a call: 1
D/XYZZY   (12432): TabCounter.java setCount() hears a call, mCount is: 1 count is: 1
D/XYZZY   (12432): TabCounter.java setCount() returns, as mCount == count

The last step of course is the interesting part
I think the problem is that BrowserToolbar.updateTabCount() calls mTabsCounter.setCurrentText, which changes the number, but doesn't change mCount in TabsCounter. I think we should update this code to make sure TabCounter is always the thing that handles updating the text or not. And a good cleanup patch would be to consolidate the shared logic in updateTabCount and updateTabCountAndAnimate. They could probably become one method, and that method could just tell TabCounter whether or not to animate while updating the count.

Capella, do you want to take this bug? It sounds like you already got us 90% of the way to fixing it :)
Sure! That sounds better ... my first thought was a workable bandaid but yours gets to the problem ...
Assignee: bnicholson → markcapella
Status: NEW → ASSIGNED
FYI ... It looks like there's a separate regression involved that can explain comment #4 ... STR:

open FF, navigate to http://arstechnica.com/ (mobile version of site)
Long click an article and open in new private tab
Long click and open another article and in new private tab
Long click and open another article and in new private tab

Tap TabCounter to open TabsTray
Switch to Private Tab and select first of three items in list

In Fx22 TabsCounter now displays "3"
In Fx24 TabsCounter still displays "1"
tracking-fennec: --- → ?
Well, I wasn't using private tabs, but maybe it is related anyway?
I only brought it up, actually, since usually things work fine.
I'm testing a patch that addresses both reproducible issues as I discovered ...

Can you reproduce yours ?
Summary: Tab counter is inflated +1 to wrong number → Tab counter displays incorrectly in some circumstances
Attached patch Patch (v1) (obsolete) — Splinter Review
This patch corrects both problems as documented in comment 0 and comment 9 ... I've tested it with both regular and private tabs and switching back and forth, and some amount of recovery situations.

Let me know if I've made any glaring errors or you can think of test cases I need to consider / try ?
Attachment #752426 - Flags: review?(lucasr.at.mozilla)
Dang... never QREF'd ... re-testing patch will re-post ...
Attached patch Patch (v2) (obsolete) — Splinter Review
Ok then, hg QRef'd, re-pulled, re-based, re-tested, re-posted...
Attachment #752426 - Attachment is obsolete: true
Attachment #752426 - Flags: review?(lucasr.at.mozilla)
Attachment #752688 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 752688 [details] [diff] [review]
Patch (v2)

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

Looks good but needs to handle the new tab animation.

::: mobile/android/base/BrowserToolbar.java
@@ -756,5 @@
> -        // If toolbar is selected, this means the entry is expanded and the
> -        // tabs button is translated offscreen. Don't trigger tabs counter
> -        // updates until the tabs button is back on screen.
> -        // See fromAwesomeBarSearch()
> -        if (!mLayout.isSelected()) {

Removing this will regress one of the refinements I've made on the toolbar (see patch 1/4 in bug 865228). Basically, we want to wait for the url bar animation to finish before animating the tab counter. To verify this, enter awesomescreen by selecting 'add tab'. Once you select or type an URL, you should see the URL bar shrink back to its original size *then* the tab counter animation.

::: mobile/android/base/TabCounter.java
@@ +64,5 @@
>                  });
>          }
>      }
>  
> +    public void setAnimatedCount(int count) {

setCountWithAnimation would probably be a better name for this method.
Attachment #752688 - Flags: review?(lucasr.at.mozilla) → review-
Thanks !

That first bit works still, it's been combined into the updateTabCount() method ... 

Fyi also, In that same updateTabCount() method, we were doing:
// Set TabCounter based on visibility
if (isVisible()) {
...

This produces a tiny visual artifact during animation (swiping closed a tab when the tabstray is open && TabsCounter is hidden by it's alpha being == 0), so I tidied it up by changing it to:

// Set TabCounter based on visibility
if (isVisible() && mTabsCounter.getAlpha() != 0) {
...
Attached patch Patch (v2) (obsolete) — Splinter Review
Renamed setAnimatedCount() to setCountWithAnimation() ...

I actually had called it that while testing but decided to change it ... both work for me :D
Attachment #752688 - Attachment is obsolete: true
Attachment #752817 - Flags: review?(lucasr.at.mozilla)
Test build?
tracking-fennec: ? → 24+
Combined TRY build for bug 874036 and bug 862445 (also bug 840144 if you want to play with that)... 

builds ok but 2.2 M() and R() testing is failing 2400-sec timeouts and I'm not sure why ... I downloaded from TRY and tested Ok... just fyi for now ... could be an issue with bug 840144 I saw from before (not related to this patch)

https://tbpl.mozilla.org/?tree=Try&rev=b095e4b909ca
(In reply to Mark Capella [:capella] from comment #19)
> Combined TRY build for bug 874036 and bug 862445 (also bug 840144 if you
> want to play with that)... 
> 
> builds ok but 2.2 M() and R() testing is failing 2400-sec timeouts and I'm
> not sure why ... I downloaded from TRY and tested Ok... just fyi for now ...
> could be an issue with bug 840144 I saw from before (not related to this
> patch)
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b095e4b909ca

Problem is likely here:

>+        if (isVisible() && mTabsCounter.getAlpha() != 0) {

View.getAlpha() is version 11 (Android 3.0 - Honeycomb) or higher. The Android 2.2 tests are all failing.
(In reply to Mark Finkle (:mfinkle) from comment #20)
> (In reply to Mark Capella [:capella] from comment #19)
> > Combined TRY build for bug 874036 and bug 862445 (also bug 840144 if you
> > want to play with that)... 
> > 
> > builds ok but 2.2 M() and R() testing is failing 2400-sec timeouts and I'm
> > not sure why ... I downloaded from TRY and tested Ok... just fyi for now ...
> > could be an issue with bug 840144 I saw from before (not related to this
> > patch)
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=b095e4b909ca
> 
> Problem is likely here:
> 
> >+        if (isVisible() && mTabsCounter.getAlpha() != 0) {
> 
> View.getAlpha() is version 11 (Android 3.0 - Honeycomb) or higher. The
> Android 2.2 tests are all failing.

Correct. Use ViewHelper.setAlpha() instead. It's a compatibility layer I added to our little animation framework.
Comment on attachment 752817 [details] [diff] [review]
Patch (v2)

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

Looks good but needs work on the tab counter's visibility tracking code (as discussed on IRC)

::: mobile/android/base/BrowserToolbar.java
@@ +461,5 @@
>                      // Reset the title in case we haven't navigated to a new page yet.
>                      updateTitle();
>                  }
>                  break;
> +            case SELECTED:

So, we don't update the counter when a tab is restored anymore? Looks suspicious. I bit of context here would help.
Attachment #752817 - Flags: review?(lucasr.at.mozilla) → review-
a SELECTED seems to always follow a RESTORE .... I though to avoid an extra call

SELECTED is required to fix the second bug mentioned in comment 9
Attached patch Patch (v3)Splinter Review
Thanks to you both! ViewHelper did the trick ...

This updated patch fixes 3 bugs from comment 0, comment 9, and flickering bug from comment 16.

TRY push: https://tbpl.mozilla.org/?tree=Try&rev=621195d0d16c
Attachment #752817 - Attachment is obsolete: true
Attachment #753761 - Flags: review?(lucasr.at.mozilla)
Keywords: reproducible
Comment on attachment 753761 [details] [diff] [review]
Patch (v3)

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

Nice.
Attachment #753761 - Flags: review?(lucasr.at.mozilla) → review+
Yah! (Was fun) Even the small patches wind up teaching me things :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/88195932371c
https://hg.mozilla.org/mozilla-central/rev/88195932371c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Status: RESOLVED → VERIFIED
Depends on: 880592
Blocks: 868222
Comment on attachment 753761 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Noticible incorrect information displayed in TabsCounter
Testing completed (on m-c, etc.): Yes ... STR outlined above
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch: none

tl;dr    This provides its stated fix, then requires bug 880592 to correct a small regression, then the combined two patches allow for bug 868222 to also be applied and address it's own user-visible issue.
Attachment #753761 - Flags: approval-mozilla-beta?
(In reply to Mark Capella [:capella] from comment #28)
> Risk to taking this patch (and alternatives if risky): 

Would you mind providing a risk evaluation here?
Flags: needinfo?(markcapella)
Same basic comment as bug 880592, and I'm still confused (and apologetic for) how I missed bugzilla mail with the needinfo request ... 

This and bug 880592 and bug 868222 (three as a package) are what I'd consider low risk in that they affect UI / displayed information only ... but, they prevent us (temporarily) displaying noticeably wrong information to users which of course is a good thing.

Changes are small and easily backed out, or more likely patched and moved forward in the event of further unseen regressions, though after baking for this long, I can't see that as a serious possibility.

The alternatives are limited, basically go without the fixes and expose wrong information to user scrutiny, or find another way of fixing the underlying problems in the current available time-frame.
Flags: needinfo?(markcapella)
Comment on attachment 753761 [details] [diff] [review]
Patch (v3)

Thanks for the update Mark, please go ahead with getting uplift - if you're not able to land to branches, mark 'checkin-needed' in the whiteboard.
Attachment #753761 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: checkin-needed
I can verify this is fixed in a Galaxy S2 phone with Android 4.0.3 and the latest Firefox Beta.
Whiteboard: [testday-20130726]
You need to log in before you can comment on or make changes to this bug.