Bug 218562 – [GTK][Regression][2.30] Application cannot override drag&drop callbacks

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Milan Crha]

Downstream bug report:
https://gitlab.gnome.org/GNOME/evolution/-/issues/1200

When dragging files from Nautilus above a message body in Evolutions' composer window (which is a WebKitWebView), the composer code listens for "drag-drop" and other signals and it can override what to do when certain types are included in those provided by the source widget/application. These callbacks are installed in the composer's GObject::constructed() method, after this method calls parent's constructed(). I think, and only think, that glib is processing multiple callbacks in the opposite order than in which they had been connected (+/- _after function/flag).

It looks like WebKitGTK adds its callbacks after those which Evolution added, thus they are called first and they pick text/plain format, which is the URI of the file, not the text/uri-list, which handles Evolution on its own.

I can (kind of) confirm my theory, because when I postpone signal connect in Evolution by one second it works as expected again, callbacks from the Evolution are called before callbacks from the WebKitGTK.

Eventually, this can be related to bug #218462, even I'm able to reproduce this regardless whether running Wayland or X11.

[Milan Crha]

Created attachment 413169 [details]
proposed patch

This fixes it for me. Three changes are included, all are needed:

1) use g_signal_connect_after(), thus the descendants can get before that callback, as it used to be when the d&d callbacks had been assigned as GtkWidgetClass members;

2) update the inner m_drop context whenever it's different from the passed-in context in the "drag-motion" handler, otherwise one cannot drag the second time, because the "drag-leave" was not called (it is not called here);

3) in the DropTarget::didPerformAction() always call gdk_drag_status(), otherwise the drag can look like being frozen.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

(In reply to Milan Crha from comment #1)
> 1) use g_signal_connect_after(), thus the descendants can get before that
> callback, as it used to be when the d&d callbacks had been assigned as
> GtkWidgetClass members;

Good.

> 2) update the inner m_drop context whenever it's different from the
> passed-in context in the "drag-motion" handler, otherwise one cannot drag
> the second time, because the "drag-leave" was not called (it is not called
> here);

Odd... seems like drag-leave ought to be called?

> 3) in the DropTarget::didPerformAction() always call gdk_drag_status(),
> otherwise the drag can look like being frozen.

OK, good.

[Milan Crha]

(In reply to Michael Catanzaro from comment #3)
> Odd... seems like drag-leave ought to be called?

Right, it should be called, according to the gtk+ documentation, but it's not called for some reason. I didn't try to figure out why. In any case, the system can have "running" only a single drag operation in the time, thus the idea of always updating the m_drop when it doesn't match the current context makes sense.

[Michael Catanzaro]

Comment on attachment 413169 [details]
proposed patch

OK, all this makes sense to me. I'm not giving cq+ yet, though, because Carlos Garcia will want to review this before it lands, so let's allow some time for that.

If we commit this, we should also remember to close bug #218462.

[Carlos Garcia Campos]

Comment on attachment 413169 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413169&action=review

> Source/WebKit/UIProcess/API/gtk/DropTargetGtk3.cpp:59
> -    g_signal_connect(m_webView, "drag-motion", G_CALLBACK(+[](GtkWidget*, GdkDragContext* context, gint x, gint y, guint time, gpointer userData) -> gboolean {
> +    g_signal_connect_after(m_webView, "drag-motion", G_CALLBACK(+[](GtkWidget*, GdkDragContext* context, gint x, gint y, guint time, gpointer userData) -> gboolean {

I guess this is not needed in GTK4, because apps wanting to override this can use the capture phase on GtkDropTargetAsync?

[Carlos Garcia Campos]

*** Bug 218163 has been marked as a duplicate of this bug. ***

[EWS]

Committed r269505: <https://trac.webkit.org/changeset/269505>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413169 [details].

[Milan Crha]

(In reply to Carlos Garcia Campos from comment #6)
> I guess this is not needed in GTK4, because apps wanting to override this
> can use the capture phase on GtkDropTargetAsync?

I do not know gtk4 at all, but my quick check on what WebKit does there didn't show anything relevant (the signals being attached to are very different from those in gtk3).

[Carlos Garcia Campos]

*** Bug 216828 has been marked as a duplicate of this bug. ***