962677 - Use content-deck and toolbar margins for the customize mode transition

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

During the customize mode transition, the tabs toolbar is getting squeezed smaller from the tab-view-deck's extra padding.

With many tabs, this can get pretty expensive - according to a reflow profile, the TabsToolbar accounts for about 672ms, which is massive. That's not just reflow - that's also painting.

A few ideas:

1) Find a way of not shrinking the tabs toolbar. This would be a pretty big win.
2) Hide some of the more expensive elements in the tabs during the transition - the labels, for example.
3) Somehow hide the tab strip during the shrink, and show it when the shrink is done. This would probably be pretty jarring, but it's an idea.
4) Snap the tab strip to the shrunk / unshrunk dimensions in one frame, as opposed to animating it. Again, jarring.

Comment 1

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

Attachment: A patch that does some stuff. (Sorry, it was just a crazy idea that kinda works.)

Comment 2

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

Comment on attachment 8363897 [details] [diff] [review]
A patch that does some stuff.  (Sorry, it was just a crazy idea that kinda works.)

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

I think this idea is definitely worth pursuing - the difference is quite apparent with this patch.

As mentioned earlier, CustomizeMode.jsm will need to be updated to listen for the transition being completed on a different element - and on margin instead of padding.

Comment 3

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

Attachment: A better version of the patch.

Okay, I think this version is worth profiling.
I double-checked the removal of background-noise-toolbar.png with Madhava before making that change, and he was okay with it.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8363910 [details] [diff] [review]
A better version of the patch.

>+toolbar:not(#TabsToolbar),

#navigator-toolbox > toolbar:not(#TabsToolbar)

Comment 5

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

Comment on attachment 8363910 [details] [diff] [review]
A better version of the patch.

I really think this is the right idea - we want to not move the TabsToolbar or nav-bar during the transition. I think this patch still does it - I'm still seeing repaints / reflows being registered for both the TabsToolbar and nav-bar in a reflow profile. Eyeballing it, I can see both moving a little bit too.

display:none'ing both elements takes a huge chunk of the performance problem away.

So I think we should try to tune this patch so that neither toolbar needs to get repainted / reflowed during the "entering" portion of the transition.

Comment 6

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

Well, let me correct myself - the current patch intentionally shrinks the nav-bar. That might be unavoidable for the effect we're looking for.

Still, eliminating all need to repaint / reflow the TabsToolbar during the transition would be a huge win.

Comment 7

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

Like I said, I think this is a good first step. I'm going to try de-glitch-ifying this patch.

Comment 8

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

Attachment: WIP Patch v1

I've started poking at this. Haven't tried it on Windows or Linux yet.

Comment 9

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

Attachment: WIP Patch v1.1

Comment 10

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

Attachment: WIP Patch v1.2

Comment 11

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

A note that with an opt build, this patch seems to really improve performance on OS X.

I'm going to see how it looks / works on Windows next.

Comment 12

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

Attachment: Patch v1

Ok, I've tested this on OS X Mountain Lion, Windows XP (Classic and Luna), Windows 7 (Classic and Aero), Windows 8 (Classic and... normal kind) and Ubuntu Linux.

The performance improvement here is mostly detectable on OS X, at least from my testing. We'll want to focus on Windows next.

Comment 13

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

Comment on attachment 8367974 [details] [diff] [review]
Patch v1

What do you think of this, jaws?

Comment 14

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

Comment on attachment 8367974 [details] [diff] [review]
Patch v1

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

Please make sure to test this with the #developer-toolbar open.

::: browser/base/content/browser.css
@@ +887,5 @@
>    -moz-image-region: auto;
>  }
>  
>  /* Customize mode */
> +toolbar:not(#TabsToolbar),

In an earlier comment, Dao asked this to be changed to `#navigator-toolbox > toolbar:not(#TabsToolbar)` otherwise this rule will apply to toolbars such as the #developer-toolbar.

@@ +894,5 @@
>    transition-duration: 150ms;
>    transition-timing-function: ease-out;
>  }
>  
> +#tab-view-deck[fastcustomizeanimation] toolbar:not(#TabsToolbar),

#tab-view-deck[fastcustomizeanimation] #navigator-toolbox > toolbar:not(#TabsToolbar),

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +425,5 @@
>          return;
>        }
>        this.window.clearTimeout(catchAllTimeout);
> +      // Bug 962677: We let the event loop breathe for 10ms before we do the final
> +      // stage of the transition to improve perceived performance.

I'm not sure why 10ms is better than 0ms. This is intentionally dropping a frame.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* Customization mode */
> +
> +#main-window:-moz-any([customize-entering], [customize-entered]) #content-deck {

By convention, we haven't been putting spaces after commas when inside of a :any() function (this ruleset and the ruleset below it).

Comment 15

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

Attachment: Patch v1.1 (r+'d by jaws)

(In reply to Jared Wein [:jaws] from comment #14)
> Comment on attachment 8367974 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8367974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make sure to test this with the #developer-toolbar open.

I did, and after I applied your correction, it seemed to behave properly (in that it's not erroneously margin'd when we enter customize mode).

> 
> ::: browser/base/content/browser.css
> @@ +887,5 @@
> >    -moz-image-region: auto;
> >  }
> >  
> >  /* Customize mode */
> > +toolbar:not(#TabsToolbar),
> 
> In an earlier comment, Dao asked this to be changed to `#navigator-toolbox >
> toolbar:not(#TabsToolbar)` otherwise this rule will apply to toolbars such
> as the #developer-toolbar.
> 

Good catch! Missed that in the shuffle.

> @@ +894,5 @@
> >    transition-duration: 150ms;
> >    transition-timing-function: ease-out;
> >  }
> >  
> > +#tab-view-deck[fastcustomizeanimation] toolbar:not(#TabsToolbar),
> 
> #tab-view-deck[fastcustomizeanimation] #navigator-toolbox >
> toolbar:not(#TabsToolbar),
> 

Done.

> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +425,5 @@
> >          return;
> >        }
> >        this.window.clearTimeout(catchAllTimeout);
> > +      // Bug 962677: We let the event loop breathe for 10ms before we do the final
> > +      // stage of the transition to improve perceived performance.
> 
> I'm not sure why 10ms is better than 0ms. This is intentionally dropping a
> frame.
> 

Yeah, moving this down to 0 didn't seem to have a detrimental effect. 0 it is.

> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  /* Customization mode */
> > +
> > +#main-window:-moz-any([customize-entering], [customize-entered]) #content-deck {
> 
> By convention, we haven't been putting spaces after commas when inside of a
> :any() function (this ruleset and the ruleset below it).

Done.

Thanks jaws!

Comment 16

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

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/b1d99c879787

https://tbpl.mozilla.org/php/getParsedLog.php?id=33823177&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=33823224&tree=Fx-Team

Comment 18

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

This test does not fail when run on its own, but it seems to be related to the browser_876926_customize_mode_wrapping.js test... with that one disabled, all tests pass.

Comment 19

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

Ok, I've figured this one out.

browser_934113_menubar_removable.js tests attempting to move the menubar items to the nav-bar (which should not be possible by default).

It turns out browser_934113_menubar_removable.js never really did the right thing on Linux (and probably Windows 7+), since the menubar toolbar is collapsed by default on these platforms. Attempting to simulate dragging from something that's not visible on the screen has, as you'd expect, unexpected consequences.

Another piece ot the puzzle is that browser_876926_customize_mode_wrapping.js adds history to the browser, meaning that the customize mode tab opens in a new tab.

So for some reason, with the patch I've got here applied, attempting to drag the (invisible) menu item to the nav-bar causes us to switch to the about:blank tab from the customize mode tab, which rightly kicks us out of customize mode. Thus, when browser_934113_menubar_removable.js attempts to exit customize mode, it goes "I'm not in customize mode", and we're done.

And the test fails on exit because the customize tab still exists in the background.

The solution I have here is to force the menubar toolbar to be displayed, thus avoiding the undefined behaviour when attempting to drag an invisible item.

Patch coming up.

Comment 20

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

Attachment: Test fix

This oughta do it - all customizableui tests pass for me locally with this.

Comment 21

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

Attachment: Patch v1.2 (r+'d by jaws, test-fix r+'d by Gijs)

Otay, let's try this again.

Comment 22

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

Re-landed folded patch:

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[u428464]

Attachment: Tabs overlapping in customization mode

Is this by design or a bug ?

Comment 25

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #24)
> Created attachment 8369118 [details]
> Tabs overlapping in customization mode
> 
> Is this by design or a bug ?

That's a bug. Please file, block australis-cust, and CC mconley and me. STR plus slighly more detail (is that maximized mode with tabsintitlebar turned on? Can't tell with the small screenshot...) would be lovely. Thank you! :-)