Closed Bug 783328 Opened 7 years ago Closed 5 years ago

Improve tab opening animation

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: ttaubert, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Snappy:p1])

Attachments

(3 files, 2 obsolete files)

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.
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.
Attachment #652506 - Attachment description: remove the opacity transition when opening a tab → part 1 - remove the opacity transition when opening a tab
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.
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.
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.
Attachment #652506 - Flags: review?(dao)
Attachment #652512 - Flags: review?(dao)
Attachment #652517 - Flags: review?(dao)
Attachment #652519 - Flags: review?(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)?
(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 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?
(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.
(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.
(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.
Reverted thread.dispatch() to setTimeout(0).
Attachment #652519 - Attachment is obsolete: true
Attachment #652519 - Flags: review?(dao)
Attachment #652564 - Flags: review?(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.
Attachment #652517 - Flags: review?(dao) → review-
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.
Attachment #652512 - Flags: review?(dao) → review-
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
Attachment #652564 - Flags: review?(dao) → review-
(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 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.
Attachment #652506 - Flags: review?(dao) → review-
Whiteboard: [Snappy] → [Snappy:p2]
Whiteboard: [Snappy:p2] → [Snappy:p1]
Depends on: 844466
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.
Attachment #652564 - Attachment is obsolete: true
Assignee: ttaubert → nobody
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
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?
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.