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/2007111904 Minefield/3.0b2pre

The selected menu looks a bit odd compared to native GTK. See screenshot.

Reproducible: Always

	Ian Spence Assignee
	Comment 2 • 17 years ago

This implements NS_THEME_MENUBARITEM, which is specifically for GtkMenuItems inside GtkMenuBars.

It's not a huge patch,the only thing I'm not sure is if I'm allowed to add NS_THEME_MENUBARITEM since it's not already in the spec.

	Teune van Steeg
	Comment 3 • 17 years ago

Comment on attachment 291084 [details] [diff] [review]
Implements NS_THEME_MENUBARITEM

I'm having a few remarks about this patch:

>+// menuitems in menubar appearance
>+#define NS_THEME_MENUBARITEM                               221
> [...]
>+CSS_KEY(menubaritem, menubaritem)
> [...]
>+  eCSSKeyword_menubaritem,            NS_THEME_MENUBARITEM,
> [...]
> menubar > menu[open] {
>+  -moz-appearance: menubaritem !important;

This is not needed: there is a way in nsNativeThemeGTK.cpp to check if a menu item is on a menubar (line 279 and further).

>         ensure_menu_item_widget();
how do you want to ensure gMenuBarItemWidget is not NULL?
>+        
>+        GtkWidget* item_widget;
this should be defined at the beginning of the function.

>+    case MOZ_GTK_MENUBARITEM:
>+        ensure_menu_bar_item_widget();
>+        w = gMenuBarItemWidget;
>+        break;
where is ensure_menu_bar_item_widget() defined?

	Teune van Steeg
	Comment 4 • 17 years ago

(In reply to comment #3)
> 
> >+    case MOZ_GTK_MENUBARITEM:
> >+        ensure_menu_bar_item_widget();
> >+        w = gMenuBarItemWidget;
> >+        break;
> where is ensure_menu_bar_item_widget() defined?
> 
Sorry, forget about this, I found it :)

	Ian Spence Assignee
	Comment 5 • 17 years ago

A whole lot simpler of a patch.

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

Comment on attachment 291090 [details] [diff] [review]
Doesn't implement NS_ anything, just works

Please remember to request review on your patches.

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

+            *aWidgetFlags = 1;
           } else {
             aState->inHover = CheckBooleanAttr(aFrame, nsWidgetAtoms::mozmenuactive);
+            *aWidgetFlags = 0;

Use a #define here, something new if necessary, e.g. MOZ_TOPLEVEL_MENU_ITEM. I believe all the other flags have #defined values.

Otherwise looks great, thanks!!!

	Ian Spence Assignee
	Comment 8 • 17 years ago

Updated to use a bitwise flag. I couldn't think of any other reasons a menuitem would need a flag, but I figured better safe than sorry.

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

Comment on attachment 291308 [details] [diff] [review]
Updated to use defined constant

Making Firefox on Linux better, one patch at a time.

	Mike Schroepfer
	Comment 11 • 17 years ago

Comment on attachment 291308 [details] [diff] [review]
Updated to use defined constant

Rockin!  Nice work..

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

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.47; previous revision: 1.46
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.40; previous revision: 1.39
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.117; previous revision: 1.116
done