916735 - Add a pref to disable animation when entering/exiting customization mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Running the CustomizableUI tests is needlessly slow at the moment, thanks to the animation for entering/exiting customization mode. Lets have a pref to disable that, and save some hours of machine time.

Also, likely useful for some people on low-spec machines or remote desktop connections/thin clients.

Comment 1

[:Gijs (he/him)]

Agreed. Let's also do this before we merge...

Comment 2

[:Gijs (he/him)]

Hmm. But can we actually do this? Of course, we do want to keep checking our animation (end detecting) stuff keeps working, not introduce non-automatic-test-only failures in our code. :-(

Comment 3

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

Instead of canceling the transitions outright, we could force the durations to be very, very quick if some attribute is applied - we could read the pref at the beginning of CustomizeMode.enter, and add the attribute...removing it on exit.

Just an idea.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Mike Conley (:mconley) from comment #3)
> Instead of canceling the transitions outright, we could force the durations
> to be very, very quick if some attribute is applied - we could read the pref
> at the beginning of CustomizeMode.enter, and add the attribute...removing it
> on exit.
> 
> Just an idea.

http://gifrific.com/wp-content/uploads/2013/04/Mr-Burns-Saying-Excellent.gif

Comment 5

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > Instead of canceling the transitions outright, we could force the durations
> > to be very, very quick if some attribute is applied - we could read the pref
> > at the beginning of CustomizeMode.enter, and add the attribute...removing it
> > on exit.
> > 
> > Just an idea.
> 
> http://gifrific.com/wp-content/uploads/2013/04/Mr-Burns-Saying-Excellent.gif

This is what I thought. But trying this with the test for bug 880382, setting the transition-duration to 1ms, I can reliably get the runtime down from 17s to 14s. If I just nuke all the start/endCustomizing calls, I get it down to 7s. It's not just the animation that's slow here, especially not on debug builds where 150ms is very little in terms of wallclock time.

So, all in all, not sure how much changing this will give us here.

Comment 6

[:Gijs (he/him)]

Attachment: use a pref to fast-forward animation for tests

Of course, maybe my patch sucks and I'm missing something obvious. This is what I tried.

Comment 7

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

(In reply to :Gijs Kruitbosch from comment #6)
> Created attachment 806236 [details] [diff] [review]
> use a pref to fast-forward animation for tests
> 
> Of course, maybe my patch sucks and I'm missing something obvious. This is
> what I tried.

What happens at 0ms? Note that the wrapping and unwrapping of widgets takes some time...

Comment 8

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > Created attachment 806236 [details] [diff] [review]
> > use a pref to fast-forward animation for tests
> > 
> > Of course, maybe my patch sucks and I'm missing something obvious. This is
> > what I tried.
> 
> What happens at 0ms? Note that the wrapping and unwrapping of widgets takes
> some time...

At 0ms the events don't fire and we don't actually go into customize mode properly (whee).

:-(

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #5)
> This is what I thought. But trying this with the test for bug 880382,
> setting the transition-duration to 1ms, I can reliably get the runtime down
> from 17s to 14s.

Runtime of our tests went down by 30% for me. Were only going to be adding more and more tests, so that seems like a win - even if it's not at big as I'd hoped. And this is useful beyond just tests.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Does setting transition-property:none still result in the events being fired?

Comment 11

[:Gijs (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #10)
> Does setting transition-property:none still result in the events being fired?

No, then the events no longer fire. Also, is it just me or should these transitions be in the browser/base CSS files rather than in the browser/themes/shared bits? Otherwise themes can break customization mode if they 'forget' to specify a transition...

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #11)
> Also, is it just me or should these
> transitions be in the browser/base CSS files rather than in the
> browser/themes/shared bits? Otherwise themes can break customization mode if
> they 'forget' to specify a transition...

Hm, yea.

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/2cdad19e044f

Filed bug 918782 for the shared theme stuff to figure out exactly what needs moving and so on.

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/2cdad19e044f