940262 - Australis tabopen animation looks twitchy because it starts 15px above the previous tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

When opening a new tab it is transitioned by min- and max-width. The .tab-background however is immediately visible with its -15px margin and makes the new tab jump back a little before sliding forward again.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Slow-motion video of current tabopen animation

Here's a slow-motion video of the current tabopen animation. I added a border to visualize the tab bounds and slowed down the transitions. You can see that the new tab starts with its rounded borders immediately visible above the previous tab.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-fade-in-.tab-background-to-improve-tabopen-animation.patch

Here's a proposed patch that fades in .tab-background and makes the tabopen animation IMO look a lot smoother.

Not sure who to ask for feedback on this. Feel free to redirect.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Slow-motion video of improved tabopen animation

Here's the same slow-motion video with the improved tabopen animation to clarify the change. With the patch applied (in non-slow-mo mode) opening a new tab looks better to me now.

Comment 4

[Girish Sharma [:Optimizer]]

A really slow mo screencast : http://www.youtube.com/watch?v=6cw4ki2V58w

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 8334419 [details] [diff] [review]
0001-fade-in-.tab-background-to-improve-tabopen-animation.patch

I'm happy to give you an f+ for this, although r+ should probably come from mconley or MattN. However, we should really run this through try and check if this regresses TART. :-)

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-940262-Fade-in-.tab-background-to-improve-tabope.patch

Applying the transition to selected tabs, only.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Let's see what TART says:

https://tbpl.mozilla.org/?tree=Try&rev=b3d38ee38099

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8334453 [details] [diff] [review]
0001-Bug-940262-Fade-in-.tab-background-to-improve-tabope.patch

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css
>@@ -114,16 +114,25 @@ tabbrowser {
> .tabbrowser-tab:not([pinned]):not([fadein]) {
>   max-width: 0.1px;
>   min-width: 0.1px;
>   visibility: hidden;
>   transition: min-width 200ms ease-out,
>               max-width 230ms ease-out;
> }
> 
>+.tabbrowser-tab[selected]:not([pinned]) > .tab-stack > .tab-background {
>+  opacity: 0;
>+}
>+
>+.tabbrowser-tab[selected][fadein]:not([pinned]) > .tab-stack > .tab-background {
>+  opacity: 1;
>+  transition: opacity 180ms ease-out 20ms;
>+}

tab-background inherits [selected] and [pinned]. It doesn't inherit [fadein], but it could.

You should set opacity:0 in a rule using :not([fadein]) and avoid setting opacity:1 at all.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Looks like a regression to me, somewhat expected due to more work when fading in...

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=4bf55c6b9c25&newRev=b3d38ee38099&submit=true

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

This patch removes the fadein, after a short offset .tab-background is just shown - i.e. switched from visibility:hidden to visible. This looks probably even better? And should not regress TART? Let's see.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

FTR, here are try builds for the previous patch, if anyone wants to try it out:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b3d38ee38099/

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Try builds for the latest patch are here:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6843671acee4/

(Some platforms are still building.)

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

While OS X 10.7 is still running it pretty sure looks like the new patch doesn't regress anything:

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=9225b9430a51&newRev=6843671acee4&submit=true

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8335161 [details] [diff] [review]
0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

Not exactly sure that those are the changes that dao had in mind but I figured I just ask for review.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8335161 [details] [diff] [review]
0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

>+.tab-background {
>+  transition: visibility 1ms ease-out 20ms;
>+}

Are the delay and duration correct for both the opening and the closing transition? Or does it not matter because closing tabs are invisible anyway? Can you document this with a comment?

>+.tabbrowser-tab[selected]:not([fadein]):not([pinned]) > .tab-stack > .tab-background {
>+  visibility: hidden;
>+}

You should update the tab binding such that tab-background inherits all the necessary attributes and change the selector to .tab-background[selected]:not([fadein]):not([pinned]).

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #15)
> >+.tab-background {
> >+  transition: visibility 1ms ease-out 20ms;
> >+}
> 
> Are the delay and duration correct for both the opening and the closing
> transition? Or does it not matter because closing tabs are invisible anyway?
> Can you document this with a comment?

What you said. Will add a comment.

> >+.tabbrowser-tab[selected]:not([fadein]):not([pinned]) > .tab-stack > .tab-background {
> >+  visibility: hidden;
> >+}
> 
> You should update the tab binding such that tab-background inherits all the
> necessary attributes and change the selector to
> .tab-background[selected]:not([fadein]):not([pinned]).

Ah, thanks. That's better.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8335161 [details] [diff] [review]
0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

This does indeed look more smooth. This looks fine, but I'll wait for the next patch with Dao's issues fixed before giving a full-on review.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

Made the suggested changes. I would have liked to do the same for the .tab-close-button because it looks a little weird that this is now also sliding from left to right but it's not as easy due to it being display:none.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8335310 [details] [diff] [review]
0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

>+.tab-background {
>+  /**
>+   * We have to explicitly specify 'visible' as the default visibility or we

This really doesn't make sense to me... the initial state should already be visibility:visible.

>+   * run into weird painting issues with the initial tab on startup.
>+   */
>+  visibility: visible;

define "weird"? I didn't notice anything over here.

>+  /**
>+   * This transition is only applied when opening a new tab. Closing tabs
>+   * are just hidden so we don't need to adjust the offset for that.
>+   */

s/offset/delay/

>+  transition: visibility 1ms ease-out 30ms;

- you can use 0ms
- you can omit ease-out, because it doesn't matter at all
- 25ms worked fine over here (while 20ms didn't) so I'd prefer that over 30ms, assuming that the latter might be closer to hiding another frame that shouldn't be hidden

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #19)
> >+.tab-background {
> >+  /**
> >+   * We have to explicitly specify 'visible' as the default visibility or we
> 
> This really doesn't make sense to me... the initial state should already be
> visibility:visible.

Doesn't make sense to me either...

> >+   * run into weird painting issues with the initial tab on startup.
> >+   */
> >+  visibility: visible;
> 
> define "weird"? I didn't notice anything over here.

At least on OS X I see that if I bump the transition delay to 3000ms, on startup the .tab-background of the initial tab is hidden. After 3s it's transitioned to visible and pushes the whole tabbar down by one pixel, causing this: http://cl.ly/image/1Z2i102d0O2W

Another report of a Windows user showed this: http://i.snag.gy/1vbLB.jpg The whole titlebar seems to be pushed down by some pixels here.

AFAIK, switching from visibility:hidden to visible shouldn't be able to push anything anywhere but I can reliably reproduce exactly that.

> >+  transition: visibility 1ms ease-out 30ms;
> 
> - you can use 0ms
> - you can omit ease-out, because it doesn't matter at all
> - 25ms worked fine over here (while 20ms didn't) so I'd prefer that over
> 30ms, assuming that the latter might be closer to hiding another frame that
> shouldn't be hidden

Ok, sure. I increased it from 20ms because of the close button.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> > - 25ms worked fine over here (while 20ms didn't) so I'd prefer that over
> > 30ms, assuming that the latter might be closer to hiding another frame that
> > shouldn't be hidden
> 
> Ok, sure. I increased it from 20ms because of the close button.

I can't say for sure that this is a reliable method but if you increase the max/min-width transition durations and the delay by 100x, 25ms seems not enough as the background is shown before the tab is wide enough and thus still makes it slide from left to right for a bit.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> At least on OS X I see that if I bump the transition delay to 3000ms, on
> startup the .tab-background of the initial tab is hidden. After 3s it's
> transitioned to visible and pushes the whole tabbar down by one pixel,
> causing this: http://cl.ly/image/1Z2i102d0O2W
> 
> Another report of a Windows user showed this: http://i.snag.gy/1vbLB.jpg The
> whole titlebar seems to be pushed down by some pixels here.

Would be good if we could create a reduced testcase and file a Layout bug on it.

(In reply to Tim Taubert [:ttaubert] from comment #21)
> I can't say for sure that this is a reliable method but if you increase the
> max/min-width transition durations and the delay by 100x, 25ms seems not
> enough as the background is shown before the tab is wide enough and thus
> still makes it slide from left to right for a bit.

Assuming 60hz and an ideal frame rate, 25ms would be roughly in the middle between two frames, so presumably a good way to ensure hitting one frame but not the subsequent one. But I could imagine the style system rounding the delay up to include the subsequent frame. Either way, you don't have to worry about this when slowing the transition down like you did, so I don't think that's a directly useful test.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #22)
> Would be good if we could create a reduced testcase and file a Layout bug on
> it.

Yeah, I fear that might be a difficult task.

> Assuming 60hz and an ideal frame rate, 25ms would be roughly in the middle
> between two frames, so presumably a good way to ensure hitting one frame but
> not the subsequent one. But I could imagine the style system rounding the
> delay up to include the subsequent frame. Either way, you don't have to
> worry about this when slowing the transition down like you did, so I don't
> think that's a directly useful test.

Fair enough.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

It seems that .tab-background inherits its visibility from the #TabsToolbar which is collapsed=true (visibility:collapsed) at startup. That would explain why the TabsInTitlebar calculations fail and why we're transitioning. Maximizing/restoring the window fixes the misaligned tab for me.

Explicitly setting visibility:visible would thus override the inherited value and prevent the transition at startup. It would also immediately show .tab-background and thus lead to correct measurements.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 8335658 [details] [diff] [review]
0001-Bug-940262-Hide-.tab-background-in-the-early-tabopen.patch

>+  /**
>+   * Explicitly set the visibility to override the value (collapsed)
>+   * we inherit from #TabsToolbar[collapsed] at startup.
>+   */

I think you actually mean the window opening rather than application startup.

nit: please use this more compact comment style here:

  /* foo bar
   * foo bar. */

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #26)
> >+  /**
> >+   * Explicitly set the visibility to override the value (collapsed)
> >+   * we inherit from #TabsToolbar[collapsed] at startup.
> >+   */
> 
> I think you actually mean the window opening rather than application startup.

Yes, thanks. That's what I wanted to say.

> nit: please use this more compact comment style here:
> 
>   /* foo bar
>    * foo bar. */

Will do.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/cd6b40046e85

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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