Michael Monreal [:micmon] Reporter
	Description • 17 years ago

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

	Michael Monreal [:micmon] Reporter
	Comment 2 • 17 years ago

Fixing the left menu side is easy:

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

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

	Michael Monreal [:micmon] Reporter
	Comment 3 • 17 years ago

Adding menu icons is filed in bug 405165 btw

	Michael Ventnor Assignee
	Comment 4 • 17 years ago

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.

	Michael Ventnor Assignee
	Comment 5 • 17 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 17 years ago

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

	Michael Ventnor Assignee
	Comment 7 • 17 years ago

Yeah, its probably a better idea.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 17 years ago

+    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.

	Reed Loden [:reed]
	Comment 10 • 17 years ago

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

Help make Firefox menus look more like normal GNOME menus.

	Hussam Al-Tayeb
	Comment 11 • 17 years ago

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

	Damon Sicore (:damons)
	Comment 12 • 17 years ago

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

	Hussam Al-Tayeb
	Comment 13 • 17 years ago

(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.

	Michael Ventnor Assignee
	Comment 16 • 17 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 17 years ago

> But it works and thats all that matters.

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 17 years ago

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.

	Reed Loden [:reed]
	Comment 19 • 17 years ago

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