404498 - Improve GTK look of menu selection

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

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

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

Reproducible: Always

Comment 1

[Michael Monreal [:micmon]]

Attachment: Screenshot FF3 <> native GTK

Comment 2

[Ian Spence]

Attachment: Implements NS_THEME_MENUBARITEM

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.

Comment 3

[Teune van Steeg]

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?

Comment 4

[Teune van Steeg]

(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 :)

Comment 5

[Ian Spence]

Attachment: Doesn't implement NS_ anything, just works

A whole lot simpler of a patch.

Comment 6

[Reed Loden [:reed]]

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

Please remember to request review on your patches.

Comment 7

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

+            *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!!!

Comment 8

[Ian Spence]

Attachment: Updated to use defined constant

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.

Comment 10

[Reed Loden [:reed]]

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

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

Comment 11

[Mike Schroepfer]

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

Rockin!  Nice work..

Comment 12

[Reed Loden [:reed]]

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