218562 2020-11-04 06:08:27 -0800 [GTK][Regression][2.30] Application cannot override drag&drop callbacks 2020-11-06 03:29:58 -0800 1 1 1 Unclassified WebKit WebKitGTK Other Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mcrha webkit-unassigned amarok berto bugs-noreply cgarcia ews-watchlist gustavo ht990332 mcatanzaro nekohayo oldest_to_newest 1704382 0 mcrha 2020-11-04 06:08:27 -0800 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. 1704455 1 413169 mcrha 2020-11-04 09:01:15 -0800 Created attachment 413169 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. 1704456 2 ews-watchlist 2020-11-04 09:02:04 -0800 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 1704462 3 mcatanzaro 2020-11-04 09:20:06 -0800 (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. 1704781 4 mcrha 2020-11-05 01:31:12 -0800 (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. 1704836 5 413169 mcatanzaro 2020-11-05 06:19:32 -0800 Comment on attachment 413169 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. 1705156 6 413169 cgarcia 2020-11-06 00:59:02 -0800 Comment on attachment 413169 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? 1705158 7 cgarcia 2020-11-06 01:00:18 -0800 *** Bug 218163 has been marked as a duplicate of this bug. *** 1705163 8 ews-feeder 2020-11-06 01:09:35 -0800 Committed r269505: <https://trac.webkit.org/changeset/269505> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413169. 1705166 9 mcrha 2020-11-06 01:16:42 -0800 (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). 1705183 10 cgarcia 2020-11-06 03:29:58 -0800 *** Bug 216828 has been marked as a duplicate of this bug. *** 413169 2020-11-04 09:01:15 -0800 2020-11-06 01:09:36 -0800 proposed patch wk.patch text/plain 3925 mcrha ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggYjMwNjgwNmJkY2MuLmRkMmJlNjBkYmFkIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZwpAQCAtMSwzICsx LDE2IEBACisyMDIwLTExLTA0ICBNaWxhbiBDcmhhICA8bWNyaGFAcmVkaGF0LmNvbT4KKworICAg ICAgICBbR1RLXSBBcHBsaWNhdGlvbiBjYW5ub3Qgb3ZlcnJpZGUgZHJhZyZkcm9wIGNhbGxiYWNr cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE4NTYy CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVSVBy b2Nlc3MvQVBJL2d0ay9Ecm9wVGFyZ2V0R3RrMy5jcHA6CisgICAgICAgIChXZWJLaXQ6OkRyb3BU YXJnZXQ6OkRyb3BUYXJnZXQpOiBVc2UgZ19zaWduYWxfY29ubmVjdF9hZnRlcigpLCB0aHVzCisg ICAgICAgIGFueSBkZXNjZW5kYW50cyBjYW4gb3ZlcnJpZGUgdGhlIGNhbGxiYWNrcy4KKyAgICAg ICAgKFdlYktpdDo6RHJvcFRhcmdldDo6ZGlkUGVyZm9ybUFjdGlvbik6IEFsd2F5cyBjYWxsIGdk a19kcmFnX3N0YXR1cygpLAorICAgICAgICB0byBoYXZlIGd0aysgbm90aWZpZWQgYWJvdXQgZHJh ZyBwcm9ncmVzcy4KKwogMjAyMC0xMS0wMyAgU2lodWkgTGl1ICA8c2lodWlfbGl1QGFwcGxlLmNv bT4KIAogICAgICAgICBTZXQgdXAgYmFzaWMgaW5mcmFzdHJ1Y3R1cmUgZm9yIFNwZWVjaFJlY29n bml0aW9uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9BUEkvZ3RrL0Ryb3BU YXJnZXRHdGszLmNwcCBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9ndGsvRHJvcFRhcmdl dEd0azMuY3BwCmluZGV4IDNmZDQxMmU4ZjQyLi5lYTIyYmNkODNmMiAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL2d0ay9Ecm9wVGFyZ2V0R3RrMy5jcHAKKysrIGIvU291 cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL2d0ay9Ecm9wVGFyZ2V0R3RrMy5jcHAKQEAgLTU2LDkg KzU2LDkgQEAgRHJvcFRhcmdldDo6RHJvcFRhcmdldChHdGtXaWRnZXQqIHdlYlZpZXcpCiAgICAg ICAgIHN0YXRpY19jYXN0PEdka0RyYWdBY3Rpb24+KEdES19BQ1RJT05fQ09QWSB8IEdES19BQ1RJ T05fTU9WRSB8IEdES19BQ1RJT05fTElOSykpOwogICAgIGd0a19kcmFnX2Rlc3Rfc2V0X3Rhcmdl dF9saXN0KG1fd2ViVmlldywgbGlzdC5nZXQoKSk7CiAKLSAgICBnX3NpZ25hbF9jb25uZWN0KG1f d2ViVmlldywgImRyYWctbW90aW9uIiwgR19DQUxMQkFDSygrW10oR3RrV2lkZ2V0KiwgR2RrRHJh Z0NvbnRleHQqIGNvbnRleHQsIGdpbnQgeCwgZ2ludCB5LCBndWludCB0aW1lLCBncG9pbnRlciB1 c2VyRGF0YSkgLT4gZ2Jvb2xlYW4geworICAgIGdfc2lnbmFsX2Nvbm5lY3RfYWZ0ZXIobV93ZWJW aWV3LCAiZHJhZy1tb3Rpb24iLCBHX0NBTExCQUNLKCtbXShHdGtXaWRnZXQqLCBHZGtEcmFnQ29u dGV4dCogY29udGV4dCwgZ2ludCB4LCBnaW50IHksIGd1aW50IHRpbWUsIGdwb2ludGVyIHVzZXJE YXRhKSAtPiBnYm9vbGVhbiB7CiAgICAgICAgIGF1dG8mIGRyb3AgPSAqc3RhdGljX2Nhc3Q8RHJv cFRhcmdldCo+KHVzZXJEYXRhKTsKLSAgICAgICAgaWYgKCFkcm9wLm1fZHJvcCkgeworICAgICAg ICBpZiAoZHJvcC5tX2Ryb3AgIT0gY29udGV4dCkgewogICAgICAgICAgICAgZHJvcC5tX2Ryb3Ag PSBjb250ZXh0OwogICAgICAgICAgICAgZHJvcC5tX3Bvc2l0aW9uID0gSW50UG9pbnQoeCwgeSk7 CiAgICAgICAgICAgICBkcm9wLmFjY2VwdCh0aW1lKTsKQEAgLTY3LDE0ICs2NywxNCBAQCBEcm9w VGFyZ2V0OjpEcm9wVGFyZ2V0KEd0a1dpZGdldCogd2ViVmlldykKICAgICAgICAgcmV0dXJuIFRS VUU7CiAgICAgfSksIHRoaXMpOwogCi0gICAgZ19zaWduYWxfY29ubmVjdChtX3dlYlZpZXcsICJk cmFnLWxlYXZlIiwgR19DQUxMQkFDSygrW10oR3RrV2lkZ2V0KiwgR2RrRHJhZ0NvbnRleHQqIGNv bnRleHQsIGd1aW50IHRpbWUsIGdwb2ludGVyIHVzZXJEYXRhKSB7CisgICAgZ19zaWduYWxfY29u bmVjdF9hZnRlcihtX3dlYlZpZXcsICJkcmFnLWxlYXZlIiwgR19DQUxMQkFDSygrW10oR3RrV2lk Z2V0KiwgR2RrRHJhZ0NvbnRleHQqIGNvbnRleHQsIGd1aW50IHRpbWUsIGdwb2ludGVyIHVzZXJE YXRhKSB7CiAgICAgICAgIGF1dG8mIGRyb3AgPSAqc3RhdGljX2Nhc3Q8RHJvcFRhcmdldCo+KHVz ZXJEYXRhKTsKICAgICAgICAgaWYgKGRyb3AubV9kcm9wICE9IGNvbnRleHQpCiAgICAgICAgICAg ICByZXR1cm47CiAgICAgICAgIGRyb3AubGVhdmUoKTsKICAgICB9KSwgdGhpcyk7CiAKLSAgICBn X3NpZ25hbF9jb25uZWN0KG1fd2ViVmlldywgImRyYWctZHJvcCIsIEdfQ0FMTEJBQ0soK1tdKEd0 a1dpZGdldCosIEdka0RyYWdDb250ZXh0KiBjb250ZXh0LCBnaW50IHgsIGdpbnQgeSwgZ3VpbnQg dGltZSwgZ3BvaW50ZXIgdXNlckRhdGEpIC0+IGdib29sZWFuIHsKKyAgICBnX3NpZ25hbF9jb25u ZWN0X2FmdGVyKG1fd2ViVmlldywgImRyYWctZHJvcCIsIEdfQ0FMTEJBQ0soK1tdKEd0a1dpZGdl dCosIEdka0RyYWdDb250ZXh0KiBjb250ZXh0LCBnaW50IHgsIGdpbnQgeSwgZ3VpbnQgdGltZSwg Z3BvaW50ZXIgdXNlckRhdGEpIC0+IGdib29sZWFuIHsKICAgICAgICAgYXV0byYgZHJvcCA9ICpz dGF0aWNfY2FzdDxEcm9wVGFyZ2V0Kj4odXNlckRhdGEpOwogICAgICAgICBpZiAoZHJvcC5tX2Ry b3AgIT0gY29udGV4dCkgewogICAgICAgICAgICAgZ3RrX2RyYWdfZmluaXNoKGNvbnRleHQsIEZB TFNFLCBGQUxTRSwgdGltZSk7CkBAIC04NCw3ICs4NCw3IEBAIERyb3BUYXJnZXQ6OkRyb3BUYXJn ZXQoR3RrV2lkZ2V0KiB3ZWJWaWV3KQogICAgICAgICByZXR1cm4gVFJVRTsKICAgICB9KSwgdGhp cyk7CiAKLSAgICBnX3NpZ25hbF9jb25uZWN0KG1fd2ViVmlldywgImRyYWctZGF0YS1yZWNlaXZl ZCIsIEdfQ0FMTEJBQ0soK1tdKEd0a1dpZGdldCosIEdka0RyYWdDb250ZXh0KiBjb250ZXh0LCBn aW50IHgsIGdpbnQgeSwgR3RrU2VsZWN0aW9uRGF0YSogZGF0YSwgZ3VpbnQgaW5mbywgZ3VpbnQg dGltZSwgZ3BvaW50ZXIgdXNlckRhdGEpIHsKKyAgICBnX3NpZ25hbF9jb25uZWN0X2FmdGVyKG1f d2ViVmlldywgImRyYWctZGF0YS1yZWNlaXZlZCIsIEdfQ0FMTEJBQ0soK1tdKEd0a1dpZGdldCos IEdka0RyYWdDb250ZXh0KiBjb250ZXh0LCBnaW50IHgsIGdpbnQgeSwgR3RrU2VsZWN0aW9uRGF0 YSogZGF0YSwgZ3VpbnQgaW5mbywgZ3VpbnQgdGltZSwgZ3BvaW50ZXIgdXNlckRhdGEpIHsKICAg ICAgICAgYXV0byYgZHJvcCA9ICpzdGF0aWNfY2FzdDxEcm9wVGFyZ2V0Kj4odXNlckRhdGEpOwog ICAgICAgICBpZiAoZHJvcC5tX2Ryb3AgIT0gY29udGV4dCkKICAgICAgICAgICAgIHJldHVybjsK QEAgLTIzMCwxMSArMjMwLDggQEAgdm9pZCBEcm9wVGFyZ2V0OjpkaWRQZXJmb3JtQWN0aW9uKCkK ICAgICBhdXRvKiBwYWdlID0gd2Via2l0V2ViVmlld0Jhc2VHZXRQYWdlKFdFQktJVF9XRUJfVklF V19CQVNFKG1fd2ViVmlldykpOwogICAgIEFTU0VSVChwYWdlKTsKIAotICAgIGF1dG8gb3BlcmF0 aW9uID0gcGFnZS0+Y3VycmVudERyYWdPcGVyYXRpb24oKTsKLSAgICBpZiAob3BlcmF0aW9uID09 IG1fb3BlcmF0aW9uKQotICAgICAgICByZXR1cm47CisgICAgbV9vcGVyYXRpb24gPSBwYWdlLT5j dXJyZW50RHJhZ09wZXJhdGlvbigpOwogCi0gICAgbV9vcGVyYXRpb24gPSBvcGVyYXRpb247CiAg ICAgZ2RrX2RyYWdfc3RhdHVzKG1fZHJvcC5nZXQoKSwgZHJhZ09wZXJhdGlvblRvU2luZ2xlR2Rr RHJhZ0FjdGlvbihtX29wZXJhdGlvbiksIEdES19DVVJSRU5UX1RJTUUpOwogfQogCg==