964262 - Fade in icons in customization mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Currently, the items that are not yet in the panel or the toolbars appear once the transition into customization mode is finished.
We should fade them in instead for ~0.3s (lengthening the overall transition time if that is necessary) in order to make the entire transition look smoother.

Comment 1

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

Attachment: The first version of the patch…

Comment 2

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

Comment on attachment 8384680 [details] [diff] [review]
The first version of the patch…

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +258,5 @@
>        this._customizing = true;
>        this._transitioning = false;
>  
>        // Show the palette now that the transition has finished.
> +      this.visiblePalette.setAttribute("showing", "true");

Drive-by - I think this is going to hurt our CART results.

I think we should still apply hidden=false/hidden=true to the palette. Apply hidden=false, and then set the showing attribute, and then *maybe* we'll want to have a transitionend listener on the palette, and yield until that transition completes.

Same for the exit transition - remove the showing attribute, yield until the transition completes, and then hide the palette for better performance.

We get better performance because hidden removes the palette from the flow of the document, meaning layout doesn't have to worry about it anymore.

Comment 3

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

Comment on attachment 8384680 [details] [diff] [review]
The first version of the patch…

mconley's feedback is good, looking forward to a revised patch :)

Comment 4

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

Attachment: The second version of the patch.

Let's try this instead.  :)

(I'm not using requestAnimationFrame because that doesn't give the dom enough time to be updated, and so the opacity doesn't transition.)

Comment 5

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

Attachment: Patch

Mike, what do you think about this? I couldn't get Blake's patch to work for me.

Comment 6

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

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

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

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +177,5 @@
> +  transition: opacity .3s ease-in-out 1s,
> +              visibility 0s linear;
> +}
> +
> +#main-window[customize-entered] #customization-palette {

The customize-entered attribute will be set on #main-window as soon as the CSS transition is completed, but _not_ before we populate the palette. I wonder if this is why you're requiring the longer transition time.

Perhaps instead of using the customize-entered attribute on the #main-window, perhaps we should select based on an attribute on the #customization-palette, which gets set after the palette is populated? And removed when depopulated?

Also, what did phlsa think of this?

Comment 7

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

phlsa gave it a ui-r+.

And I'll work on a new patch that adds a new attribute…

Comment 8

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

Attachment: The fourth(?) version of the patch. ;)

(Re-requesting ui-review from Philip, just to make sure this still works the way he wants.  Also to give him a chance to check it out on Jared's Windows machine.  ;)

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8385369 [details] [diff] [review]
The fourth(?) version of the patch.  ;)

ui-r+, assuming that this fixes the issues of the animation not triggering on Windows under certain circumstances.

Comment 10

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

Comment on attachment 8385369 [details] [diff] [review]
The fourth(?) version of the patch.  ;)

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

Perfect, this works great. Thanks :)

Comment 11

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

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/89011a82f5f4

Comment 13

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

Comment on attachment 8385369 [details] [diff] [review]
The fourth(?) version of the patch.  ;)

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

Australis!


User impact if declined: 

A less pleasant customize mode transition.


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

Lots of baking on m-c.


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

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 14

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/614541a85200

Comment 15

[:Gijs (he/him)]

(huh, I only just noticed that this having my name on is a lie... but oh well :-\ )