342515 - [Pinstripe] Use better font and spacing in menus/menuitems

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Stefan [:stefanh]]

The current font used in menuitems is the standard system font which is smaller than the OS menuitemfont. Bug 335683 made it possible to use the real system font for menuitems. Also, compared with the OS, Pinstripe's menuitems has around 7 px more spacing to the left. This is probably done in order to give the icons enough room at the left - icons in menuitems do not line up with the label text (in menuitems without icons) as they do in the OS.

In here, I intend to fix the menuitemfont and make left spacing in "iconic" menuitems and "non-iconic" menuitems the same. Since we'll have cocoa toolkit for 1.9 this bug is targetted for 1.8 branch.

Comment 1

[Stefan [:stefanh]]

*** Bug 308010 has been marked as a duplicate of this bug. ***

Comment 2

[Asaf Romano (gone)]

(Cocoa toolkit does not mean native context menus, really).

Comment 3

[Stefan [:stefanh]]

Attachment: New menu-arrow-dis.gif for Pinstripe

Pinstripe actually uses a menu-arrow-dis.gif that doesn't have the same shape as the other arrows. This new one is ment to go in with the forthcoming patch.

Comment 4

[Stefan [:stefanh]]

Attachment: autorepeat-arrow-up.gif --> pinstripe/global/scrollbox (new directory)

New up-arrow for the scrollbox...

Comment 5

[Stefan [:stefanh]]

Attachment: autorepeat-arrow-dn.gif --> pinstripe/global/scrollbox

New down-arrow for the scrollbox...

Comment 6

[Stefan [:stefanh]]

Attachment: Testcase - menus

Comment 7

[Stefan [:stefanh]]

Attachment: Testcase - menulist

Comment 8

[Stefan [:stefanh]]

Setting a more realistic target milestone...

Comment 9

[Stefan [:stefanh]]

Attachment: autorepeat-arrow-up-dis.gif --> pinstripe/global/scrollbox

Comment 10

[Stefan [:stefanh]]

Attachment: autorepeat-arrow-dn-dis.gif --> pinstripe/global/scrollbox

Comment 11

[Stefan [:stefanh]]

I expect to have a patch in a couple of days.

Comment 12

[Stefan [:stefanh]]

Attachment: Make menuitems/menus look more mac-alike

I'll let Mano look at this first - then I'll request ui-review from beltzner


This patch assumes the following:

1) The existing menu-arrow-dis.gif in toolkit/themes/pinstripe/global/menu is replaced by the one in attachment #226944 [details]

2) The new icons in attachment #227076 [details], attachment #227077 [details], attachment #228535 [details] and attachment 228536 [details] are placed in a  new scrollbox directory in toolkit/themes/pinstripe/global.

Some comments & explanations to ease the review:
-------------------------------------
 .menu-accel,
 .menu-iconic-accel {
-  margin: 0px 2px 0px 7px !important;
-  padding-right: 14px;
+  margin: 0px 2px 0px 2px !important;
+  padding-right: 2px;
   color: inherit;
+
 }

This part is a change that I haven't done in the seamonkey version. One issue with not changing this is that there will be too much space to the right in menuitems. Space at the left is affected by the left padding of the menuitem and how much left margin you'll have on label text and .menu-iconic-left. Space at the right in a menuitem without accelkeys depends on margin/padding of .menu-accel and the right margin of the label text (and of course also the right padding of the menuitem itself). The effect of doing this change is that there will be less space for the accelkeys and that without accelkeys the sub-menu arrow will be more at the left (17px). I haven't really seen any issue with this. I'll try and post a worst-scenario testcase here tomorrow.

-------------------------------------
menupopup > menu,
 popup > menu,
 menupopup > menuitem,
 popup > menuitem {
-  padding: 1px 8px 1px 8px !important;
+  padding: 1px 11px 1px 3px !important;
   max-width: 42em;
-  min-height: 20px;
 }

Note that removing the min-height will make menuitem labels in xul menulists (like in the testcase) almost touch the top if you use the default font in your prefs (the font is inherited from the parent). I don't really consider this an issue, though - this happened before too - provided that you had a larger font-size than the default.

The effect on "normal" menuitems is that they now are the same height as Apple's own. One small side-affect is that the baseline of the label text is rather low positioned. I guess it's just a matter of taste here. I prefer keeping the height adjustable to font-size.

-------------------------------------
.menu-right {
-  margin: 0px 0px 0px 6px;
+  margin: 1px 0px 0px 6px;

I noticed that the right-arrow didn't aligned with the label text/accels baseline. And it looks like there is no reasonable way of making the .menu-right hbox know the baseline of the two labels (talked with Neil about it). So, I'm forced to add some top margin here.

Note also that I haven't used any right margin here as I did for seamonkey. Instead I've used more right padding on the menuitem itself.


-------------------------------------
+/* ::::: menu/menuitems in menubar ::::: */
+
+menubar > menu {
+  -moz-appearance: none !important;
+  padding: 2px 5px 2px 7px;
+  margin: 1px 0px 1px 0px;
+}
+
+menubar > menu[_moz-menuactive="true"][open="true"] {
+  -moz-appearance: menuitem !important;
+  color: -moz-mac-menutextselect !important
+}
+

Together with the changes in toolbar.css this is the patch in bug 289300.

-------------------------------------
 menulist {
   -moz-appearance: menulist;
   margin: 2px 4px;
-  height: 20px !important;
+  min-height: 20px !important;
 }

I don't think there should be a fixed height here since the menulists label text is inherited from the parent (using a large font will make the label text appear outside the menulist).

Comment 13

[Stefan [:stefanh]]

I might play a little bit more with padding/margin on the accel/arrow boxes (the goal is however to have the same left/righ space in menuitems).

Comment 14

[Stefan [:stefanh]]

Attachment: Testcase, sub-menu arrow position

Comment 15

[Stefan [:stefanh]]

Attachment: New patch

OK, so this is a new patch. This one includes (and changes some of) Marks stuff from bug 344570. Since I don't see any difference  between the seamonkey padding on menuitems and my "new" idea in the previous attachment I decided to keep the 3px left/right padding from seamonkey.

I'll update the new menu-arrow-dis.gif.

Comment 16

[Stefan [:stefanh]]

Attachment: Testcase (sub-menu arrow and accelkeys position)

Just a new and  updated testcase. This just shows the worst-case scenario regarding the space around sub-menu arrows and accelkeys.

Comment 17

[Stefan [:stefanh]]

Attachment: Screenshots of testcases before/after patch

Comment 18

[Stefan [:stefanh]]

Comment on attachment 229353 [details] [diff] [review]
New patch

mark, since I'm actually changing your stuff a bit here - can you take a look at this? See comment #12 for what's necessary before applying the patch.

Comment 19

[Mark Mentovai]

Comment on attachment 229353 [details] [diff] [review]
New patch

Looks and feels great.  I grabbed a few menus and compared a few metrics with their native counterparts, and they're well-matched.

Comment 20

[Mark Mentovai]

Attachment: Icons with reduced palettes

The icons attached to this bug have full 8-bit palettes, when they can really get away with 4-bit color tables.  The extra unused slots in the color table (and maybe other factors) are causing the file size to be larger than the uncompressed representation of these images!  This zip archive contains the original four images with appropriately reduced color tables and without any extra cruft.

Comment 21

[Stefan [:stefanh]]

(In reply to comment #20)
> Created an attachment (id=229673) [edit]
> Icons with reduced palettes
> 
> The icons attached to this bug have full 8-bit palettes, when they can really
> get away with 4-bit color tables.  The extra unused slots in the color table
> (and maybe other factors) are causing the file size to be larger than the
> uncompressed representation of these images!  This zip archive contains the
> original four images with appropriately reduced color tables and without any
> extra cruft.
> 

Thanks Mark :-)

Comment 22

[Asaf Romano (gone)]

Comment on attachment 229353 [details] [diff] [review]
New patch

r=mano

Comment 23

[Stefan [:stefanh]]

Attachment: New and updated (shifted 2px downwards) menu-arrow-dis.gif

> I'll update the new menu-arrow-dis.gif.

Here's the updated menu-arrow-dis.gif.

Comment 24

[Mark Mentovai]

Checked in on the trunk:

mozilla/toolkit/themes/pinstripe/global/jar.mn 1.17
mozilla/toolkit/themes/pinstripe/global/menu.css 1.11
mozilla/toolkit/themes/pinstripe/global/menulist.css 1.10
mozilla/toolkit/themes/pinstripe/global/scrollbox.css 1.6
mozilla/toolkit/themes/pinstripe/global/toolbar.css 1.5
mozilla/toolkit/themes/pinstripe/global/menu/menu-arrow-dis.gif 1.4
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-dn-dis.gif 1.1
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-dn.gif 1.1
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-up-dis.gif 1.1
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-up.gif 1.1

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229353 [details] [diff] [review]
New patch

a=drivers, please land on the 1.8.1branch, with thanks!

Comment 26

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH for 1.8.1b2:

mozilla/toolkit/themes/pinstripe/global/jar.mn 1.12.2.4
mozilla/toolkit/themes/pinstripe/global/menu.css 1.8.2.2
mozilla/toolkit/themes/pinstripe/global/menulist.css 1.8.8.1
mozilla/toolkit/themes/pinstripe/global/scrollbox.css 1.2.26.3
mozilla/toolkit/themes/pinstripe/global/toolbar.css 1.3.10.2
mozilla/toolkit/themes/pinstripe/global/menu/menu-arrow-dis.gif 1.2.26.2
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-dn-dis.gif 1.1.2.1
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-dn.gif 1.1.2.1
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-up-dis.gif 1.1.2.1
mozilla/toolkit/themes/pinstripe/global/scrollbox/autorepeat-arrow-up.gif 1.1.2.1