345252 - [Mac Classic] more menuitem polish/corrections

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Stefan [:stefanh]]

Bug 344570 introduced a new native color for disabled text that we should use and made a few other changes that I like to port to Classic. Also, when porting my menu changes (bug 301105) to Pinstripe (bug 342515) I became aware of a few things that I want to correct in Classic.

Comment 1

[Stefan [:stefanh]]

Attachment: New menu-arrow.gif (global/mac/menu)

New icon (2 more to be attached) from bug 342515. Less size and arrow moved downwards 2px.

Comment 2

[Stefan [:stefanh]]

Attachment: New menu-arrow-dis.gif (global/mac/menu)

Comment 3

[Stefan [:stefanh]]

Attachment: New menu-arrow-hov.gif (global/mac/menu)

Comment 4

[Stefan [:stefanh]]

Attachment: Screenshot of current menulist checkmark

The current checkmark looks ok in menus, but menulist menuitems uses a rather small font. This makes the checkmark look too large in menulists. I'll either revert to the old checkmark for just menulists or I'll come up with a new one. Note that from a widget point of view it's not a very good idea to hard-code a font size like  this when Apple doesn't (Apple uses lots of different sized "menulists"). Toolkit has "-moz-pull-down-menu" (same as menus) which will make  suiterunner prefs look funny (too large font). The best way would be not have  any font-size set (so it inherits) and use some nsNativeTheme magic and make the checkmark size depend on font-size.

Comment 5

[Stefan [:stefanh]]

Attachment: menulist-check.png --> global/mac/menu/

New icon for checked menulists (2 more will be attached).

Comment 6

[Stefan [:stefanh]]

Attachment: menulist-check-dis.png --> global/mac/menu/

Comment 7

[Stefan [:stefanh]]

Attachment: menulist-check-hov.png --> global/mac/menu/

Comment 8

[Stefan [:stefanh]]

Attachment: Fix padding/margin etc and use new checkmarks for menulists

I'm going to run a few tests with this patch before I ask for review.

Comment 9

[Stefan [:stefanh]]

Attachment: Screenshot - new menulist checkmark

I think this is better - what we do here is sort of faking menuitems in the smaller menulist that Apple uses in, for instance, the textEdit pref window. Our ones is higher, but I think the new checkmark is enough (It's more complicated to start messing with the menuitem height and we should also mimic the menulist better then).

Comment 10

[Stefan [:stefanh]]

Karsten what do you think -  shall we keep the current checkmarks? I'm starting to think the new ones are not that nice... :/

Comment 11

[Stefan [:stefanh]]

Comment on attachment 237766 [details] [diff] [review]
Fix padding/margin etc and use new checkmarks for menulists

I'm going to skip the new checkmarks for menulists. From the few reactions I got it seems that the current ones are OK.

Comment 12

[Stefan [:stefanh]]

Attachment: New version without menuitem checkmarks

The 3 new menu-arrows needs to be in global/mac/menu/ - the arrows are moved down 2px compared to the old ones (I think I was wrong about the size diff).

1) Uses disabled textcolor (-moz-mac-menutextdisable) instead of GrayText.

2) Tweaks padding/margin a little bit. Basically the same values as the ones in bug 342515 (I had to use !important on menuitem padding styles to fix the bm-menu). Also, fixes the margin on .menu-iconic-left so the look of menuitems with icons matches the OS (this is just 1px more top-margin). I filed bug 352679 for the Pinstripe .menu-iconic-left fix.

3)
 
-menuitem[checked="true"][disabled="true"] {
-  list-style-image: url("chrome://global/skin/menu/menu-check-dis.png");
+menuitem[checked="true"][_moz-menuactive="true"] {
+  list-style-image: url("chrome://global/skin/menu/menu-check-hov.png");
   -moz-image-region: auto;
 }
 
-menuitem[checked="true"][_moz-menuactive="true"] {
-  list-style-image: url("chrome://global/skin/menu/menu-check-hov.png");
+menuitem[checked="true"][disabled="true"] {
+  list-style-image: url("chrome://global/skin/menu/menu-check-dis.png");
   -moz-image-region: auto;
 }

If you hover over a checked menuitem that is disabled you'll see a hover effect on the checkmark(try the left menu in the testcase at https://bugzilla.mozilla.org/attachment.cgi?id=227966). We don't want this. The above code flip fixes the issue.

Comment 13

[[e-mail]]

(In reply to comment #12)
>If you hover over a checked menuitem that is disabled you'll see a hover effect
>on the checkmark (try the left menu in the testcase at
>https://bugzilla.mozilla.org/attachment.cgi?id=227966). We don't want this. The
>above code flip fixes the issue.
If you don't need a hover effect why not remove the [_moz-menuactive] rule?

Comment 14

[Stefan [:stefanh]]

Comment on attachment 238450 [details] [diff] [review]
New version without menuitem checkmarks

I don't know if Mano has the time for this and I would really like this to land before 1.1b - Karsten, can you take a look at this? Should be pretty straight-forward (see comment #12 for some clarifications).

Comment 15

[Stefan [:stefanh]]

Neil, any chance you could take a look at the patch? I wouldn't mind having this fixed in 1.1 (it starts to look like it won't make it for 1.1b).

Comment 16

[[e-mail]]

(In reply to comment #15)
>Neil, any chance you could take a look at the patch?
Any chance you could answer comment #13?

Comment 17

[Stefan [:stefanh]]

(In reply to comment #16)
> (In reply to comment #15)
> >Neil, any chance you could take a look at the patch?
> Any chance you could answer comment #13?
> 

Oh, I thought we sorted that out on irc... it was some time ago, though. I want the hover effect, but not on disabled menuitems. The error has been there for ages - I don't know if that was a desired behaviour on mac classic (old classic OS) or if it's been there just because it has slipped through (not many menuitems are checked and disabled except for some in the main menu, but that's native on mac).

Comment 18

[[e-mail]]

Comment on attachment 238450 [details] [diff] [review]
New version without menuitem checkmarks

Looks like Mac isn't the only theme with the disabled checkbox hover bug - care to spin up a bug/patch for Win/Unix?

Comment 19

[Stefan [:stefanh]]

Comment on attachment 238450 [details] [diff] [review]
New version without menuitem checkmarks

Asking approval for some theme-polish of the mac classic skin. This is pretty safe, most of the tweaks I've done here is already in Firefox 2 ;-)

Comment 20

[Stefan [:stefanh]]

(In reply to comment #18)
> (From update of attachment 238450 [details] [diff] [review] [edit])
> Looks like Mac isn't the only theme with the disabled checkbox hover bug - care
> to spin up a bug/patch for Win/Unix?
> 

Filed bug 358553 for the Win/Unix issue.

Comment 21

[Robert Kaiser]

Comment on attachment 238450 [details] [diff] [review]
New version without menuitem checkmarks

a=me for 1.1b

Comment 22

[Stefan [:stefanh]]

Neil can you check in the patch on trunk/1.8.1 please? attachment #237652 [details], attachment #237653 [details] and attachment #237653 [details] need to go in as well.

Comment 23

[Karsten Düsterloh]

Landed patch (#238450) and icons (#237652, #237653 and #237655) on MOZILLA_1_8_BRANCH.

Comment 24

[Stefan [:stefanh]]

Attachment: Unbitrottened trunk patch

Bug 293207 bitrottened the patch on trunk.

Comment 25

[Karsten Düsterloh]

Landed patch (#243940) and icons (#237652, #237653 and #237655) on trunk.

Comment 26

[Stefan [:stefanh]]

--> Fixed.