792806 - Missing smooth scroll animation when opening a new tab on an overflown tabstrip

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Not sure why nobody noticed this before but I was wondering why I sometimes don't see the tab open animation. After some bisecting it turned out to be caused by bug 696602.

To reproduce this, open a couple of tabs until the tabstrip overflows. Once you activate Panorama in this browsing session the tabstrip doesn't scroll smoothly to newly opened tabs but is immediately positioned.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Panorama calls gBrowser.showOnlyTheseTabs() a little too often. When a new tab is created it's also selected afterwards and that's where we call showOnlyTheseTabs() again.

Fixing UI.onTabSelect() is quite hard as that's one of the core paths of Panorama. It's way eaasier to make GroupItems._updateTabBar() a no-op in case there's really nothing to do. Everything else would need a mid-size refactoring, I tried some other options and all of them made lots of tests fail.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Looks good on try:

https://tbpl.mozilla.org/?tree=Try&rev=d488ac0da29e

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 663439 [details] [diff] [review]
patch v1

Why does updateActiveGroupItemAndTabBar call _updateTabBar when the group remained the same?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #3)
> Why does updateActiveGroupItemAndTabBar call _updateTabBar when the group
> remained the same?

I tried not doing that if the group doesn't change but the problem is that this makes a test fail because we re-use groupItems when switching back from private browsing mode. So technically it's still the same group but we need to re-hide tabs from other groups after we switched back.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > Why does updateActiveGroupItemAndTabBar call _updateTabBar when the group
> > remained the same?
> 
> I tried not doing that if the group doesn't change but the problem is that
> this makes a test fail because we re-use groupItems when switching back from
> private browsing mode. So technically it's still the same group but we need
> to re-hide tabs from other groups after we switched back.

Private browsing mode is gone by now. Can this be simplified now?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: update list of visible tabs only once after switching to another group

Thanks for reminding me of this one. This indeed makes it a little easier.

I put the check in _updateTabBar() (and not updateActiveGroupItemAndTabBar) because there are a couple of direct callers in ui.js.

We need to keep track of the last group item that caused a tab strip update because otherwise we would not update right after Panorama has been initialized. The active group in that case is set before _updateTabBar() was called.

With that change a couple of tests that move tabs around started to fail. There is no group change here it's quite clear that this can't work but I didn't see why we need to update the whole tab bar. We may just need to hide a single tab and that's it.

I removed the whole test for bug 624102 because that whole STR is invalid now with per-window PB.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 764031 [details] [diff] [review]
update list of visible tabs only once after switching to another group

>     } else {
>-      shouldUpdateTabBar = true
>+      shouldHideTab = true

nit: add semicolon

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Landed after try push was green:

https://hg.mozilla.org/integration/fx-team/rev/1230bd543454
https://hg.mozilla.org/integration/fx-team/rev/24e725b72a0d

The second changeset addresses the review comment about the semicolon. Forgot to include that, sorry.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1230bd543454
https://hg.mozilla.org/mozilla-central/rev/24e725b72a0d

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Backed out for causing bug 902979:

https://hg.mozilla.org/integration/fx-team/rev/68f142172838
https://hg.mozilla.org/integration/fx-team/rev/eab4756f594d

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Backout patch for Aurora and Beta

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 788004 [details] [diff] [review]
Backout patch for Aurora and Beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Um, this one.
User impact if declined: Bug 902979.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Panorama didn't change at all in the last weeks so a backout should be very safe.
String or IDL/UUID changes made by this patch: None.

I think we should back this patch out from Aurora and Beta. I think we can live with the issue reported here much more than with the issue reported in bug 902979.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 788004 [details] [diff] [review]
Backout patch for Aurora and Beta

Agree - backing this out is best, approved for uplift.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4fb1c635b52a

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 16

[Justin Dolske [:Dolske]]

Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.

If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.

See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.

We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.