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

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Firefox 29
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P-])

Attachments

(2 attachments, 8 obsolete attachments)

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 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.
Attachment #8363897 - Flags: feedback?(mconley) → feedback+
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.
Attachment #8363897 - Attachment is obsolete: true
Attachment #8363910 - Flags: feedback?(mconley)
Comment on attachment 8363910 [details] [diff] [review]
A better version of the patch.

>+toolbar:not(#TabsToolbar),

#navigator-toolbox > toolbar:not(#TabsToolbar)
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.
Attachment #8363910 - Flags: feedback?(mconley) → feedback+
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.
Assignee

Updated

5 years ago
Summary: Find a solution to expensive TabsToolbar reflows and paints → Use content-deck and toolbar margins for the customize mode transition
Like I said, I think this is a good first step. I'm going to try de-glitch-ifying this patch.
Posted patch WIP Patch v1 (obsolete) — Splinter Review
I've started poking at this. Haven't tried it on Windows or Linux yet.
Assignee: nobody → mconley
Attachment #8363910 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Posted patch WIP Patch v1.1 (obsolete) — Splinter Review
Attachment #8366063 - Attachment is obsolete: true
Posted patch WIP Patch v1.2 (obsolete) — Splinter Review
Attachment #8367567 - Attachment is obsolete: true
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.
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8367627 - Attachment is obsolete: true
Comment on attachment 8367974 [details] [diff] [review]
Patch v1

What do you think of this, jaws?
Attachment #8367974 - Flags: review?(jaws)
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).
Attachment #8367974 - Flags: review?(jaws) → review+
Posted patch Patch v1.1 (r+'d by jaws) (obsolete) — Splinter Review
(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!
Attachment #8367974 - Attachment is obsolete: true
Attachment #8367988 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/da8776bc458e
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-fx-team]
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.
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.
Posted patch Test fix (obsolete) — Splinter Review
This oughta do it - all customizableui tests pass for me locally with this.
Attachment #8368603 - Flags: review?(gijskruitbosch+bugs)

Updated

5 years ago
Attachment #8368603 - Flags: review?(gijskruitbosch+bugs) → review+
Otay, let's try this again.
Attachment #8367988 - Attachment is obsolete: true
Attachment #8368603 - Attachment is obsolete: true
Attachment #8368612 - Flags: review+
Re-landed folded patch:

remote:   https://hg.mozilla.org/integration/fx-team/rev/6f8ada6cbee5
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6f8ada6cbee5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P-][fixed-in-fx-team] → [Australis:P-]
Target Milestone: --- → Firefox 29

Comment 24

5 years ago
Is this by design or a bug ?

Comment 25

5 years ago
(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! :-)
Assignee

Updated

5 years ago
Depends on: 967220

Updated

5 years ago
Depends on: 978068
You need to log in before you can comment on or make changes to this bug.