932947 - [Australis] Mouse-down style for buttons in customization mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

Improve the mouse-down style for buttons in customization mode (AKA the blue box)

Comment 1

[:Gijs (he/him)]

P-'ing until we know what this should look like. Zhenshuo, did you have a design for what should happen instead?

Comment 2

[Zhenshuo Fang (:fang) - Firefox UX Team]

Putting this on shorlander's radar.

Comment 3

[Zhenshuo Fang (:fang) - Firefox UX Team]

Assigning it "P3" in alignment with other visual design needs

Comment 4

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

We should do a combination of things here:
- Get rid of the border/blue box in customization mode alltogether
- Slightly increase the scaling effect that happens when the user starts dragging an icon
- Don't make the icon transparent while dragging

Here's a quick demo:
http://phlsa.github.io/ff-customization-animation

All animation properties and timings are in the CSS

Comment 5

[u428464]

Is it intentional that the name of the widget disappears during dragging ?

Comment 6

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

(In reply to Guillaume C. [:ge3k0s] from comment #5)
> Is it intentional that the name of the widget disappears during dragging ?

Yes, that's intentional.
This makes the item look more »droppable« in the toolbar, because the size of the space and the size of the item being dragged match more closely.

Comment 7

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

That is of course assuming that every item in customization mode has some sort of visual representation/icon. If that is not the case, it's better to have the text visible at all times.

Comment 8

[:Gijs (he/him)]

Philipp, the mockup looks finished to my untrained eyes. Is my perception correct, or was there more you wanted to do here? And, did you want to implement this, or do you want one of us to try to take the mockup and make it happen for real? (you're still assigned, hence my asking)

(In reply to Philipp Sackl [:phlsa] from comment #7)
> That is of course assuming that every item in customization mode has some
> sort of visual representation/icon. If that is not the case, it's better to
> have the text visible at all times.

Unfortunately Addon-SDK-created widgets will only have text (empty space where the widget's content is, when used in the panel/palette). Are you sure you want to show the text always, though, or should we just do it only if there's no icon to show when dragging?

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #8)
> Philipp, the mockup looks finished to my untrained eyes. Is my perception
> correct, or was there more you wanted to do here? And, did you want to
> implement this, or do you want one of us to try to take the mockup and make
> it happen for real? (you're still assigned, hence my asking)

You are right, they are finished. Not clearing the assignment is just me still struggling with bugzilla.
 
> Unfortunately Addon-SDK-created widgets will only have text (empty space
> where the widget's content is, when used in the panel/palette). Are you sure
> you want to show the text always, though, or should we just do it only if
> there's no icon to show when dragging?

If we can make this conditional, that would be great!

Comment 10

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

And I did not answer your question: I think it is better if someone else takes care of the implementation. There's people more qualified for that :)

Comment 11

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

I'm someone more qualified!  ;)

Comment 12

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

Attachment: bug932947.patch

I basically copied the styles from Philipp's patch, and tested it on Mac.
I don't _think_ we have any tests that check CSS, so I didn't run them, but I'm happy to do a try-build if this seems reasonable.

Comment 13

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

Thanks for the patch and for sending me a build Blake! Here are a couple of issues:

- The animation itself looks great!
- The effect doesn't work on some icons (Bookmark button/menu, Email link, Cut/copy/paste, Zoom bar)
- As soon as the mouse is moved, the icon gets dimmed (which it shouldn't)
- The reordering animation that is in the current Nightly doesn't work anymore

Bonus points:
- Would it be possible to let the icons move into place when releasing the mouse instead of jumping there?

Comment 14

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

(The build is at https://people.mozilla.org/~bwinton/temp/firefox-29.0a1.en-US.mac64.dmg)

Replies:
1) Thanks!  ;)
2) The scaling effect doesn't work on those icons in regular Nightly, so I think we should file a new bug for that.
3) I'll definitely fix that.
4) Huh.  That's odd.  I'll need to look into that.

Bonus:
5) In theory, yes.  In practice, I'm not sure.

Comment 15

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) from comment #14)
> (The build is at
> https://people.mozilla.org/~bwinton/temp/firefox-29.0a1.en-US.mac64.dmg)
> 
> Replies:
> 1) Thanks!  ;)
> 2) The scaling effect doesn't work on those icons in regular Nightly, so I
> think we should file a new bug for that.
> 3) I'll definitely fix that.
> 4) Huh.  That's odd.  I'll need to look into that.
> 
> Bonus:
> 5) In theory, yes.  In practice, I'm not sure.

4 and 5 are to do with the fact that those animations use (transitioned) transforms, too. I... don't know how easy it is to adapt this patch to not mess with them. :-\

Comment 16

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

Attachment: bug932947.patch

Okay, so, here's the status.

3) As soon as the mouse is moved, the icon gets dimmed (which it shouldn't)
Still happens.  Surprisingly large pain in the ass to fix.

4) The reordering animation that is in the current Nightly doesn't work anymore
Fixed.

5) Would it be possible to let the icons move into place when releasing the mouse instead of jumping there?
In theory, yes, but in practice, this is related to #3, which I never quite got working.

Thanks,
Blake.

Comment 17

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

Comment on attachment 8359837 [details] [diff] [review]
bug932947.patch

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

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +71,2 @@
>  toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon {
> +  transition: transform .3s cubic-bezier(.6, 2, .75, 1.5);

What should happen here for [notransition]?

Comment 18

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

Attachment: bug932947.patch

For [notransition] we shouldn't transition.  Fixed!
Carrying forward ui-r=phlsa.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8360262 [details] [diff] [review]
bug932947.patch

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

LGTM apart from the nits below.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ -57,5 @@
>    background-attachment: scroll, scroll, fixed, fixed, scroll;
>  }
>  
> -toolbarpaletteitem {
> -  transition: background-color, border-color, box-shadow, border-width;

Please leave the border-width (and its duration+timing-function). It's used for the animation in the toolbar.

@@ +73,3 @@
>  }
>  
> +.panel-customization-placeholder,

Nit: please remove the placeholder selector for this transform.

Comment 20

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

Attachment: The final version of the patch.

Carrying forward r=Gijs and ui-r=phlsa.

Comment 21

[:Gijs (he/him)]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Carsten Book [:Tomcat]]

backed this fix out to see if this fix https://bugzilla.mozilla.org/show_bug.cgi?id=960389

Comment 24

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

Can someone do a push to try to see if this causes the problem?
The try message is: "try: -b o -p win32 -u mochitests[Windows XP] -t none"

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Green on backout.

BTW, for Try pushes, you need to manually enable PGO as well if you want it.
https://wiki.mozilla.org/ReleaseEngineering/TryChooser#What_if_I_want_PGO_for_my_build

Comment 26

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #25)
> Green on backout.
> 
> BTW, for Try pushes, you need to manually enable PGO as well if you want it.
> https://wiki.mozilla.org/ReleaseEngineering/
> TryChooser#What_if_I_want_PGO_for_my_build

It was backed out together with another patch, so there's no knowing which of these two it was. The other patch is way more suspect, seeing as it touched like all the tests.

Comment 27

[:Gijs (he/him)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=e6ec225174a3

Comment 28

[:Gijs (he/him)]

Re-landed given the try run:

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/356ef7c535f2

Comment 30

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed using latest Aurora (build ID: 20140307004002).