404751 - Change menu spacing to match native GTK apps

Status: RESOLVED FIXED

Description

[Michael Monreal [:micmon]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071022 Ubuntu/7.10 (gutsy) Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112004 Minefield/3.0b2pre

In order to match native GTK applications, I started a patch for displaying icons in menus. This already works and looks great, but I wonder if it would be possible to change menu spacing globally.

Right now, icons start just one pixel from the left menu border. In GTK, there is more space between icon and menu border (4px). Also, the space after the icon is bigger (6px compared to 3px). I have done a mockup with this and I think GTK's look has a nicer feel to it. So, would it be possible to change this globally?

Also, the space when a separator is used is slightly larger with native GTK. This is not much of a problem but I think it still looks better. Perhaps thsi can be changed, too?

One last thing is the position of the accelerator keys on the right. Firefox seems to have a huge space between the label and the right menu border compared to GTK, but this seems to be because Firefox does not align accelerator lables to the right border. Would it be possible to copy the GTK way in Firefox?

Reproducible: Always

Comment 1

[Michael Monreal [:micmon]]

Created attachment 289650 [details]
Screenshots and Mockup

Comment 2

[Michael Monreal [:micmon]]

Fixing the left menu side is easy:

menu, menuitem {
  padding-left: 3px !important; 
}

Fixing the space after the icon is a bit tricky...

Comment 3

[Michael Monreal [:micmon]]

Adding menu icons is filed in bug 405165 btw

Comment 4

[Michael Ventnor]

Created attachment 290804 [details] [diff] [review]
Patch

Fixes spacing, plus fixes the position of the indicator in check-menuitems. I'll try to get back to the alignment of the accelerators, I think I'm encountering a bug with that one.

Comment 5

[Michael Ventnor]

Created attachment 290805 [details] [diff] [review]
Patch 2

The error for accelerator alignment was pinpointed between my chair and my monitor.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are these 3px values true for all themes, or should we be using the horizontal-padding or something?

Comment 7

[Michael Ventnor]

Created attachment 290814 [details] [diff] [review]
Patch 3

Yeah, its probably a better idea.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    aResult->left = aResult->right = horizontal_padding;
+    return PR_TRUE;

You'd better set top and bottom to zero.

-  -moz-margin-start: 18px !important;
+  -moz-margin-start: 21px !important;

A comment should say how this 21px is computed.

+  -moz-padding-end: 3px !important;

So we're hardcoding the gap between the icon and the text? A comment should say that.

Comment 9

[Michael Ventnor]

Created attachment 290820 [details] [diff] [review]
Patch 4

Comment 10

[Reed Loden [:reed] (use needinfo?)]

Comment on attachment 290820 [details] [diff] [review]
Patch 4

Help make Firefox menus look more like normal GNOME menus.

Comment 11

[Hussam Al-Tayeb]

This patch here caused Bug 406129. Please don't check in.

Comment 12

[Damon Sicore (:damons)]

What's the story with Comment #11?  Re-request approval once clarified.

Comment 13

[Hussam Al-Tayeb]

(In reply to comment #12)
> What's the story with Comment #11?  Re-request approval once clarified.
> 
when I applied patch 4 to my builds, I got some problems. I reported one of them as Bug 406129. When I removed the patch from my builds, Bug 406129 no longer happens. So it is definitely caused by patch 4.

Comment 16

[Michael Ventnor]

Created attachment 291102 [details] [diff] [review]
Patch that doesn't cause chaos

Unfortunately this means we have to assume that every theme doesn't change from Clearlooks' paddings. But it works and thats all that matters.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> But it works and thats all that matters.

Well no, robust, maintainable code that will keep working in the future also matters.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 291102 [details] [diff] [review]
Patch that doesn't cause chaos

but at least this is reasonably well documented, so I guess we can live with it.

Comment 19

[Reed Loden [:reed] (use needinfo?)]

Checking in toolkit/themes/gnomestripe/global/menu.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/menu.css,v  <--  menu.css
new revision: 1.16; previous revision: 1.15
done
Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.46; previous revision: 1.45
done
Checking in toolkit/content/widgets/menu.xml;
/cvsroot/mozilla/toolkit/content/widgets/menu.xml,v  <--  menu.xml
new revision: 1.15; previous revision: 1.14
done