783328 - Improve tab opening animation

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

This bug is not about any underlying issues like 2d accel or similar. This is about the animation itself - I think we can do a couple of improvements here and I'll post some patches soon.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - remove the opacity transition when opening a tab

Bug 604735 removed artifacts when closing a tab but also introduced an opacity transition when opening a new tab.

First I don't think we actually need this as there have no artifacts been reported when opening and second (while there's nothing wrong with the transition itself when viewed in slowmo mode) this creates a little flicker when seeing the tab open animation at normal speed.

It's a rather subtle difference but I think it's very visible when you quickly open and close a few tabs in succession and compare this to a Nightly build. The flicker is gone and the tab slides a little smoother onto the tabstrip.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - hide overflow of tab contents

With [part 1] applied, you see that the tab's contents overflow the tab itself at the start of the transition. We should hide this overflow because that's clearly stuff we don't have to render at all unless the tab is wide enough.

Another thing this solves is that tab contents (like image and label) do not shift around anymore as the tab grows wider but they're rather always aligned to the left and more of them is revealed as the tab opening animation progresses. This should reduce visual distraction even more.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 3 - don't crop tab label unless tab is fully opened

Carrying on the thoughts from [part 2] we should also not crop the tab label while we're transitioning. This should make the transition easier to render (as we're just clipping text that doesn't fit instead of putting an ellipsis at the end) and it should look a little calmer without a blinking ellipsis.

This also causes the tab title to reveal parts of letters as the transition progresses - not only whole letters at once (which is how 'crop' works). That should result in a smoother animation as well.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 4 - don't start the animation off a timeout

The animation is currently started using setTimeout(0). I don't think we should do this as the delay is rather unpredictable, especially if there's a CC/GC scheduled or the like.

When we have a tab open animation we can just insert the tab into DOM and set the fadein attribute to let it start ASAP. When there's no animation we can use thread.dispatch() to call _handleNewTab() so that it's the selected one if we opened it in the foreground.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 652519 [details] [diff] [review]
part 4 - don't start the animation off a timeout

>+              Services.tm.currentThread.dispatch(function () {
>+                this.tabContainer._handleNewTab(t);
>+              }.bind(this), Ci.nsIThread.DISPATCH_NORMAL);

Do you have proof that this is somehow better than setTimeout(0)?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #5)
> >+              Services.tm.currentThread.dispatch(function () {
> >+                this.tabContainer._handleNewTab(t);
> >+              }.bind(this), Ci.nsIThread.DISPATCH_NORMAL);
> 
> Do you have proof that this is somehow better than setTimeout(0)?

No, I don't. We should ask someone more familiar with this about what's the difference. AFAIK dispatch() isn't clamped to a minimum of 4ms. Maybe setTimeout() in chrome windows isn't either?

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 652506 [details] [diff] [review]
part 1 - remove the opacity transition when opening a tab

Pinstripe overrides the transition. Did you intentionally keep the opacity there? Also, did you intentionally remove the opacity for the opening transition but not for the closing transition?

Comment 8

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > >+              Services.tm.currentThread.dispatch(function () {
> > >+                this.tabContainer._handleNewTab(t);
> > >+              }.bind(this), Ci.nsIThread.DISPATCH_NORMAL);
> > 
> > Do you have proof that this is somehow better than setTimeout(0)?
> 
> No, I don't. We should ask someone more familiar with this about what's the
> difference. AFAIK dispatch() isn't clamped to a minimum of 4ms. Maybe
> setTimeout() in chrome windows isn't either?

It shouldn't be clamped, see bug 512645.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #8)
> It shouldn't be clamped, see bug 512645.

Ok, then we should probably change it back to setTimeout(0) here.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #7)
> Pinstripe overrides the transition. Did you intentionally keep the opacity
> there? Also, did you intentionally remove the opacity for the opening
> transition but not for the closing transition?

Pinstripe only overrides the closing transition. I have another patch for the closing transition that should improve it as well and includes removing the opacity transition. I wanted this bug to be only about the tab opening animation.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 4 - don't start the animation off a timeout

Reverted thread.dispatch() to setTimeout(0).

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 652517 [details] [diff] [review]
part 3 - don't crop tab label unless tab is fully opened

>       <method name="_handleNewTab">
>         <parameter name="tab"/>
>         <body><![CDATA[
>           if (tab.parentNode != this)
>             return;
>           tab._fullyOpen = true;
>+          tab.setAttribute("crop", "end");

crop could already be set to something else by now, via setTabTitle.

Other code such as _setMenuitemAttributes also copies the crop attribute from tabs, and it seems like we don't want "none" there.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 652512 [details] [diff] [review]
part 2 - hide overflow of tab contents

This creates an extra scroll frame for each tab -- doesn't seem like it's going to be a perf win.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 652564 [details] [diff] [review]
part 4 - don't start the animation off a timeout

>-            // When overflowing, new tabs are scrolled into view smoothly, which
>-            // doesn't go well together with the width transition. So we skip the
>-            // transition in that case.
>-            if (aSkipAnimation ||
>+             // When overflowing, new tabs are scrolled into view smoothly, which
>+             // doesn't go well together with the width transition. So we skip the
>+             // transition in that case.

messed up the indentation here

>+            if (t.pinned ||
>+                aSkipAnimation ||
>                 this.tabContainer.getAttribute("overflow") == "true" ||
>                 !Services.prefs.getBoolPref("browser.tabs.animate")) {
>-              t.setAttribute("fadein", "true");
>+
>+              if (!t.pinned) {
>+                t.setAttribute("fadein", "true");
>+              }

the tab can't be pinned at this point

Comment 15

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> Created attachment 652506 [details] [diff] [review]
> part 1 - remove the opacity transition when opening a tab
> 
> Bug 604735 removed artifacts when closing a tab but also introduced an
> opacity transition when opening a new tab.
> 
> First I don't think we actually need this as there have no artifacts been
> reported when opening

Why would the artifacts exist at the end of the closing transition but not at the beginning of the opening transition?

> and second (while there's nothing wrong with the
> transition itself when viewed in slowmo mode) this creates a little flicker
> when seeing the tab open animation at normal speed.

What kind of flicker? I don't think I'm seeing it.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 652506 [details] [diff] [review]
part 1 - remove the opacity transition when opening a tab

Per comment 15, it's not clear to my how and why this would improve the animation overall.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 652564 [details] [diff] [review]
part 4 - don't start the animation off a timeout

Something like part 4 landed in bug 844466.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Can we please not close this? I still think the tabopen animation looks quite broken and we should do a better job. I just won't have the time to work on that in the near future. Maybe Australis will fix some of this?

Comment 19

[Dão Gottwald [:dao]]

This bug had no clear description of what exactly should be improved. We've repeatedly tweaked the tab opening animation and it looks good to me, so I'm going to close this again.