566480 - In GTK themes with unified titlebar/menubar, dragging menubar should drag window

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The default theme on Ubuntu 10.04 has a unified titlebar/menubar appearance and behavior.  We pick up the appearance perfectly, but we don't get the behavior right.

In particular: there's no visual distinction between the titlebar of the window and the menubar.  However, it just so happens that the window manager draws the titlebar and we draw the menubar.  In other GNOME apps, dragging the titlebar or the parts of the menubar without any menus has the same behavior: it drags the Window.  In Firefox, however, dragging the menubar doesn't do anything (so there's an invisible boundary line between an area that works and an area that doesn't).


Fortunately, we already have code to do pretty much what we want, because it's needed to support the unified toolbar appearance on Mac:  http://hg.mozilla.org/mozilla-central/rev/b66468507a59

We just need to figure out how to detect whether the current GTK theme has the unified toolbar/menubar appearance and enable this behavior when it does.  I'm not sure how to detect this.  (I didn't see anything obviously related to this behavior in gtkmenubar.c in the GTK source, and I'm not sure where else to look.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, I think I found the relevant bit in the theme:
/usr/share/themes/Ambiance/gtk-2.0/gtkrc has the line:
        GtkMenuBar::window-dragging = 1

Comment 2

[Michael Ventnor]

This will require a bit more than a simple hookup to the dragging utils, since we need to allow the window manager to handle drags (Compiz wobbly windows FTW!)

Comment 3

[Michael Ventnor]

Attachment: Patch

Leaving myself a note here: only ever use -moz-binding in global.css when in OS theme files. Doing it elsewhere (like toolbar.css) will cause a strange bug where it will work the first time you run after recompiling, but not afterwards. You wouldn't believe how long it took me to debug that.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, one thing I'd note is that, in theory, any GTK widget could have "window-dragging" set.  So if we wanted to support it generally, we'd probably want to be doing this via nsITheme rather than nsILookAndFeel.  But that would be a good bit more work, and this seems sufficient for the immediate problem.

Comment 5

[Michael Ventnor]

I took this approach because I tried to reuse the WindowDraggingUtils code already there. In the case of generic toolbars, it shouldn't be any more difficult than another system metric plus a dab of extra CSS. I don't know of any themes so far that have completely unified toolbars or whether it does anything beyond GtkToolbar::window-dragging (such as ID's or ancestry checks).

Comment 6

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

Comment on attachment 446645 [details] [diff] [review]
Patch

Probably better for dbaron to review the style bits and Karl to review the GTK bits

Comment 7

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 446645 [details] [diff] [review]
Patch

In principal, this looks sensible to me.

>+    /**
>+     * Begin a window moving drag. We don't need an event.
>+     */
>+    NS_IMETHOD BeginMoveDrag() = 0;

>+    // get the current pointer position and button state
>+    GdkScreen* screen = NULL;
>+    gint screenX, screenY;
>+    GdkModifierType mask;
>+    gdk_display_get_pointer(display, &screen, &screenX, &screenY, &mask);
>+
>+    // we only support moving with button 1
>+    if (!(mask & GDK_BUTTON1_MASK)) {
>+        return NS_ERROR_FAILURE;
>+    }
>+
>+    // tell the window manager to start the move
>+    gdk_window_begin_move_drag(gdk_window, 1, screenX, screenY, GDK_CURRENT_TIME);

This really does need an event for the root window coordinates and timestamp.
If the user is dragging, the coordinates at the time of processing will often
differ from those of the ButtonPress.  And a window drag is something that we
should be careful not to initiate on the wrong button press.

nsMouseEvents really deserve root window coordinates, but the easiest way to
implement this might be by setting nsGUIEvent::pluginEvent.  BeginMoveDrag can
fail for synthetic events and WindowDraggingUtils will use the fallback code.

(I see that
http://standards.freedesktop.org/wm-spec/1.4/ar01s04.html#id2568642 adds
_NET_WM_MOVERESIZE_CANCEL, but GDK doesn't yet support it, so I guess we won't
worry about that.)

>+    // get the gdk window for this widget
>+    GdkWindow* gdk_window = mGdkWindow;
>+    if (!GDK_IS_WINDOW(gdk_window)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+

Only test mGdkWindow against NULL.  It won't be any other GObject type.

>+    gdk_window = gdk_window_get_toplevel(gdk_window);
>+    if (!GDK_IS_WINDOW(gdk_window)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+

No need to check the result.
It won't be NULL because the parameter was not NULL.

Comment 8

[Karl Tomlinson (:karlt back Feb 10)]

(pluginEvent was actually called nativeMsg before it was changed in bug 527617.)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 446645 [details] [diff] [review]
Patch

r=dbaron on the layout/ and content/ changes, except I think you want to check with jst on whether nsIDOMChromeWindow is the right place for this to be

Comment 10

[Michael Ventnor]

Sorry that I haven't been active on this lately. If anyone else wants to take this up they're free to do so.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I'll work on the revisions.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I think this should address all of karlt's comments except for storing root-relative coordinates on the event.  While it seems like that would be nice, I think the chance of the widget moving between the mousedown and the time we process it doesn't seem like an important case.  (I could imagine it happening if, e.g., script moves or resizes a window, though.)

This does change the existing resize-drag processing to use the coordinates from the event.

I tested the basic usage of both resize and move drags on the Firefox window, and checked that in both cases we're getting through to calling gdk_window_begin_*_drag.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 454370 [details] [diff] [review]
patch

also requesting review from Dão on the toolkit/ changes

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 454370 [details] [diff] [review]
patch

Also, I wanted to ask jst if nsIDOMChromeWindow is an appropriate place for this.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 454370 [details] [diff] [review]
patch

>--- a/toolkit/content/WindowDraggingUtils.jsm
>+++ b/toolkit/content/WindowDraggingUtils.jsm
>@@ -65,26 +65,36 @@ WindowDraggingElement.prototype = {
>             break;
>           parent = parent.parentNode;
>         }
>         while (target != this._elem) {
>           if (this.dragTags.indexOf(target.localName) == -1)
>             return;
>           target = target.parentNode;
>         }
>+
>+        // On GTK at least, there is a toolkit-level function which handles
>+        // window dragging, which must be used.
>+        try {
>+          this._window.beginWindowMove(aEvent);
>+          return;
>+        } catch (e) {}

What exactly is this going to catch? Should this call be conditional on MOZ_WIDGET_GTK2?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It'll catch the function not being implemented (since the default implementation in nsBaseWidget throws), but allow other platforms to implement it without changing that code.  Or it could just be #ifdef MOZ_WIDGET_GTK2.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Try server was happy with the above patch.)

Comment 18

[Dão Gottwald [:dao]]

Unless it's expected that beginWindowMove would sometimes throw on a platform that should support it, I think we want this:

#ifdef MOZ_WIDGET_GTK2
  this._window.beginWindowMove(aEvent);
#else
  [rest of the function]
#endif

We'd just update the ifdef when another platform implements this.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Updated to #ifdef, per Dão's comments.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 454689 [details] [diff] [review]
patch

>--- a/toolkit/content/WindowDraggingUtils.jsm
>+++ b/toolkit/content/WindowDraggingUtils.jsm

>+#ifdef MOZ_WIDGET_GTK2
>+        // On GTK, there is a toolkit-level function which handles
>+        // window dragging, which must be used.
>+        this._window.beginWindowMove(aEvent);
>+#else
>         this._deltaX = aEvent.screenX - this._window.screenX;
>         this._deltaY = aEvent.screenY - this._window.screenY;
>         this._draggingWindow = true;
>         this._window.addEventListener("mousemove", this, false);
>         this._window.addEventListener("mouseup", this, false);
>+#endif
>         break;
>       case "mousemove":
>         if (this._draggingWindow)
>           this._window.moveTo(aEvent.screenX - this._deltaX, aEvent.screenY - this._deltaY);
>         break;
>       case "mouseup":
>-        this._draggingWindow = false;
>-        this._window.removeEventListener("mousemove", this, false);
>-        this._window.removeEventListener("mouseup", this, false);
>+        if (this._draggingWindow) {
>+          this._draggingWindow = false;
>+          this._window.removeEventListener("mousemove", this, false);
>+          this._window.removeEventListener("mouseup", this, false);
>+        }
>         break;

The change in the mouseup case looks unnecessary, as the mouseup listener is only added when _draggingWindow is set to true. Anyway, Enn really needs to review this, not only because he owns this code, but because he's actually touching these lines in a conflicting way in bug 552982.

r=me on the global.css change

Comment 21

[Dão Gottwald [:dao]]

Also note that bug 555081 did something similar for Windows by implementing a "MozMouseHittest" event.

Comment 22

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 454689 [details] [diff] [review]
patch

r+ on the widget parts.

(In reply to comment #21)
> Also note that bug 555081 did something similar for Windows by implementing a
> "MozMouseHittest" event.

Bug 555081 comment 55 suggests that MozMouseHitTest is really only necessary
because of MS APIs.

Using "mousedown" seems reasonable to me, and, I suspect
it may avoid mouse capture issues.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 454689 [details] [diff] [review]
patch

Looks fine to me.

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b00bb3a115de

Comment 25

[Daniel C]

Mozilla/5.0 (X11; Linux i686; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

gtk+ 2.18.9
Clearlooks theme

When starting Minefield I now get this printed to the console:

(firefox-bin:5229): Gtk-WARNING **: gtkwidget.c:9567: widget class `GtkMenuBar' has no property named `window-dragging'

Comment 26

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 454689 [details] [diff] [review]
patch

>+    // Some themes have a unified menu bar, and support window dragging on it
>+    gboolean supports_menubar_drag = FALSE;
>+    gtk_widget_style_get(menuBar,
>+                         "window-dragging", &supports_menubar_drag,
>+                         NULL);

I can't see window-dragging as a style property in gtk+ 2.20.1.
Is it an Ubuntu customization?
Or can a theme install style properties (as well as just setting them)?

file:///usr/share/gtk-doc/html/gtk/GtkWidget.html#GtkWidget.style-properties
http://library.gnome.org/devel/gtk/stable/GtkMenuBar.html#GtkMenuBar.style-properties

Given that this is not a standard property, its type is not guaranteed, so I guess it is not always safe to use gtk_widget_style_get() with its varargs.

Sounds like we need gtk_widget_class_find_style_property(), and either confirm that it is boolean or use gtk_widget_style_get_property().

Comment 27

[Mike Hommey [:glandium]]

> I can't see window-dragging as a style property in gtk+ 2.20.1.
> Is it an Ubuntu customization?

Apparently a Fedora patch:
http://cvs.fedoraproject.org/viewvc/rpms/gtk2/devel/window-dragging.patch?revision=1.1&view=markup&sortby=date

Comment 28

[Karl Tomlinson (:karlt back Feb 10)]

Thanks, Mike.  Filed bug 580779.