894048 - RemoveAllTabsBut should use {animate: true} when closing tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 887515 tried to set the {animate: true} option when closing tabs for the RemoveAllTabsBut function but that caused test issues with browser_bug577121.js.

WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug577121.js | there are two remaining tabs open - Got 3, expected 2
WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug577121.js | tab1 stayed open - Got [object XULElement], expected [object XULElement]
WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug577121.js | Found an unexpected tab at the end of test run: about:blank

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Why?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> Why?

Because animations in our user interface help are generally understood to reduce the utilitarian feel of software. We should be including animations where possible. All other times that the user closes a tab we show an animation, I think the alternate question to ask is "why wouldn't we?"

Comment 4

[:Gavin Sharp [email: [e-mail]]]

I don't quite recall the original motivation for this choice, but they may have been:
- animations hurt responsiveness in the closing-many-tabs case
- animations don't look good when many tabs are closing at once

Have we confirmed these aren't issues anymore?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

I haven't seen any unexpected animation issues when running with this (Close Tabs To The Right uses {animate:true}). Of course, this bug doesn't remove any jank that we have if you close tons of tabs, but I don't think it necessarily makes that case worse.

What this patch does do though is make the case of closing 2 or 3 tabs nicer. So in that, I see it as a net win.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] from comment #5)
> I haven't seen any unexpected animation issues when running with this (Close
> Tabs To The Right uses {animate:true}). Of course, this bug doesn't remove
> any jank that we have if you close tons of tabs, but I don't think it
> necessarily makes that case worse.

Running animations during tab closing is certainly a net increase in workload. Whether that is significant enough/perceptible is worth measuring. Do we wait for one tab's animation to end before triggering the next tab close, or are they all animating in parallel?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

They are all animating in parallel.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 792315 [details] [diff] [review]
Patch

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

I personally think this looks much nicer than not animating. Also as far as I understand it, animating multiple tabs shouldn't have a huge overhead compared to animating a single tab. We reflow the whole document per animation step anyway and calculating sizes and position for a few tabs more does probably not make a big difference?

I think it would be good to wait with landing this patch until Gavin had a chance to intervene :)

Comment 9

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> I understand it, animating multiple tabs shouldn't have a huge overhead
> compared to animating a single tab. We reflow the whole document per
> animation step anyway and calculating sizes and position for a few tabs more
> does probably not make a big difference?

Yes. Also, for historic reasons, we only animate three tabs at a time, the remaining ones will be removed immediately.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> I think it would be good to wait with landing this patch until Gavin had a
> chance to intervene :)

I haven't heard anything, so I went ahead and pushed it:
https://hg.mozilla.org/integration/fx-team/rev/d7f20a38e66e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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