Created attachment 85456 [details] [review]
Patch fixing the above

Comment on attachment 85456 [details] [review]
Patch fixing the above

>+static GtkWidget *
>+gtk_menu_find_item (GdkEventButton *event)
>+{
>+  GtkWidget *menu_item;
>+
>+  menu_item = gtk_get_event_widget ((GdkEvent*) event);
>+
>+  while (menu_item && !GTK_IS_MENU_ITEM (menu_item))
>+    menu_item = menu_item->parent;

this code checking for menu items in combination with the code calling it below restricts clickable widgets in menus to menu items. i'm not sure that this is generally a good idea (e.g. check buttons could conceivably be used in menus at some point).

>+
>+  return menu_item;
>+}
>+
>+static gboolean
>+pointer_in_menu_tree (GtkWidget *widget)
>+{
>+  GtkMenuShell *menu_shell;
>+  gint x, y, width, height;
>+
>+  menu_shell = GTK_MENU_SHELL (widget);
>+
>+  gdk_window_get_pointer (widget->window, &x, &y, NULL);
>+  gdk_drawable_get_size (widget->window, &width, &height);
>+
>+  if ((x >= 0) && (x < width) && (y >= 0) && (y < height))
>+    return TRUE;

this code looks ok, except that the parenthesis around the comparisons are bogus, && and || were introduced to have a much lower precedence than comparisons (they are not broken wrg precedence like & and | are).

>+
>+  if (GTK_IS_MENU (menu_shell->parent_menu_shell))
>+    return pointer_in_menu_tree (menu_shell->parent_menu_shell);
>+
>+  return FALSE;
>+}
>+
> static gboolean
> gtk_menu_button_scroll (GtkMenu        *menu,
>                         GdkEventButton *event)
>@@ -2586,11 +2640,20 @@ gtk_menu_button_press (GtkWidget      *w
>   if (event->type != GDK_BUTTON_PRESS)
>     return FALSE;
> 
>-  /* Don't pop down the menu for presses over scroll arrows
>+  /* Don't pass down to menu shell for presses over scroll arrows
>    */
>   if (gtk_menu_button_scroll (GTK_MENU (widget), event))
>     return TRUE;
> 
>+  if (!gtk_menu_find_item (event))

hm, like i said above i don't like this approach very much.
this code is pro-actively searching for a set of widgets it considers worth processing events. and that's all done just to ignore button presses on window borders? i'd much rather have the code process events as normal and once not other widget processed the event, check whether the click was outside or inside a menu window.
do you think you can reform the patch towards that?

>+    {
>+      /*  Don't pass down to menu shell if a non-menuitem part of the menu
>+       *  was clicked.
>+       */
>+      if (pointer_in_menu_tree (widget))
>+        return TRUE;
>+    }
>+
>   return GTK_WIDGET_CLASS (gtk_menu_parent_class)->button_press_event (widget, event);
> }

Right, checking for menu items only is wrong. I'll also fix the useless
parenthesis.

However chaining up to evil code like gtk_menu_shell_button_press,release()
first and then checking if we clicked the border is something I'd rather
like to avoid.

What about checking if gtk_get_event_widget() returns a GtkMenuShell
(which would mean we either clicked outside any menu or on a menu border)?
That would simplify the code quite a bit and avoid dealing with the
return value of weird GtkMenuShell handlers.

Created attachment 85505 [details] [review]
Updated patch

- doesn't walk the widget hierarchy.
- doesn't have any GtkMenuItem special casing.
- doesn't cause any server roundtrip

Oops, one of the "window_x" in the last patch should be "window_y". Fixed
in my tree, I won't upload a new patch just because of this.

Comment on attachment 85505 [details] [review]
Updated patch

> static gboolean
>+pointer_in_menu_tree (GtkWidget *widget,

this should be named pointer_in_menu_window() now, since that's what the new code checks and it also is a way better check.

>+                      gdouble    x_root,
>+                      gdouble    y_root)
>+{
>+  GtkMenu *menu = GTK_MENU (widget);
>+
>+  if (GTK_WIDGET_MAPPED (menu->toplevel))
>+    {
>+      GtkMenuShell *menu_shell;
>+      gint          window_x, window_y;
>+
>+      gdk_window_get_position (menu->toplevel->window, &window_x, &window_y);
>+
>+      if (x_root >= window_x && x_root < window_x + widget->allocation.width &&
>+          y_root >= window_y && y_root < window_x + widget->allocation.height)

i see, heres the x/y you caught already.

>+        return TRUE;
>+
>+      menu_shell = GTK_MENU_SHELL (widget);
>+
>+      if (GTK_IS_MENU (menu_shell->parent_menu_shell))
>+        return pointer_in_menu_tree (menu_shell->parent_menu_shell,
>+                                     x_root, y_root);
>+    }
>+
>+  return FALSE;
>+}
>+
>+static gboolean
> gtk_menu_button_press (GtkWidget      *widget,
>                        GdkEventButton *event)
> {
>   if (event->type != GDK_BUTTON_PRESS)
>     return FALSE;
> 
>-  /* Don't pop down the menu for presses over scroll arrows
>+  /* Don't pass down to menu shell for presses over scroll arrows
>    */
>   if (gtk_menu_button_scroll (GTK_MENU (widget), event))
>     return TRUE;
> 
>+  if (GTK_IS_MENU_SHELL (gtk_get_event_widget ((GdkEvent *) event)))

i don't think this extra check should be here. pointer_in_menu_window() should work without it, and for some cases it'll wrongly supress pointer_in_menu_window() checks (e.g. when clicking on the area created by window->border_width = 20).

>+    {
>+      /*  Don't pass down to menu shell if a non-menuitem part of the menu
>+       *  was clicked.
>+       */
>+      if (pointer_in_menu_tree (widget, event->x_root, event->y_root))
>+        return TRUE;
>+    }
>+
>   return GTK_WIDGET_CLASS (gtk_menu_parent_class)->button_press_event (widget, event);
> }

Ok, pointer_in_menu_window() is better i guess.

The check for GTK_IS_MENU_SHELL() is IMHO correct to catch any
click on a non-child area of the window or outside. I've tested
both setting the window's border_width to != 0 and to increase
the menu's padding, both work fine.

(In reply to comment #7)
> Ok, pointer_in_menu_window() is better i guess.
> 
> The check for GTK_IS_MENU_SHELL() is IMHO correct to catch any
> click on a non-child area of the window or outside. I've tested
> both setting the window's border_width to != 0 and to increase
> the menu's padding, both work fine.

ok, after a bit of IM discussion it seems you're right. the reason it works is that because gtkmenu.c grabs with grab_window=menu_shell->window and owner_events=TRUE, gtk_get_event_widget() will always either return a child widget of menu_shell, or the menu_shell itself (events outside will be reported menu_shell->window relative).
so thanks for hashing this out, since you've also done proper testing i think this can go in now (adding the explanation i just gave as a comment wouldn't hurt though).

Fixed in trunk:

2007-03-29  Michael Natterer  <mitch@imendio.com>

	Don't close menus on clicks on their border area (bug #423761).
	(modified patch from maemo-gtk).

	* gtk/gtkmenu.c (gtk_menu_button_press)
	(gtk_menu_button_release): bail out early if the click was on the
	menu's border (not on any item and not outside the window).

	(pointer_in_menu_window): new utility function which checks if
	passed root coords are inside the menu_shell or one of its
	parent shells.