814165 - Closing last tab reanimates tab counter

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently when one closes the last remaining open tab and closes the tab-tray, the tab counter will re-animate (for the single tab open; about:home).

Considering a tab-count of one, is it necessary to re-animate the counter in this instance?

I expected to see no animation; akin to just opening and closing the tab menu with a single tab open (just seeing the '1', appear minus any animation).

--
Nightly (11/21); Samsung Galaxy Nexus (Android 4.2)

Comment 1

[Aaron Train [:aaronmt]]

Re steps above, mistake, it is not necessary to open the tab tray as it's already open. Just close the tab tray after closing the last tab.

Comment 2

[Lucas Rocha (:lucasr)]

I'd say we should not animate the tab counter when you remove tabs. it should just have the new number of tabs when you close the tabs tray.

Comment 3

[Sriram Ramasubramanian [:sriram]]

What exactly should the condition be?
if "tablet && side bar shown"
    then animate
else
    don't animate
??

This would fail for tablets for last-tab case.

If "side bar shown"
   animate
else
   don't

This would fail for tablets, again.

When closing the last tab, the count goes to 2 and comes back to 1, causing the animation.

Comment 5

[Aaron Train [:aaronmt]]

Closing the last tab animates from 2 > 1; makes no sense.

Tracking for 19 as this is on Aurora?

Comment 6

[Lucas Rocha (:lucasr)]

Ian, what's your take here?

Comment 7

[crazyskeggy]

You shouldn't be able to remove the last tab, can you? (You can't do it in Firefox Release, so it's either a bug or a new feature)

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to crazyskeggy from comment #7)
> You shouldn't be able to remove the last tab, can you? (You can't do it in
> Firefox Release, so it's either a bug or a new feature)

Recent new feature (see bug 786982).

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #2)
> I'd say we should not animate the tab counter when you remove tabs. it
> should just have the new number of tabs when you close the tabs tray.
> Ian, what's your take here?

Agreed.

Comment 10

[Aaron Train [:aaronmt]]

Still an issue

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Remove closed tab before selecting new tab

Sorry for the long comment, but it's not easy to tell how anything is fixed since the toolbar updates are hidden away in callbacks...

When the last tab is closed, the following order of events happen:

1) nextTab is null (since we're closing the last tab), so create a new about:home tab
  1a) This fires TabEvents.ADDED
  1b) Which calls updateTabCountAndAnimate(), animating from 1->2 [1]
2) Select the new about:home tab
  2a) This fires TabEvents.SELECTED, which calls refresh()
  2b) Which calls updateTabCount() (without animating, keeping the count at 2) [2]
3) Remove the old tab from the tab list
4) Destroy the old tab
  3a) This fires TabEvents.CLOSED
  3b) Which calls updateTabCountAndAnimate(), animating from 2->1 [1]

We add an extra tab before removing the old one, thus resulting in the 2->1 animation we see when the last tab is closed. If we reorder these events, we can end up with this:

1) Remove the old tab from the tab list
2) nextTab is null (since we're closing the last tab), so create a new about:home tab
  1a) This fires TabEvents.ADDED
  1b) Which calls updateTabCountAndAnimate() (but this will do nothing since the old tab count == new tab count == 1)
3) Select the new about:home tab
  2a) This fires TabEvents.SELECTED, which calls refresh()
  2b) Which calls updateTabCount() (without animating, keeping the count at 1)
4) Destroy the old tab
  3a) This fires TabEvents.CLOSED
  3b) Which calls updateTabCountAndAnimate() (but this will again do nothing)

Since we remove the last tab before adding a new one, this prevents us from getting in the state where we have two tabs when closing the last tab.

But this change also means the closing a tab will never animate, even when closing a tab with tab count > 1. Following the chain of events listed above (but skipping #2 since it won't apply here), we end up calling refresh() *after* we remove a tab, which updates the internal tab count without doing an animation.

So the second part of this fix is to prevent doing an updateTabCount() inside of refresh(). Since BrowserToolbar's internal mCount won't be updated until tab's onDestroy(), this will trigger an animation. I think the only reason we needed to have an updateTabCount() inside of refresh() to begin with is to handle orientation changes. This will still be handled if we call updateTabCount() in refreshChrome() instead of the more generic refresh().

[1] http://hg.mozilla.org/mozilla-central/file/07e17dd7813b/mobile/android/base/BrowserToolbar.java#l484
[2] http://hg.mozilla.org/mozilla-central/file/07e17dd7813b/mobile/android/base/BrowserToolbar.java#l1339

Comment 12

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e019562a29e4

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e019562a29e4