RemoveAllTabsBut should use {animate: true} when closing tabs

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 26
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Why?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?"
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?
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.
(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?
They are all animating in parallel.
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 :)
Attachment #792315 - Flags: review?(ttaubert) → review+
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/d7f20a38e66e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.