344570 - Make contextual menus look more native

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Mark Mentovai]

I'm rolling up a few fixes to improve the appearance and behavior of contextual menus on the Mac.  Some of the changes also apply to drop-down menus, and some of the changes apply to all pop-up windows.  All of these changes make certain XUL interface elements look much more like their native counterparts.

Comment 1

[Mark Mentovai]

Attachment: Patch v1

The size of menu items in contextual menus and drop-down menus was wrong.  The OS standard is to use kThemeMenuItemFont, Lucida Grande 13, but we were using kThemeSystemFont, Lucida Grande 12.  This is fixed by requesting the -moz-pull-down-menu font which maps to eSystemFont_PullDownMenu and uses the correct constant from above.  Note that it IS correct OS behavior for the current item shown in a collapsed pop-up menu to be displayed at 12 points (kThemeSystemFont) before being popped up, but once popped up, the menu items themselves should be displayed at 13 points.

With that out of the way, the theme needed some positioning tweaks to ensure that the heights and spacings of menu items would be correct, and that menu item text would be displayed at the appropriate position within the frame.  The menus were also too wide, so tweaks were applied to bring them into conformance with the native appearance.  New icons will be attached in a separate zip file: they move the existing icons down two pixels to match the new positioning and text size.

Disabled menu items were displayed with the wrong color.  This was fixed by introducing -moz-mac-menutextdisable which corresponds to kThemeTextColorMenuItemDisabled, and using that color for disabled menu items.  We had been using GrayText, kThemeTextColorDialogInactive, which was too dark.

There's also a fix in here to forbid keyboard navigation to disabled menu items.  Josh and I hit that last week.

Now, finally, we get to the real interesting changes.  I'm making all popup windows translucent, with an alpha value of 0.95.  The native equivalents of everything that we use popups for have very gentle transparencies: contextual menus, pop-up menus, autocomplete drop-down menus.  This feature is implemented with a single call and is handled entirely by the window server.

The native equivalents of all of these things except for autocomplete drop-downs also fade out quickly when closing.  I've included changes here to implement this behavior for us whenever hiding any pop-up window.  This feature will only function on Mac OS X 10.3 or later, but I've included the necessary build-fu to make sure that nothing will break on 10.2, which we still support on the 1.8 branch.  This feature is also handled entirely by the window server, and I've implemented it in such a way that the fades occur asynchronously.  (The async fade means that the window object might be destroyed before the fade is finished.  This could potentially cause crashes, so I've added an nsCOMPtr to hold a death-grip on the widget and release it when the fade is done.)

I'd like to get this for 1.8.1 as a major look-and-feel enhancement to match the native appearance on the Mac.

Comment 2

[Mark Mentovai]

Attachment: New icons

These icons go with attachment 229159 [details] [diff] [review].  They're completely identical to the existing icons, but shifted downward by 2px.

Comment 3

[Mark Mentovai]

Attachment: Picture of old non-native-looking contextual menu

Comment 4

[Mark Mentovai]

Attachment: Picture with new implementation - can you believe it's XUL?

Comment 5

[Håkan Waara]

That looks awesome Mark!

Comment 6

[Josh Aas]

Comment on attachment 229159 [details] [diff] [review]
Patch v1

+        if (mWindowType == eWindowType_popup && transitionFunc) {
+          mDeathGripDuringTransition = this;
+          TransitionWindowOptions transitionOptions = { version  : 0,
+                                                        duration : 0.2,
+                                                        window   : nsnull,
+                                                        userData : nsnull };
+          transitionFunc(mWindowPtr, kWindowFadeTransitionEffect,
+                         kWindowHideTransitionAction, nsnull,
+                         PR_TRUE, &transitionOptions);
+        }

Why not drop the eWindowType_popup check here and move this if block into the scope above it? Check seems redundant.

Comment 7

[Josh Aas]

Comment on attachment 229159 [details] [diff] [review]
Patch v1

otherwise, looks good

Comment 8

[Mark Mentovai]

Comment on attachment 229159 [details] [diff] [review]
Patch v1

Leaving the check the way it is lets me take advantage of it for the |else| clause below.  The checks are:

+        if (mWindowType == eWindowType_popup) {
           // attempt to set transitionFunc
+        }
+        if (mWindowType == eWindowType_popup && transitionFunc) {
           // call transitionFunc
+        }
+        else
           // hide window without transitionFunc

If I didn't do this, then I'd need to have two |else| clauses to handle that final case.

Comment 9

[Mark Mentovai]

Checked in on the trunk.

Comment 10

[Stefan [:stefanh]]

Comment on attachment 229159 [details] [diff] [review]
Patch v1

This looks great. However, label text in "plain" (non-iconic) menuitems and label text in menuitems with icons doesn't line up anymore. You can see the effect if you right-click in the help windows content area. But I suppose I could fix that in bug 342515

Comment 11

[Mark Mentovai]

Attachment: Fix for iconic menus

Good point, Stefan.  This fixes the layout with iconic menu items, using the proper native positioning for the check icons and the text.  (It's important to me to match native positioning.)

Comment 12

[Mark Mentovai]

Comment on attachment 229358 [details] [diff] [review]
Fix for iconic menus

I discussed this with Stefan, and he will roll the fixes into bug 342515.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 229159 [details] [diff] [review]
Patch v1

a=mconnor on behalf of drivers for checkin by Wednesday, July 19th

If 342515 is not going to be ready, we should also take the added fix here to fix the checkmark positioning. Either way by the end of the week is the cutoff to make that call.

Comment 14

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH for 1.8.1b2.

mconnor, you gave a/1.8.1 but did not actually mark the attachment as such (and it's a locked-down flag so I can't fix it).