933926 - Hide customization palette during transition phase

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Reflowing all of the junk inside the palette and panelholder isn't necessary during the 150ms transition.

Initial measurements suggest this could buy us a frame for both entering and exiting customization mode.

Comment 1

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

Attachment: Patch

I'm not sure how to test that this increases perf, but it is plausible that it will make the transition smoother.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8348523 [details] [diff] [review]
Patch

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

Hmm. This does make it noticeably smoother for me. But we're going to need to do some extra work if you're going to use this, because that extra smoothness doesn't actually feel like a net improvement right now. 

The most obvious issue is that the width of the panelWrapper collapses when panelHolder is hidden - it should stay the same width.

Also, just suddenly hiding all the content while smoothly transitioning all the supporting UI feels odd/unfinished/rough. But IMO, at least fixing the width of the panelWrapper would make this bug a net improvement.

One thing we could do is draw the palette and the panel to <canvas>s, and display that when we transition out. When transitioning into customization mode, we could do a quick opacity transition of the (live) contents, once the zooming transition has completed.

Comment 3

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

Attachment: Patch v1.1

I played with adding the opacity transition but it made it seem slower to me, almost like there is lag with painting the palette. This patch fixes the width of the panel holder when exiting though, which does make it look a bit better.

Comment 4

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

Attachment: Patch v1.2

The 'checked' getter and setter won't exist if the element is display:none since the binding is detached, so the code now just checks the attribute directly instead of relying on the getter/setter.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8348964 [details] [diff] [review]
Patch v1.2

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

What about transitioning *in* to customize mode?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #1)
> I'm not sure how to test that this increases perf, but it is plausible that
> it will make the transition smoother.

We need an easy/relatively-generic way to measure animation perf for arbitrary animations like this one. Can we re-purpose infrastructure from TART to apply more broadly? Mark's been thinking about related issues for panel animations.

Comment 7

[Avi Halachmi (:avih)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #6)
> We need an easy/relatively-generic way to measure animation perf for
> arbitrary animations like this one. Can we re-purpose infrastructure from
> TART to apply more broadly? Mark's been thinking about related issues for
> panel animations.

Sure. We could start by adding it as an optional test to TART such that devs could test iteratively while developing right now, and if needed, later make it a stand alone one, possibly adding it to talos if you think we should.

For the animations which we want to test, I'd need to know how to trigger each, and the event which indicates the animation's done. If needed, I could possibly also generalize it by accepting custom trigger functions without rebuilding the addon.

Let me know how you want to take this forward.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8348964 [details] [diff] [review]
Patch v1.2

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +548,5 @@
>        wrapper.setAttribute("itemobserves", aNode.getAttribute("observes"));
>        aNode.removeAttribute("observes");
>      }
>  
> +    if (aNode.hasAttribute("checked")) {

This should check getAttribute("checked") == "true", shouldn't it?

Comment 9

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

Many of these places would also need to be updated if we were to support [checked=false]. http://mxr.mozilla.org/mozilla-central/search?string=[checked]

Comment 10

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

But I will make that change, it is more correct.

Comment 11

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

(In reply to :Gijs Kruitbosch (Unavailable Dec 19 - Jan 2) from comment #8)
> Comment on attachment 8348964 [details] [diff] [review]
> Patch v1.2
> 
> Review of attachment 8348964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +548,5 @@
> >        wrapper.setAttribute("itemobserves", aNode.getAttribute("observes"));
> >        aNode.removeAttribute("observes");
> >      }
> >  
> > +    if (aNode.hasAttribute("checked")) {
> 
> This should check getAttribute("checked") == "true", shouldn't it?

Fwiw, boolean attributes in HTML use hasAttribute too.

Comment 12

[:Gijs (he/him)]

(In reply to :Ms2ger from comment #11)
> (In reply to :Gijs Kruitbosch (Unavailable Dec 19 - Jan 2) from comment #8)
> > Comment on attachment 8348964 [details] [diff] [review]
> > Patch v1.2
> > 
> > Review of attachment 8348964 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/components/customizableui/src/CustomizeMode.jsm
> > @@ +548,5 @@
> > >        wrapper.setAttribute("itemobserves", aNode.getAttribute("observes"));
> > >        aNode.removeAttribute("observes");
> > >      }
> > >  
> > > +    if (aNode.hasAttribute("checked")) {
> > 
> > This should check getAttribute("checked") == "true", shouldn't it?
> 
> Fwiw, boolean attributes in HTML use hasAttribute too.

But XUL ones do not. See also e.g. bug 947410 where a bug was caused by our code setting collapsed="false" instead of removeAttribute...

Comment 13

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

Attachment: Patch v1.3

This patch only hides the palette during the transition. It's simpler for now, and the items in the panel holder won't rearrange as the transition is occurring (as opposed to the palette items).

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8355345 [details] [diff] [review]
Patch v1.3

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

Hmm, ok. This is a good first step, but I think we still need followup bugs that should be blockers (with appropriate measurements to back that up), that continue improving on this.

Comment 15

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

Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/b4b87fcc8f8f

I filed bug 956388 to add a TART-like test for entering/exiting customization mode.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4b87fcc8f8f