Make contextual menus look more native

RESOLVED FIXED in mozilla1.8.1beta2

Status

Core Graveyard
Widget: Mac
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Mark Mentovai, Assigned: Mark Mentovai)

Tracking

({fixed1.8.1})

Trunk
mozilla1.8.1beta2
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

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

Comment 2

12 years ago
Created attachment 229160 [details]
New icons

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

Comment 3

12 years ago
Created attachment 229162 [details]
Picture of old non-native-looking contextual menu
(Assignee)

Comment 4

12 years ago
Created attachment 229163 [details]
Picture with new implementation - can you believe it's XUL?
(Assignee)

Updated

12 years ago
Attachment #229163 - Attachment mime type: text/plain → image/png

Comment 5

12 years ago
That looks awesome Mark!

Comment 6

12 years ago
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

12 years ago
Comment on attachment 229159 [details] [diff] [review]
Patch v1

otherwise, looks good
Attachment #229159 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 8

12 years ago
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.
Attachment #229159 - Flags: superreview?(bryner)
Attachment #229159 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 9

12 years ago
Checked in on the trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #229159 - Flags: approval1.8.1?

Comment 10

12 years ago
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
(Assignee)

Comment 11

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

Comment 12

12 years ago
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.
Attachment #229358 - Flags: review?(stefanh)
(Assignee)

Updated

12 years ago
Attachment #229358 - Attachment is obsolete: true
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.
(Assignee)

Comment 14

12 years ago
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).
Keywords: fixed1.8.1
Target Milestone: --- → mozilla1.8.1beta2

Updated

12 years ago
Attachment #229159 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Updated

12 years ago
Depends on: 345739

Updated

11 years ago
Blocks: 349437
(Assignee)

Updated

11 years ago
Depends on: 353716

Updated

8 years ago
Component: Widget: Mac → Widget: Mac
Product: Core → Core Graveyard
Version: 1.8 Branch → Trunk
You need to log in before you can comment on or make changes to this bug.