Closed Bug 792806 Opened 7 years ago Closed 4 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(firefox24 wontfix, firefox25 wontfix)

RESOLVED WONTFIX
Firefox 24
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix

People

(Reporter: ttaubert, Unassigned)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #663439 - Flags: review?(dao)
Comment on attachment 663439 [details] [diff] [review]
patch v1

Why does updateActiveGroupItemAndTabBar call _updateTabBar when the group remained the same?
(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.
(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?
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.
Attachment #663439 - Attachment is obsolete: true
Attachment #663439 - Flags: review?(dao)
Attachment #764031 - Flags: review?(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
Attachment #764031 - Flags: review?(dao) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/1230bd543454
https://hg.mozilla.org/mozilla-central/rev/24e725b72a0d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Depends on: 902979
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.
Attachment #788004 - Flags: approval-mozilla-beta?
Attachment #788004 - Flags: approval-mozilla-aurora?
Comment on attachment 788004 [details] [diff] [review]
Backout patch for Aurora and Beta

Agree - backing this out is best, approved for uplift.
Attachment #788004 - Flags: approval-mozilla-beta?
Attachment #788004 - Flags: approval-mozilla-beta+
Attachment #788004 - Flags: approval-mozilla-aurora?
Attachment #788004 - Flags: approval-mozilla-aurora+
Assignee: ttaubert → nobody
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago4 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.