750417 - closing a tab reflows the entire tab bar during the animation, making the animation crappy

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andreas Gal :gal]

STR: Enable paint flashing, open a few tabs and close the rightmost one. As the animation happens we repaint the entire tab bar as we do the "shrink tab" animation.

Comment 1

[Dão Gottwald [:dao]]

I doubt that front-end tabbrowser code can do anything about this...

Comment 2

[Andreas Gal :gal]

Diagnose it. You are doing something that causes a reflow. Probably changing the size and it's not absolute positioned. Once we know what causes it we can see if we can avoid triggering it or teach the platform to not redraw since the sizes aren't affected.

Comment 3

[Patrick Walton (:pcwalton)]

(In reply to Andreas Gal :gal from comment #2)
> Diagnose it. You are doing something that causes a reflow. Probably changing
> the size and it's not absolute positioned. Once we know what causes it we
> can see if we can avoid triggering it or teach the platform to not redraw
> since the sizes aren't affected.

I noticed this back in the Firefox 4 days. Off the top of my head: The fundamental problem is that we're using flexbox for our CSS transitions. If the animations were only on CSS transforms and nothing else, we could layerize the tab strip animations and avoid the repaint.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Gal :gal from comment #2)
> Once we know what causes it we
> can see if we can avoid triggering it or teach the platform to not redraw
> since the sizes aren't affected.

Pretty much any "we paint stuff that hasn't changed" bug should depend on DLBI. DLBI is very close to landing and even if it doesn't fix all those bugs, it makes them much easier to fix.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Patrick Walton (:pcwalton) from comment #3)
> I noticed this back in the Firefox 4 days. Off the top of my head: The
> fundamental problem is that we're using flexbox for our CSS transitions. If
> the animations were only on CSS transforms and nothing else, we could
> layerize the tab strip animations and avoid the repaint.

The current setup is not an oversight, it's the behavior we want. It makes a difference as soon as you close a tab that isn't the last one or as soon as you have enough tabs to reduce their width.

Comment 7

[Patrick Walton (:pcwalton)]

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Patrick Walton (:pcwalton) from comment #3)
> > I noticed this back in the Firefox 4 days. Off the top of my head: The
> > fundamental problem is that we're using flexbox for our CSS transitions. If
> > the animations were only on CSS transforms and nothing else, we could
> > layerize the tab strip animations and avoid the repaint.
> 
> The current setup is not an oversight, it's the behavior we want. It makes a
> difference as soon as you close a tab that isn't the last one or as soon as
> you have enough tabs to reduce their width.

Those could be done with CSS transforms as well, by making the middle part of each tab a separate piece that resizes via a width transform.

Comment 8

[Patrick Walton (:pcwalton)]

A scaleX transform, I mean.

Comment 9

[Dão Gottwald [:dao]]

CSS transforms can't get us the current behavior.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove calls to Redraw from nsSprocketLayout

DLBI should be able to handle these changes itself.

This reduces the invalidation considerably when closing a tab. I now only see changes to the last two tabs.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 671727 [details] [diff] [review]
Remove calls to Redraw from nsSprocketLayout

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

It looks OK to me, but who knows really? Let's do it! :-)

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Screen recording of paint flashing during tab closing

Sorry about the format, quicktime isn't much help here.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Invalidation log that matches the video: http://pastebin.mozilla.org/1868182

Comment 14

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/000af356f525

Comment 15

[Marco Castelluccio [:marco]]

Can this change be backported to Firefox 18? (after enough testing, I mean)

Comment 16

[Matt Woodrow (:mattwoodrow)]

I wasn't planning on requesting approval, since we don't really know what regressions this might cause.

However, if it's a significant performance improvement, then it might be worth considering.

We should probably try get input from the Snappy team in that case.

Comment 17

[(dormant account)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> I wasn't planning on requesting approval, since we don't really know what
> regressions this might cause.
> 
> However, if it's a significant performance improvement, then it might be
> worth considering.
> 
> We should probably try get input from the Snappy team in that case.

I'm waiting for this to land to see what the impact is.

Matt, can we do something similar to fix flicker in overflow animation(ie when you open and close tabs in tab overflow situation)?

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/000af356f525