Closed Bug 932947 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 29

People

(Reporter: zfang, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file, 3 obsolete files)

Improve the mouse-down style for buttons in customization mode (AKA the blue box)
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)
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.
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 :)
I'm someone more qualified!  ;)
Assignee: nobody → bwinton
Attached patch bug932947.patch (obsolete) — Splinter Review
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?
(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.
(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. :-\
Attached patch bug932947.patch (obsolete) — Splinter Review
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)
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-
Attached patch bug932947.patch (obsolete) — Splinter Review
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 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+
Carrying forward r=Gijs and ui-r=phlsa.
Attachment #8360262 - Attachment is obsolete: true
Attachment #8360307 - Flags: ui-review+
Attachment #8360307 - Flags: review+
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
Closed: 10 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 → ---
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
(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.
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
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
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
Depends on: 1102848
You need to log in before you can comment on or make changes to this bug.