Last Comment Bug 750417 - closing a tab reflows the entire tab bar during the animation, making the animation crappy
: closing a tab reflows the entire tab bar during the animation, making the ani...
Status: RESOLVED FIXED
[snappy:p1]
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal with 5 votes (vote)
: mozilla19
Assigned To: Matt Woodrow (:mattwoodrow)
:
: Jet Villegas (:jet)
Mentors:
: 750495 (view as bug list)
Depends on: dlbi 803556
Blocks: 593680
  Show dependency treegraph
 
Reported: 2012-04-30 12:41 PDT by Andreas Gal :gal
Modified: 2012-10-19 20:55 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove calls to Redraw from nsSprocketLayout (4.88 KB, patch)
2012-10-15 22:15 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Screen recording of paint flashing during tab closing (496.35 KB, video/x-m4v)
2012-10-15 23:19 PDT, Matt Woodrow (:mattwoodrow)
no flags Details

Description Andreas Gal :gal 2012-04-30 12:41:53 PDT
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] 2012-04-30 12:46:42 PDT
I doubt that front-end tabbrowser code can do anything about this...
Comment 2 Andreas Gal :gal 2012-04-30 15:15:39 PDT
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) 2012-04-30 16:07:54 PDT
(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 4 (dormant account) 2012-04-30 17:11:49 PDT
*** Bug 750495 has been marked as a duplicate of this bug. ***
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-30 23:21:10 PDT
(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] 2012-05-01 03:22:13 PDT
(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) 2012-05-01 07:36:26 PDT
(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) 2012-05-01 07:36:43 PDT
A scaleX transform, I mean.
Comment 9 Dão Gottwald [:dao] 2012-05-01 08:40:21 PDT
CSS transforms can't get us the current behavior.
Comment 10 Matt Woodrow (:mattwoodrow) 2012-10-15 22:15:51 PDT
Created attachment 671727 [details] [diff] [review]
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) 2012-10-15 22:22:29 PDT
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) 2012-10-15 23:19:48 PDT
Created attachment 671735 [details]
Screen recording of paint flashing during tab closing

Sorry about the format, quicktime isn't much help here.
Comment 13 Matt Woodrow (:mattwoodrow) 2012-10-15 23:20:09 PDT
Invalidation log that matches the video: http://pastebin.mozilla.org/1868182
Comment 14 Matt Woodrow (:mattwoodrow) 2012-10-16 14:32:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/000af356f525
Comment 15 Marco Castelluccio [:marco] 2012-10-16 14:35:36 PDT
Can this change be backported to Firefox 18? (after enough testing, I mean)
Comment 16 Matt Woodrow (:mattwoodrow) 2012-10-16 14:43:59 PDT
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) 2012-10-16 15:03:26 PDT
(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)?

Note You need to log in before you can comment on or make changes to this bug.