Closed Bug 118296 Opened 23 years ago Closed 20 years ago

NS_THEME_MENU* implementations (GTK)

Categories

(Core Graveyard :: Skinability, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ian, Assigned: p_ch)

References

Details

Attachments

(1 file, 4 obsolete files)

This covers the implementation of NS_THEME_MENUBAR, NS_THEME_MENU_POPUP and
NS_THEME_MENUITEM for XUL menus on GTK.
Blocks: 34572, 117584
Blocks: 39403
No longer blocks: 34572
taking.

I changed the current theme keyword list. NS_THEME_MENU is too vague, menu can
refer to the xul <menu> element and in CSS3, gtk or the common language, menu
refers to popups. So I eliminated NS_THEME_MENU from the codebase in favor of:

NS_THEME_MENU_BAR   | <menubar>            | -moz-appearance: menu-bar;
NS_THEME_MENU_POPUP | <menupopup>, <popup> | -moz-appearance: menu-popup;
NS_THEME_MENU_ITEM  | <menu>, <menuitem>   | -moz-appearance: menu-item;
 
The only difference between <menu> and <menuitem> is the <menu> arrow when
included in a menupopup. This patch does not address this issue, but for
clarity, I think that there should be a single NS_THEME_MENU_ITEM for both, and
deal with the differences in the nsITheme implementation.

I've done some refactoring, and in particular, I needed more flexibility: I
dumped the GtkReliefStyle argument in moz_gtk_button_paint, and preferred to
modify the widget state before, in GetGtkWidgetAndState.

This patch also does not address the fact that in gtk2, hovering on a menubar
doesn't select the "menu", because it's not a theme issue.
Assignee: blizzard → p_ch
Attached patch preliminary patch (obsolete) — Splinter Review
I still need to deal with the widget borders
hyatt, do you agree with the changes in the theme keyword list?
Attachment #140357 - Attachment is obsolete: true
Attached patch still not ready (obsolete) — Splinter Review
> I dumped the GtkReliefStyle argument in moz_gtk_button_paint
I reverted it back, it was wrong.

This patch is tested on more themes (cruz, aero and H20), but still lacks the
border stuff.
Attached patch patch v1.0 (obsolete) — Splinter Review
this patch deals with borders and is ready for reviews. I also implemented
NS_THEME_TOOLBAR for real.
Attachment #140382 - Attachment is obsolete: true
Attachment #140635 - Flags: superreview?(roc)
Attachment #140635 - Flags: review?(bryner)
The more I think of it, the more I would not use dashes for the values of the
-moz-appearance property (menubar, menupoup, menuitem and toolbar)...
Having toolbar and menu-bar would be inconsistent and tool-bar isn't nice imo.
Blocks: 233462
Attached patch patch v1.1 (obsolete) — Splinter Review
use of menubar, menupopup and menuitem
Attachment #140635 - Attachment is obsolete: true
Attachment #140635 - Flags: superreview?(roc)
Attachment #140635 - Flags: review?(bryner)
Attachment #141217 - Flags: superreview?(roc)
Attachment #141217 - Flags: review?(bryner)
Comment on attachment 141217 [details] [diff] [review]
patch v1.1

> static GtkStateType
> ConvertGtkState(GtkWidgetState* state)
> {
>     if (state->disabled)
>         return GTK_STATE_INSENSITIVE;
>+    else if (state->active)
>+        return GTK_STATE_ACTIVE;
>     else if (state->inHover)
>-        return (state->active ? GTK_STATE_ACTIVE : GTK_STATE_PRELIGHT);
>+        return GTK_STATE_PRELIGHT;
>     else
>         return GTK_STATE_NORMAL;
> }

I don't think this is quite right... our 'active' doesn't quite map to gtk's
'active'.  I'll take the example of pressing the left mouse button with the
pointer over a <button>.  In that case, we don't want to draw STATE_ACTIVE
(which is the button's pressed-in look) if you hold down the button while
moving the pointer out of the button, even though the css state 'active'
continues to apply.

>@@ -289,13 +355,16 @@
>         width -= 2;
>         height -= 2;
>     }
>-	
>+
>     shadow_type = (state->active && state->inHover) ?
>         GTK_SHADOW_IN : GTK_SHADOW_OUT;
>-  
>+
>     if (relief != GTK_RELIEF_NONE || (button_state != GTK_STATE_NORMAL &&
>                                       button_state != GTK_STATE_INSENSITIVE)) {
>         TSOffsetStyleGCs(style, x, y);
>+        /* the following line can trigger an assertion (Crux theme)
>+           file ../../gdk/gdkwindow.c: line 1846 (gdk_window_clear_area):
>+           assertion `GDK_IS_WINDOW (window)' failed */

Does this draw correctly despite the assertion?  Would we want to blacklist
this widget if you're using Crux?

>+static gint
>+moz_gtk_menu_bar_paint(GdkDrawable* drawable, GdkRectangle* rect,
>+                       GdkRectangle* cliprect)
>+{
>+    GtkStyle* style;
>+    GtkShadowType shadow_type;
>+    ensure_menu_bar_widget();
>+    style = gMenuBarWidget->style;
>+    gtk_widget_style_get(gMenuBarWidget, "shadow_type",
>+                         &shadow_type, NULL);

We should maybe consider caching the shadow type.  It shouldn't be changing (we
would have to invalidate it on theme change though).

>+static gint
>+moz_gtk_menu_item_paint(GdkDrawable* drawable, GdkRectangle* rect,
>+                        GdkRectangle* cliprect, GtkWidgetState* state)
>+{
>+    GtkStyle* style;
>+    GtkShadowType shadow_type;
>+
>+    if (state->inHover && !state->disabled) {
>+        ensure_menu_item_widget();
>+
>+        style = gMenuItemWidget->style;
>+        TSOffsetStyleGCs(style, rect->x, rect->y);
>+        gtk_widget_style_get(gMenuItemWidget, "selected_shadow_type",
>+                             &shadow_type, NULL);
>+        gtk_paint_box(style, drawable, GTK_STATE_PRELIGHT, shadow_type,
>+                      cliprect, gMenuItemWidget, "", rect->x, rect->y,

Isn't the string here supposed to be "menuitem"?
Attachment #141217 - Flags: review?(bryner) → review-
>     if (state->disabled)
>         return GTK_STATE_INSENSITIVE;
>+    else if (state->active)
>+        return GTK_STATE_ACTIVE;
>     else if (state->inHover)
>-        return (state->active ? GTK_STATE_ACTIVE : GTK_STATE_PRELIGHT);
>+        return GTK_STATE_PRELIGHT;
>     else
>         return GTK_STATE_NORMAL;

else-after-return drive-by whaaaahhh!!!

/be
> >     if (state->disabled)
> >         return GTK_STATE_INSENSITIVE;
> >+    else if (state->active)
> >+        return GTK_STATE_ACTIVE;
> >     else if (state->inHover)
> >-        return (state->active ? GTK_STATE_ACTIVE : GTK_STATE_PRELIGHT);
> >+        return GTK_STATE_PRELIGHT;
> >     else
> >         return GTK_STATE_NORMAL;
> > }
> 
> I don't think this is quite right... our 'active' doesn't quite map to gtk's
> 'active'.  I'll take the example of pressing the left mouse button with the
> pointer over a <button>.  In that case, we don't want to draw STATE_ACTIVE
> (which is the button's pressed-in look) if you hold down the button while
> moving the pointer out of the button, even though the css state 'active'
> continues to apply.

From CSS2 5.11.3: "CSS doesn't define which elements may be in the [:hover,
:active,...] states, or how the states are entered and left"
Our css implementation unsets the :active state when the pointer (held down in a
button) goes outside it. So my patch doesn't make any difference, here. Do you
have other examples in which our css implementation doesn't match GTK?

The reason why I changed it (sorry, I wanted to comment on it, but forgot) is
that the current convertGTKState will return GTK_STATE_NORMAL if an element is
in the css state :active with :hover unset. That seemed wrong to me because if a
button has the focus, pressing the enter key should trigger the GTK_STATE_ACTIVE
state even if the mouse is outside the button. But in that case, it boils down
that our current implementation doesn't fail because it looks like the :hover
state is always set when :active is set... That's why my patch doesn't change
the current behaviour for buttons. But as far as I could test, I've seen no
difference in all the native widgets with and without my patch.

Somewhat related to that, in gtkbutton.c, the button shadow type is determined by:
button->depressed ? GTK_SHADOW_IN : GTK_SHADOW_OUT
so, in gtk_button_paint, we should rather have:
    shadow_type = button_state == GTK_STATE_ACTIVE ? GTK_SHADOW_IN : GTK_SHADOW_OUT;
instead of shadow_type = (state->active && state->inHover) ? GTK_SHADOW_IN :
GTK_SHADOW_OUT;

> >+        /* the following line can trigger an assertion (Crux theme)
> >+           file ../../gdk/gdkwindow.c: line 1846 (gdk_window_clear_area):
> >+           assertion `GDK_IS_WINDOW (window)' failed */
> 
> Does this draw correctly despite the assertion?  Would we want to blacklist
> this widget if you're using Crux?

Well, GTK_STATE_NORMAL is fine. That's better than nothing, I'd say. The problem
appears when hovering on buttons. I added this comment because It took some time
to track this assertion down and it may be valuable for further investigations.
It's up to you.

> 
> >+static gint
> >+moz_gtk_menu_bar_paint(GdkDrawable* drawable, GdkRectangle* rect,
> >+                       GdkRectangle* cliprect)
> >+{
> >+    GtkStyle* style;
> >+    GtkShadowType shadow_type;
> >+    ensure_menu_bar_widget();
> >+    style = gMenuBarWidget->style;
> >+    gtk_widget_style_get(gMenuBarWidget, "shadow_type",
> >+                         &shadow_type, NULL);
> 
> We should maybe consider caching the shadow type.  It shouldn't be changing (we
> would have to invalidate it on theme change though).

good point. But in order to not overoptimize, since there should be only one
menubar per window, I'll also provide a patch on top of the new one that reverts
to no caching to be checked in separately and we'll see if it hurts. Is it ok?

> >+    if (state->inHover && !state->disabled) {
> >+        ensure_menu_item_widget();
> >+        gtk_paint_box(style, drawable, GTK_STATE_PRELIGHT, shadow_type,
> >+                      cliprect, gMenuItemWidget, "", rect->x, rect->y,
> 
> Isn't the string here supposed to be "menuitem"?
> 
Not sure what I had in mind. I removed it because it's only used to draw the
menuitem arrows, and this patch doesn't deal with that. But I agree it's only
confusing.

> >     else
> >         return GTK_STATE_NORMAL;
> else-after-return drive-by whaaaahhh!!!

not mine, but I'll fix it.

Attached patch patch v1.2Splinter Review
after discussing with bryner, this patch still modifies convertGtkState to
match our CSS.
This patch caches the shadow_type of the menubar and toolbar widget.
Attachment #141217 - Attachment is obsolete: true
Attachment #142312 - Flags: superreview?(roc)
Attachment #142312 - Flags: review?(bryner)
forgot to tell that I prefer to deal with NS_THEMECHANGED in another bug for the
cached values, since there is also some look and feel cached values that I want
to fix at the same time.
Attachment #142312 - Flags: superreview?(roc) → superreview+
Comment on attachment 142312 [details] [diff] [review]
patch v1.2

Just one comment, I think the css should be #ifdef'd MOZ_WIDGET_GTK2 instead of
XP_UNIX.  XP_UNIX applies to gtk1 and mac os x as well and I don't think we
want to affect those.

r=bryner with those changes.
Attachment #142312 - Flags: review?(bryner) → review+
Just build:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040304 Firefox/0.8.0+

when I hover over menuitems the foreground colours are always white.
err should have been clearer, it seems like the font colour is hardcoded to
white, and does not follow the theme colours.
ok, my bad. What I was seeing was bug 67448. Sorry for the spam
(In reply to comment #1)
> NS_THEME_MENU_BAR   | <menubar>            | -moz-appearance: menu-bar;
> NS_THEME_MENU_POPUP | <menupopup>, <popup> | -moz-appearance: menu-popup;
> NS_THEME_MENU_ITEM  | <menu>, <menuitem>   | -moz-appearance: menu-item;

The patch here did not seem to change seamonkey's css files; but if I understand
comment 1 correctly you made an incompatible change; am I missing something?
(In reply to comment #17)
> pch, did this cause http://forums.mozillazine.org/viewtopic.php?p=414780#414780?
yup, I will create a toolkit/skin/gtk2 directory and add there the files that I
ifdef'd, then I will back out the changes in skin/win.

(In reply to comment #18)
> The patch here did not seem to change seamonkey's css files; but if I understand
> comment 1 correctly you made an incompatible change; am I missing something?

comment#1 is outdated. For the record:
NS_THEME_MENUBAR   | <menubar>            | -moz-appearance: menubar;
NS_THEME_TOOLBAR   | <toolbar>            | -moz-appearance: toolbar;
NS_THEME_MENUPOPUP | <menupopup>, <popup> | -moz-appearance: menupopup;
NS_THEME_MENUITEM  | <menu>, <menuitem>   | -moz-appearance: menuitem;

Then, the appearances menu (removed and replaced by menupopup and menuitem),
menubar, toolbar were implemented in none of our 3 platforms (the mac version
has a 'menu' appearance in its skin but it's not supported. I will deal with it
today). The patch implements the appearances for gtk2 only.
It means that for gtk1, mac and win, this patch won't make any difference for
seamonkey, since the theme are not supported.
For seamonkey-gtk2, it won't change anything as well, since those appearances
were not used in the skin files.

I did it only for gtk2, because gtkdrawing.c has been forked in gtk2drawing.c.
I'll be doing several patches on gtk nsITheme, I can not afford duplicate my
work. We have to move on. If the suite would default to gtk2, I would be glad to
provide the update for the skins but otherwise, I think it's not worth.



Comment on attachment 142312 [details] [diff] [review]
patch v1.2

bah
Attachment #142312 - Flags: superreview+
Attachment #142312 - Flags: review+
Comment on attachment 142312 [details] [diff] [review]
patch v1.2

oops my bad, wrong bug, can someone restore those? Sorry for the spam again.
/me slap himself with huge trout
Comment on attachment 142312 [details] [diff] [review]
patch v1.2

adding back reviews.
Attachment #142312 - Flags: superreview+
Attachment #142312 - Flags: review+
marking fixed because we think most if not all is done.  open new bugs for other
stuff if needed
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Blocks: 243372
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: