Last Comment Bug 798753 - [OS X] Visual polish for social provider user menu
: [OS X] Visual polish for social provider user menu
Status: VERIFIED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All Mac OS X
: -- minor (vote)
: Firefox 19
Assigned To: Matthew N. [:MattN]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-06 00:50 PDT by Matthew N. [:MattN]
Modified: 2012-12-04 15:29 PST (History)
5 users (show)
MattN+bmo: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
Screenshot of proposed changes (144.20 KB, image/png)
2012-10-06 00:50 PDT, Matthew N. [:MattN]
no flags Details
WIP patch for OS X only (4.87 KB, patch)
2012-10-06 00:55 PDT, Matthew N. [:MattN]
no flags Details | Diff | Splinter Review
v.1 Use normal menuitem hover and separator for OS X + Linux (7.43 KB, patch)
2012-10-08 15:14 PDT, Matthew N. [:MattN]
jaws: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Matthew N. [:MattN] 2012-10-06 00:50:10 PDT
Created attachment 668746 [details]
Screenshot of proposed changes

1) There is no menu separator below my name and profile photo on the social provider menu.  This makes the menu look unusual since the menu items have different heights.  Adding a menu separator would align more with the native OS X user switcher (see attachment 665650 [details]).

2) The hover effect for the menu item with my name and photo does not align with platform or website conventions with the hyperlink text-decoration. IMO it should highlight the whole menu item background with no text-decoration (like it does for the other items). See the native OS X user switcher.

3) IMO the left edge of the profile photo should align with the left edge of the text in the menu items below i.e. leave a gutter on the left for the checkmarks. (see attachment 665650 [details]).
Comment 1 Matthew N. [:MattN] 2012-10-06 00:55:55 PDT
Created attachment 668748 [details] [diff] [review]
WIP patch for OS X only

This patch turned out well as it was mostly deleting lines. I still need to get the XUL changes to work with Windows/Linux.  Having the large menu item (before and after my patch) doesn't seem natural for Windows so I think it may be best to switch to normal <menuitem>s there.  Let me know if this is going against existing designs/requirements.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-10-06 02:57:16 PDT
Thanks Matt, this looks much better! Do you think we can we take this patch without affecting Windows/Linux?
Comment 3 Shane Caraveo (:mixedpuppy) 2012-10-06 15:33:44 PDT
(In reply to Matthew N. [:MattN] from comment #1)
> Having the large menu item
> (before and after my patch) doesn't seem natural for Windows so I think it
> may be best to switch to normal <menuitem>s there.  Let me know if this is
> going against existing designs/requirements.

It is the design that Boriss created, please touch base with her prior to making any changes on that.

Looks good, should be tried on win/lin.
Comment 4 Matthew N. [:MattN] 2012-10-08 15:14:32 PDT
Created attachment 669304 [details] [diff] [review]
v.1 Use normal menuitem hover and separator for OS X + Linux

Windows styling left the same. Linux and OS X work like normal menuitems and use a real separator. I would do the same for Windows but -moz-appearance: menuitem wouldn't show the XUL children and this bug was primarily for OS X styling.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-10-09 15:18:11 PDT
Comment on attachment 669304 [details] [diff] [review]
v.1 Use normal menuitem hover and separator for OS X + Linux

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

r=me with the one nit below addressed.

It's worth noting that this patch also fixes a small issue with keyboard navigation in this menu. On Windows, the username item previously could not be reached by pressing up & down on the keyboard while the menu was open with the mouse hovering over the Show Sidebar menuitem. With this patch, the username can now be reached.

::: browser/themes/pinstripe/browser.css
@@ +4094,5 @@
>  #social-statusarea-user-portrait {
>    width: 32px;
>    height: 32px;
> +  margin: 4px;
> +  margin-left: 0;

This would most likely need to be -moz-margin-start: 0;
Comment 7 Ed Morley [:emorley] 2012-10-11 07:05:59 PDT
https://hg.mozilla.org/mozilla-central/rev/38ca5a784d75
Comment 8 Matthew N. [:MattN] 2012-10-11 13:09:31 PDT
Comment on attachment 669304 [details] [diff] [review]
v.1 Use normal menuitem hover and separator for OS X + Linux

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: User menuitem will look non-native.
Testing completed (on m-c, etc.): m-c and locally on three platforms
Risk to taking this patch (and alternatives if risky): low risk styling glitches on the social provider menu. Social only.
String or UUID changes made by this patch: none
Comment 9 Alex Keybl [:akeybl] 2012-10-12 14:36:28 PDT
Comment on attachment 669304 [details] [diff] [review]
v.1 Use normal menuitem hover and separator for OS X + Linux

In support of our new feature.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 15:29:34 PST
Verified fixed across all branches.

Note You need to log in before you can comment on or make changes to this bug.