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

VERIFIED FIXED in Firefox 29

Status

()

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: zfang, Assigned: bwinton)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 1

5 years ago
P-'ing until we know what this should look like. Zhenshuo, did you have a design for what should happen instead?
Flags: needinfo?(zfang)
Whiteboard: [Australis:M?] [Australis:P?] → [Australis:M?] [Australis:P-]
Putting this on shorlander's radar.
Flags: needinfo?(shorlander)
Assigning it "P3" in alignment with other visual design needs
Whiteboard: [Australis:M?] [Australis:P-] → [Australis:M?] [Australis:P3]
Assignee: nobody → psackl
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
Flags: needinfo?(zfang)
Flags: needinfo?(shorlander)

Comment 5

5 years ago
Is it intentional that the name of the widget disappears during dragging ?
(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.
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

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

Comment 11

5 years ago
I'm someone more qualified!  ;)
Assignee: nobody → bwinton
(Assignee)

Comment 12

5 years ago
Created attachment 8357973 [details] [diff] [review]
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.
Attachment #8357973 - Flags: review?(jaws)
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?
(Assignee)

Comment 14

5 years ago
(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

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

Comment 16

5 years ago
Created attachment 8359837 [details] [diff] [review]
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.
Attachment #8357973 - Attachment is obsolete: true
Attachment #8357973 - Flags: review?(jaws)
Attachment #8359837 - Flags: ui-review?
Attachment #8359837 - Flags: review?(jaws)
(Assignee)

Updated

5 years ago
Attachment #8359837 - Flags: ui-review? → ui-review?(philipp)
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]?
Attachment #8359837 - Flags: review?(jaws) → review-
(Assignee)

Comment 18

5 years ago
Created attachment 8360262 [details] [diff] [review]
bug932947.patch

For [notransition] we shouldn't transition.  Fixed!
Carrying forward ui-r=phlsa.
Attachment #8359837 - Attachment is obsolete: true
Attachment #8360262 - Flags: ui-review+
Attachment #8360262 - Flags: review?(jaws)

Comment 19

5 years ago
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.
Attachment #8360262 - Flags: review?(jaws) → review+
(Assignee)

Comment 20

5 years ago
Created attachment 8360307 [details] [diff] [review]
The final version of the patch.

Carrying forward r=Gijs and ui-r=phlsa.
Attachment #8360262 - Attachment is obsolete: true
Attachment #8360307 - Flags: ui-review+
Attachment #8360307 - Flags: review+

Comment 21

5 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/dad295e99240
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?] [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dad295e99240
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 29
backed this fix out to see if this fix https://bugzilla.mozilla.org/show_bug.cgi?id=960389
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 24

5 years ago
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"
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

5 years ago
(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 28

5 years ago
Re-landed given the try run:

remote:   https://hg.mozilla.org/integration/fx-team/rev/356ef7c535f2
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/356ef7c535f2
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]

Updated

5 years ago
QA Contact: cornel.ionce
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).
Status: RESOLVED → VERIFIED

Updated

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