Closed Bug 566480 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Widget: Gtk, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: dbaron, Assigned: ventnor.bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

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.)
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
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!)
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #446645 - Flags: review?(roc)
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.
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 on attachment 446645 [details] [diff] [review]
Patch

Probably better for dbaron to review the style bits and Karl to review the GTK bits
Attachment #446645 - Flags: review?(roc)
Attachment #446645 - Flags: review?(karlt)
Attachment #446645 - Flags: review?(dbaron)
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.
Attachment #446645 - Flags: review?(karlt) → review-
(pluginEvent was actually called nativeMsg before it was changed in bug 527617.)
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
Attachment #446645 - Flags: review?(dbaron) → review+
Sorry that I haven't been active on this lately. If anyone else wants to take this up they're free to do so.
OK, I'll work on the revisions.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #446645 - Attachment is obsolete: true
Attachment #454370 - Flags: review?
Attachment #454370 - Flags: review? → review?(karlt)
Comment on attachment 454370 [details] [diff] [review]
patch

also requesting review from Dão on the toolkit/ changes
Attachment #454370 - Flags: review?(dao)
Comment on attachment 454370 [details] [diff] [review]
patch

Also, I wanted to ask jst if nsIDOMChromeWindow is an appropriate place for this.
Attachment #454370 - Flags: feedback?(jst)
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?
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.
(Try server was happy with the above patch.)
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.
Attached patch patchSplinter Review
Updated to #ifdef, per Dão's comments.
Attachment #454370 - Attachment is obsolete: true
Attachment #454689 - Flags: review?(dao)
Attachment #454689 - Flags: feedback?(jst)
Attachment #454370 - Flags: review?(karlt)
Attachment #454370 - Flags: review?(dao)
Attachment #454370 - Flags: feedback?(jst)
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
Attachment #454689 - Flags: review?(enndeakin)
Attachment #454689 - Flags: review?(dao)
Attachment #454689 - Flags: review+
Also note that bug 555081 did something similar for Windows by implementing a "MozMouseHittest" event.
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.
Attachment #454689 - Flags: review?(karlt) → review+
Comment on attachment 454689 [details] [diff] [review]
patch

Looks fine to me.
Attachment #454689 - Flags: feedback?(jst) → feedback+
Attachment #454689 - Flags: review?(enndeakin) → review+
Component: Toolbars and Toolbar Customization → Widget: Gtk
Keywords: checkin-needed
Product: Toolkit → Core
QA Contact: toolbars → gtk
http://hg.mozilla.org/mozilla-central/rev/b00bb3a115de
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
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 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().
> 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
Thanks, Mike.  Filed bug 580779.
You need to log in before you can comment on or make changes to this bug.