Closed Bug 908796 Opened 7 years ago Closed 6 years ago
Stop full tabstrip from re-painting on tab close
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. That's an inefficiency that's unique to UX - m-c doesn't do this at all. We should find out why, and then stop doing that. : http://screencast.com/t/mcaEPHgyR7 : http://screencast.com/t/KTl1bDcMTOqe
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.
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.
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
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.
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.
UX Baseline: https://tbpl.mozilla.org/?tree=Try&rev=3ad729c7e3fb with patch: https://tbpl.mozilla.org/?tree=Try&rev=aa31428662b6
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).
Attachment #796660 - Flags: review?(mnoorenberghe+bmo)
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 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.
Attachment #796660 - Flags: review?(mnoorenberghe+bmo) → review+
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.
Status: ASSIGNED → NEW
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M9][Australis:P1][fixed-in-ux]
Is this related and/or will this workaround fix bug 904889 ?
Not sure but that bug was near the top of my list to investigate for TART so I'll find out soon.
Status: NEW → ASSIGNED
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.
Whiteboard: [Australis:M9][Australis:P1][fixed-in-ux] → [Australis:M?][Australis:P3]
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M?][Australis:P3][leave open]
6 years ago
12:33 PM <mconley> jaws: yeah, I thought this affected OS X, but it doesn't. Let's close it.
Whiteboard: [Australis:M?][Australis:P3][leave open] → [Australis:M9][Australis:P3][fixed-in-ux]
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...
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Whiteboard: [Australis:M9][Australis:P3][fixed-in-ux] → [Australis:M-][Australis:P-]
You need to log in before you can comment on or make changes to this bug.