Can't move a firefox window by draging the menubar in GTK themes with unified titlebar/menubar

[Yann Dìnendal]

Binary package hint: light-themes

With Ambiance and Radiance, we can move any windows by grabbing the menu bar. Except a Firefox window!
This is causes a big inconsistency: users won't understand why sometimes, they can't drag Firefox windows.

[James Schriver (dashua)]

It's a GTK+ patch so you're not going to be able to drag non-GTK applications, Firefox, Open Office, Thunderbird, etc., by the titlebar. I am not sure if there is a workaround for this, but I would assume probably not.

[Vish (vish)]

To fix it in the theme we'd need a fix/patch/hack in firefox.

[Connor Carney (cscarney)]

It's an issue for all non-GTK apps, including Firefox/Thunderbird/OpenOffice. Considering how much time a default Ubuntu user spends in Firefox+OpenOffice this really is a glaringly obvious UI inconsistency.

[Micah Gersten (micahg)]

Firefox and Thunderbird are both GTK apps.

On 04/04/2010 03:05 PM, Connor Carney wrote:
> It's an issue for all non-GTK apps, including
> Firefox/Thunderbird/OpenOffice. Considering how much time a default
> Ubuntu user spends in Firefox+OpenOffice this really is a glaringly
> obvious UI inconsistency.
>
>

[Connor Carney (cscarney)]

Hmm, I didn't know that. They certainly don't use GTK menu bars, which seems to be the problem here.

[L. David Baron (dbaron)]

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.)

[L. David Baron (dbaron)]

I think this would be relatively straightforward to fix in Firefox; we already have code for this behavior for Mac (although for Mac we can move the window by dragging even more area than we'd want for these GTK themes: on Mac, empty parts of the toolbars and statusbar can be dragged too).

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=566480 on fixing this. (I thought there was a way to link that to this bug in the UI, but I don't see it.)

However, I don't see how to detect when the current GTK theme has this unified titlebar+menubar behavior. I don't see any obviously-related code in gtkmenubar.c (after a quick skim of the file), and I'm not sure where else to look, or what terms to look for.

[L. David Baron (dbaron)]

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

[Ventnor-bugzilla (ventnor-bugzilla)]

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!)

[Ventnor-bugzilla (ventnor-bugzilla)]

Created an attachment (id=446645)
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.

[L. David Baron (dbaron)]

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.

[Ventnor-bugzilla (ventnor-bugzilla)]

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).

[Roc-ocallahan (roc-ocallahan)]

(From update of attachment 446645)
Probably better for dbaron to review the style bits and Karl to review the GTK bits

[Karlt (karlt)]

(From update of attachment 446645)
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.

[Karlt (karlt)]

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

[L. David Baron (dbaron)]

(From update of attachment 446645)
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

[Ventnor-bugzilla (ventnor-bugzilla)]

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

[L. David Baron (dbaron)]

OK, I'll work on the revisions.

[L. David Baron (dbaron)]

Created an attachment (id=454370)
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.

[L. David Baron (dbaron)]

(From update of attachment 454370)
also requesting review from Dão on the toolkit/ changes

[L. David Baron (dbaron)]

(From update of attachment 454370)
Also, I wanted to ask jst if nsIDOMChromeWindow is an appropriate place for this.

[Dao (dao)]

(From update of attachment 454370)
>--- 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?

[L. David Baron (dbaron)]

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.

[L. David Baron (dbaron)]

(Try server was happy with the above patch.)

[Dao (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.

[L. David Baron (dbaron)]

Created an attachment (id=454689)
patch

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

[Dao (dao)]

(From update of attachment 454689)
>--- 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

[Dao (dao)]

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

[Karlt (karlt)]

(From update of attachment 454689)
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.

[Jst (jst)]

(From update of attachment 454689)
Looks fine to me.

[Dao (dao)]

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

[Daniel C (djcater)]

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'

[Karlt (karlt)]

Comment on attachment 454689
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().

[Mh+mozilla (mh+mozilla)]

> 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

[Karlt (karlt)]

Thanks, Mike. Filed bug 580779.

[Micah Gersten (micahg)]

Marking bug as triaged since we have an upstream bug. It's fixed for Firefox 4.0

[Micah Gersten (micahg)]

This is fixed in Natty in Firefox 4.0b7. Please report any other issues you may find.