967220 - If about:customizing is the first tab, the tabstrip looks detached from the nav-bar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

STR:

1) Open a browser window with a single tab
2) Browse to about:customizing in that single tab

ER:

A customize mode transition that's gloriously beautiful and smooth.

AR:

A clear disconnect between the tabstrip and the nav-bar.

See screenshot: http://i.imgur.com/0hPVkrn.png

This mostly affects Windows and Linux, and only affects OS X if browser.tabs.drawInTitlebar is set to false.

This is fallout from bug 962677.

Comment 1

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

Any opinions on what we might do in this case, Blake?

My first thought is to try snapping in the margins of the tabstrip without animating it, as per one of our original ideas, but that might look too jumpy.

We could also sneak in *just* the selected tab the 2em margin, if it's the first selected tab... but that might be inconsistent and weird.

I'm open to other suggestions.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Lemme think on it, and get back to you tonight or tomorrow…

Comment 4

[:Gijs (he/him)]

This doesn't just happen if it's the first tab, judging by 966680.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: temp.diff

So, this is untested on Mac, but something like this on Windows seems okay to me.  What do you think?

Comment 6

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

Comment on attachment 8370227 [details] [diff] [review]
temp.diff

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

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +12,5 @@
>    margin-left: 2em;
>    margin-right: 2em;
>  }
>  
> +#main-window[customizing] #navigator-toolbox > #TabsToolbar > #tabbrowser-tabs:not([overflow="true"]) {

I think the idea is right, in that it will not add too too much in the way of layout overhead when kicking off the transition.

I don't think it's right to only do this if we're not overflowing. Another way to get into this glitchy state is if about:customizing is an app tab - then we'd want this adjustment even if we're overflowing.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8370227 [details] [diff] [review]
temp.diff

This seems wrong when #TabsToolbar contains other items besides #tabbrowser-tabs.

Comment 8

[:Gijs (he/him)]

So, why did we not add the margin on the tabstoolbar here in the first place? (ie, what's with the :not(#TabsToolbar) in the selector we're using)

Is the tabstrip responsible for all the jank? If so, have we looked at why that is and can we fix that some other way, so that we can just have the tabstoolbar transition the same way it does now?

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #8)
> Is the tabstrip responsible for all the jank? If so, have we looked at why
> that is and can we fix that some other way, so that we can just have the
> tabstoolbar transition the same way it does now?

Animating the margins (or padding) of the tabstrip was causing a pretty big slowdown, yes - and it was proportional to the number of tabs in the tabstrip. It was a primary contributor to the customize mode transition jank on OS X.

I tried a number of alternative solutions - primarily, hiding things in the tab strip during the transition, but this didn't really make much difference.

Comment 10

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

One thing we should test is to see how much animating the margins on Windows slows down the transition. If it's negligible, we could put it back there.

Comment 11

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

I'm going to see what can be done about this today.

Comment 12

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

According to a reflow profile on Windows 7, not animating the TabsToolbar only saves us somewhere in the neighbourhood of 10ms throughout th ewhole transition - which is miniscule, considering the amount of time we're spending reflowing the menu panel.

Two possibilities, assuming this reflow profiler is accurate

1) Not animating the tabstrip had a negligible performance impact in bug 962677, and it was other changes in that patch that made things get smoother on OS X.
2) Animating the tab strip is cheaper on Windows

In either case, this suggests that at least on Windows, we might be able to animate the tabstrip again. I'll see what gains we got from not animating the tab strip on OS X next.

Comment 13

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

Reflow profiles on OS X suggest that not animating the tabstrip only saves us in the neighbourhood of ~5ms.

It's starting to look like the real savings from bug 962677 came from something else - perhaps not animating the whole tab-view-deck, and just animating each individual toolbar and content-deck accounts for it.

In either case, I think I'm starting to get convinced that we can animate the tabs again, eliminate this glitch, and pick up more savings elsewhere.

Comment 14

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

Measuring the animation intervals for both OS X and Windows, it looks like not animating the tab strip buys us something around 1 frame.

So not animating the tab strip *did* buy us some performance, but not a whole lot of it.

I'm going to see what CART says next, and then settle on a decision.

Comment 15

[:Gijs (he/him)]

Attachment: include the tabstoolbar in things animating (with a margin),

Comment 16

[:Gijs (he/him)]

Attachment: include the tabstoolbar in things animating (with a margin),

Tweaks for Linux. Tested on Windows, OS X and Linux now. This fixes the bug and also fixes the bottom border of the toolbox to stay within the bounds of the shrunk area in customize mode, by using the same CSS as Windows (we should really unify those sometime...)

Comment 17

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

Comment on attachment 8372683 [details] [diff] [review]
include the tabstoolbar in things animating (with a margin),

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

Ok, yep, let's take this. I expect a small CART blip, but I figure we can win that back with some other things.

Comment 18

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/6cdfa68aeeb4

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6cdfa68aeeb4

Comment 20

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

Comment on attachment 8372683 [details] [diff] [review]
include the tabstoolbar in things animating (with a margin),

I expect this is something we want on Aurora.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 962677 and Australis.


User impact if declined: 

Users with app tabs, or who visit about:customizing with the first tab experience a strange disconnect between the tab strip and the nav-bar (see screenshot in comment 0).


Testing completed (on m-c, etc.): 

Landed on m-c several days ago, and works as expected.


Risk to taking this patch (and alternatives if risky): 

Might regress CART talos test slightly, but this will (hopefully) be made up with changes from bug 962657.


String or IDL/UUID changes made by this patch:

None.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1acebd4c8912

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed using latest Nightly (build ID: 20140310030201) and latest Aurora (build ID: 20140310004003) on Windows 7 32bit, Ubuntu 12.04 and Mac OS X 10.9