407069 - GTK Menus ignore native vertical padding

Status: REOPENED

(Core :: Widget: Gtk, defect)

Description

[Ian Spence]

Attachment: Image showing the problem

GTK Menus have the option for vertical padding, which some popular themes do take advantage of. Currently padding for a menu is set to 0 pixels all around. The top and bottom padding should take from the GTK style.

Attached is an image showing the problem.
On the top is a native GTK application menu. In the middle is how Firefox currently renders the menu. On the bottom is how it renders with the pending patch.

Comment 1

[Ian Spence]

Attachment: Patch to give menus native vertical padding

Actually a pretty straight forward patch. I look up the vertical padding, and set it in nsNativeThemeGTK::GetWidgetPadding.

Comment 2

[Ian Spence]

Comment on attachment 291783 [details] [diff] [review]
Patch to give menus native vertical padding

*wonders if he's allowed to request approval1.9*

Comment 3

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

Comment on attachment 291783 [details] [diff] [review]
Patch to give menus native vertical padding

a=drivers for after the Firefox 3 Beta 2 freeze

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 291783 [details] [diff] [review]
Patch to give menus native vertical padding

>Index: widget/src/gtk2/nsNativeThemeGTK.cpp

>+  if (aWidgetType == NS_THEME_MENUPOPUP) {
>+      gint vertical_padding;
>+      moz_gtk_get_menu_popup_vertical_padding(&vertical_padding);
>+      
>+      aResult->SizeTo(0, vertical_padding, 0, vertical_padding);
>+      return PR_TRUE;
>+  }

The indention is wrong. Should be two spaces in this file.

Comment 5

[Ian Spence]

Attachment: Updates the indentation

The same the the last patch, but with reduced tabs.

Do I need to re-request approval?

Comment 6

[Reed Loden [:reed]]

(In reply to comment #5)
> Do I need to re-request approval?

Nope. Once the tree reopens, I'll commit the change.

Comment 7

[Reed Loden [:reed]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.49; previous revision: 1.48
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.42; previous revision: 1.41
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.121; previous revision: 1.120
done

Comment 8

[Ian Spence]

This should probably be backed out until I can figure out how to determine how to be sure it is a dropdown from the menubar/toolbar (or a submenu of one).

This padding, while it is the correct thing to do, it being exposed as a problem with the fix of bugs like bug 398242

Comment 9

[Reed Loden [:reed]]

Backed out.

Comment 11

[James Schwinabart [:mutantmonkey] (he/him)]

Since bug 398242 is obsolete, can someone take a look at this again? This is very annoying for me.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:stransky, could you have a look please?
For more information, please visit auto_nag documentation.