908796 - Stop full tabstrip from re-painting on tab close

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

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

Not sure if this is the right component, but I'll just leave this here for now.

Dao noticed in bug 908067 comment 1 that with paint flashing on, the tabstrip is fully repainting on tab close.[1]

That's an inefficiency that's unique to UX - m-c doesn't do this at all.[2]

We should find out why, and then stop doing that.

[1]: http://screencast.com/t/mcaEPHgyR7
[2]: http://screencast.com/t/KTl1bDcMTOqe

Comment 1

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

I hypothesize that this redraw is related to the spike :avih and I were seeing only on tab close TART numbers. That would make sense, since the timing of that spike seemed independent of the transition time, and I think this repaint is the same way.

Comment 2

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

Attachment: no relative.png

So I've tracked this down to a position: relative CSS rule set on the #nav-bar.

If I remove that rule, the tabstrip doesn't need to repaint itself, apparently.

The position: relative is used in conjunction with z-index: 1 to ensure that the nav-bar's top pixel isn't overlapped by the background tab's bottom pixel.

The attachment shows how z-index and position: relative are used with the default theme.

Comment 3

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

Hey Matt,

BenWa suggested you might be able to help us find out what's going wrong here. Do you know why the whole tabstrip needs to get re-drawn if the nav-bar is higher up in the stacking context?

-Mike

Comment 4

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

A workaround is to alter the tab-background-start/-middle/-end graphics so as to not overlap the nav-bar border.

This is a little tricky because the nav-bar border is different with lw-themes (the inner-highlight constitutes the border, whereas the border constitutes the border when not using lw-themes). Not sure if that's a strangeness we want to fix. If not, and we want to alter the graphics, then we'll need two types - a set for lw-themes, and a set for no lw-themes.

Comment 5

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

Attachment: Workaround

I talked to shorlander, and he said he's willing to take this regression in tab polish for the potential boost in performance (I'll throw up a baseline and patch try push in a second).

He said he'd try fiddling with the bottom row of pixels in a follow-up, which I'll file if/after this patch lands.

Alternatively, if I can reduce this down to a minimal test-case, :tn has expressed interest in figuring out why the repaint is happening.

Anyhow, let's wait for the try results to tell us if this is a win, and if so, I'll request review.

Comment 6

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

UX Baseline:

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

with patch:

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

Comment 7

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

Comment on attachment 796660 [details] [diff] [review]
Workaround

According to http://compare-talos.mattn.ca/breakdown.html?oldTestIds=28910933,28913881,28913903,28915387,28915393,28915425,28915441,28915521,28915531&newTestIds=28911007,28913869,28913897,28915371,28915377,28915447,28915471,28915477,28915487&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org, this gives us a little bit of a win on the tab close animation measurements for Windows XP.

Steven said he was cool with this workaround - though if we land it, I'm going to file a follow-up to have him tweak the bottom 2 pixels of tab-background-start/-middle-end to make the layering not look so crazy. (Alternatively, we can rope in layout and find out why this workaround is even necessary - assuming we can create a minimal test-case).

Comment 8

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

Here's m-c vs UX without the patch on Windows XP:

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=28914099,28916763,28916771,28916821,28916829,28916835&newTestIds=28910933,28913881,28913903,28915387,28915393,28915425,28915441,28915521,28915531&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

and here's m-c vs UX with the patch on Windows XP:

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=28914099,28916763,28916771,28916821,28916829,28916835&newTestIds=28911007,28913869,28913897,28915371,28915377,28915447,28915471,28915477,28915487&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

Seems to take a bit of a bite out of the regression.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 796660 [details] [diff] [review]
Workaround

I'm fine with this for now but I agree that we should file a follow-up or two for polish and/or investigation on the cause. I confirmed on Windows 7 that the repaint of the tabstrip on tab close is gone with this patch.

Comment 10

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

Cool, workaround landed on UX as:

https://hg.mozilla.org/projects/ux/rev/58fc4af0f9b9

I'll file a separate bug for the pixel fiddling for Steven, and another to back out this workaround and figure out why we're re-painting.

Comment 11

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

Filed bug 910987 for the pixels and bug 910987 to back out the workaround.

Comment 12

[:Gijs (he/him)]

Is this related and/or will this workaround fix bug 904889 ?

Comment 13

[Matthew N. [:MattN]]

Not sure but that bug was near the top of my list to investigate for TART so I'll find out soon.

Comment 14

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

I just noticed that this affects OS X as well. Not sure about Linux GTK, but we should check that out. OS X and Linux GTK TART performance is, however, lower priority at this time. So removing fixed-in-ux, and lowering priority.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

12:33 PM <mconley> jaws: yeah, I thought this affected OS X, but it doesn't. Let's close it.

Comment 18

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

I think we can back this workaround out, since we're not displaying the closing tab anymore. Local testing confirms that we don't re-paint the tabstrip with this workaround backed out.

Standby for backout patch...

Comment 19

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

Backed out as https://hg.mozilla.org/projects/ux/rev/bf390551be03.

Resolving as WORKSFORME, because as far as I can tell, the bug is gone.

Let's just keep an eye on those Windows TART numbers to be completely sure.