[Wayland] Drag and drop action not always right
Categories
(Core :: Widget: Gtk, defect, P3)
Tracking
()
People
(Reporter: rmader, Assigned: rmader)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 3 obsolete files)
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0
Steps to reproduce:
- select some item on a website and drag it (for example some text in this bug report)
- hover over some other comment / some random area on the site
- hover over the 'add comment' text-input area
Actual results:
On Wayland:
- the mouse has a small '+' indicator in both cases
- when dropping / releasing the mouse over the other comment / random area, the drag finish, but nothing indicates that actually nothing happened
Expected results:
On X11:
- when hovering over another comment, the mouse does not have the '+', it's only a hand
- when dropping over the comment, the dragged item 'flies' back to it's original position, indicating that the drop did not succeed
- when hovering the 'add comment' text-input area, the plus appears just like on Wayland
Additional info:
- AFAIK this was introduced in bug 1576268, which was an improvement over the previous situation. Maybe it just went a little bit too far :)
- A good reference GTK program for this could be Nautilus
Assignee | ||
Comment 1•5 years ago
|
||
Correction: it's not a '+' indicator but an arrow.
Comment 2•5 years ago
|
||
Which Wayland compositor do you run? That testcase works for me on Gnome/Fedora 31, for instance when I try D&D on this BZ page. I see an arrow and text from page is copied fine to this input field.
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
(In reply to Martin Stránský [:stransky] from comment #2)
Which Wayland compositor do you run?
GS / Mutter, 3.34 or git master.
That testcase works for me on Gnome/Fedora 31, for instance when I try D&D on this BZ page. I see an arrow and text from page is copied fine to this input field.
Yes indeed, dropping text to the input field works. The problem is: the little arrow indicator is also visible when not above the text input field.
To iterate on this:
- drag some text
- hover and drop it somewhere not on an input field
Expected result:
- cursor sprite should not have an arrow indicator
- text should 'fly' back to where it was
Observed result:
- cursor sprite does have a arrow indicator
- text simply vanishes
Note: I ran over it because I just fixed a glitch in the 'fly back' animation in GTK on X11, so it looks exactly the same on X11 and Wayland for apps like Nautilus. That's why I tested Firefox as well and figured it is never used on Wayland atm.
https://gitlab.gnome.org/GNOME/gtk/merge_requests/1235
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
To make it crystal clear I attached two screencasts.
Assignee | ||
Comment 6•5 years ago
|
||
Video of Nautilus, behaving just as we would expect on Wayland, just for comparison how other GTK apps handle that.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Do not assume any default behaviour but instead mirror GTK behaviour
in simply applying the choosen action.
Also, clear possible actions when necessary, so the right cursor
sprite is choosen and the drop cancel animation gets played.
Compare: gdk_wayland_selection_set_current_offer_actions
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
The patch above appears to solve all issues I saw and notably does not reintroduce bug 1576268 (at least when tested here).
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
:stransky, apparently we duplicate a lot of GTK code for Wayland. Is that still necessary? I looks to me like we could remove a lot of the platform specific handling and just rely on the GTK abstraction. Is there something specific that can not be done with a modern GTK 3.24?
Comment 10•5 years ago
|
||
(In reply to robert.mader from comment #9)
:stransky, apparently we duplicate a lot of GTK code for Wayland. Is that still necessary? I looks to me like we could remove a lot of the platform specific handling and just rely on the GTK abstraction. Is there something specific that can not be done with a modern GTK 3.24?
Yes, the all code here is because Firefox has sync clipboard, see:
https://searchfox.org/mozilla-central/rev/8f7b017a31326515cb467e69eef1f6c965b4f00e/widget/gtk/nsClipboardWayland.cpp#97
so we need to use Wayland clipboard directly. AFAIK It also means we need to implement D&D as it shares clipboard code on Wayland side.
Assignee | ||
Comment 11•5 years ago
|
||
GTK3 provides syncronous, display protocol agnostic APIs for clipboard
management. Replace the custom Wayland code by them.
This fixes some bugs and reduces maintenance burden.
While the new implementation is protocol agnostic and could also
replace the custom X11 code, there is a higher chance of regressing
there as the code is more mature. So lets give it a cycle for
Wayland only to make sure it is stable.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
(In reply to Martin Stránský [:stransky] from comment #10)
Yes, the all code here is because Firefox has sync clipboard, see:
https://searchfox.org/mozilla-central/rev/8f7b017a31326515cb467e69eef1f6c965b4f00e/widget/gtk/nsClipboardWayland.cpp#97so we need to use Wayland clipboard directly. AFAIK It also means we need to implement D&D as it shares clipboard code on Wayland side.
As it turns out, GTK3 does have synchronous API for that. If I understand things correctly, there are quite a few places in whole GTK widget folder that date back to GTK2 or early GTK3 days, working around bugs or copying code that was not available in still supported GTK version at that time. If the patch above works out well, I'll have a look to clean up the code base a bit, as we now have GTK 3.4 as minimum (and we might be able to raise it further, no? Maybe something that is not older than five years?).
Comment 13•5 years ago
•
|
||
(In reply to robert.mader from comment #12)
(In reply to Martin Stránský [:stransky] from comment #10)
Yes, the all code here is because Firefox has sync clipboard, see:
https://searchfox.org/mozilla-central/rev/8f7b017a31326515cb467e69eef1f6c965b4f00e/widget/gtk/nsClipboardWayland.cpp#97so we need to use Wayland clipboard directly. AFAIK It also means we need to implement D&D as it shares clipboard code on Wayland side.
As it turns out, GTK3 does have synchronous API for that. If I understand things correctly, there are quite a few places in whole GTK widget folder that date back to GTK2 or early GTK3 days, working around bugs or copying code that was not available in still supported GTK version at that time. If the patch above works out well, I'll have a look to clean up the code base a bit, as we now have GTK 3.4 as minimum (and we might be able to raise it further, no? Maybe something that is not older than five years?).
Robert, I'm sorry I was not accurate here. GTK provides synchronous clipboard API but that's only a wrapper around the basic async one and it's implemented internally by gtk_main_loop iterations at Gtk. Unfortunately we can't process gtk event queue in our clipboard code as the events from Gtk can do anything, for instance delete/free the affected window and so.
Updated•5 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
Stransky: thanks for the explanation with the GTK main loop! I should have known that you didn't write all that code without a good reason :) I'll give it another try as I now have a somewhat better idea of what to do to make things work as expected, so the patches above are both obsolete.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 15•5 years ago
|
||
Okay, Thanks, I'll mark them abandoned then.
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
:stransky : While trying to find out why DnD actions are different on Wayland, I stumbled over https://searchfox.org/mozilla-central/source/widget/gtk/nsDragService.cpp#1082:
if (mTargetDragContext) {
gtk_drag_get_data(mTargetWidget, mTargetDragContext, aFlavor, mTargetTime);
MOZ_LOG(sDragLm, LogLevel::Debug, ("about to start inner iteration."));
PRTime entryTime = PR_Now();
while (!mTargetDragDataReceived && mDoingDrag) {
// check the number of iterations
MOZ_LOG(sDragLm, LogLevel::Debug, ("doing iteration...\n"));
PR_Sleep(20 * PR_TicksPerSecond() / 1000); /* sleep for 20 ms/iteration */
if (PR_Now() - entryTime > NS_DND_TIMEOUT) break;
gtk_main_iteration();
}
}
This is for X11 and it looks to me like here asynchronous code is made synchronous by calling gtk_main_iteration()
manually. Is there a specific reason why this work here and we can't do it for gtk_clipboard_request_...()
? Sorry for all the noise, just trying to understand the whole thing.
Comment 17•5 years ago
•
|
||
(In reply to robert.mader from comment #16)
:stransky : While trying to find out why DnD actions are different on Wayland, I stumbled over https://searchfox.org/mozilla-central/source/widget/gtk/nsDragService.cpp#1082:
if (mTargetDragContext) { gtk_drag_get_data(mTargetWidget, mTargetDragContext, aFlavor, mTargetTime); MOZ_LOG(sDragLm, LogLevel::Debug, ("about to start inner iteration.")); PRTime entryTime = PR_Now(); while (!mTargetDragDataReceived && mDoingDrag) { // check the number of iterations MOZ_LOG(sDragLm, LogLevel::Debug, ("doing iteration...\n")); PR_Sleep(20 * PR_TicksPerSecond() / 1000); /* sleep for 20 ms/iteration */ if (PR_Now() - entryTime > NS_DND_TIMEOUT) break; gtk_main_iteration(); } }
This is for X11 and it looks to me like here asynchronous code is made synchronous by calling
gtk_main_iteration()
manually. Is there a specific reason why this work here and we can't do it forgtk_clipboard_request_...()
? Sorry for all the noise, just trying to understand the whole thing.
Yes, you're correct, we use the main loop during D&D on X11. That's sub optimal and causes various issues like browser crash etc.
The D&D backend on Linux is not much maintained and we update it for urgent fixes only.
Assignee | ||
Comment 18•5 years ago
|
||
- Properly initialize
mSelectedDragAction
soGetSelectedDragAction()
does not return random values when is should return 0 - Remove the work-around from Bug 1576268 and do unconditionally return
available actions if none are there. - Use
..._get_selected_action
on X11 and Wayland (no functional change
expected here, just cleanup to bundle functional decisions in one place) - Clean up the code a bit
Assignee | ||
Comment 19•5 years ago
|
||
Ok, this time I'm quite confident to have found the culprits, the main one being the uninitialized mSelectedDragAction
, resulting in random behaviour concerning the default action. Results look good here, hope the code is acceptable.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 20•5 years ago
|
||
- Properly initialize
mSelectedDragAction
of data_offers soGetSelectedDragAction()
does not return random values. - Emit a motion event on
data_offer_action
, just like GTK does, to chain up to
nsDragService::UpdateDragAction()
- Only advertise possible actions if we actually accept them.
- Rename some things to make them easier to understand
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 21•5 years ago
|
||
:stransky : I created a new revision because the latest version is sufficiently different. This one does not touch X11 code any more and just mimics GTK behaviour a bit more where we didn't before. While there are still issues to be tackled, I like to see this one land for 74 already, but keep the bug open for further enhancements.
Assignee | ||
Comment 22•5 years ago
|
||
To be clear here: as far as I can tell the patch does not regress anything but is a pure enhancement. But while working on it I found some issues that I didn't fix yet and probably won't for this release.
Comment 23•5 years ago
|
||
Thanks Robert. Which revision is the correct one? I see two opened:
https://phabricator.services.mozilla.com/D60853
https://phabricator.services.mozilla.com/D58634
If there's any revision deprecated please mark it as abandoned at mercurial.
Thanks.
Comment 25•5 years ago
|
||
Comment 26•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Comment 27•4 years ago
|
||
Since a few months the DnD behavior changed for me and stalls sometimes completely in Wayland (sway/wlroots).
As I use two profiles in parallel I have to close both instances to get the DnD working again, but sometimes I've to restart sway or even have to restart the machine to get it working again.
What I experience:
When I only use one profile it can take hours and many successful attempts before DnD stops working. If I use DnD with the second profile in parallel it may take as few as 3 DnD attempts to make it stop working. This example is with regard to a linked video stream that I want to watch in mpv. As the DnD stalls it is not possible anymore for other programs to do the same, like dropping a video from thunar into mpv. This reverts to normal as soon as both firefox instances (70%) , sway (20%) or the machine (10%) has been restarted.
The people over at sway said that I should report this here first as it seems to be a known problem but I couldn't find a more related bug report than this. If I should open a new bug report or if you need more information, please let me know.
Assignee | ||
Comment 28•4 years ago
|
||
I think dnd actions are correct now, however we still have these occasional errors after which dnd stays broken at least until FF gets restarted - tracking that in bug 1622107 and closing this one.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Description
•