1604048 - [Wayland] Drag and drop action not always right

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Robert Mader [:rmader]]

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

Comment 1

[Robert Mader [:rmader]]

Correction: it's not a '+' indicator but an arrow.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 3

[Robert Mader [:rmader]]

(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

Comment 4

[Robert Mader [:rmader]]

Attachment: D&D on X11

Comment 5

[Robert Mader [:rmader]]

Attachment: D&D on Wayland

To make it crystal clear I attached two screencasts.

Comment 6

[Robert Mader [:rmader]]

Attachment: D&D in Nautilus on Wayland

Video of Nautilus, behaving just as we would expect on Wayland, just for comparison how other GTK apps handle that.

Comment 7

[Robert Mader [:rmader]]

Attachment: Bug 1604048 - WIP: Wayland drag and drop action not always right. r=stransky

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

Comment 8

[Robert Mader [:rmader]]

The patch above appears to solve all issues I saw and notably does not reintroduce bug 1576268 (at least when tested here).

Comment 9

[Robert Mader [:rmader]]

: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

[Martin Stránský [:stransky] (ni? me)]

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

Comment 11

[Robert Mader [:rmader]]

Attachment: Bug 1604048 - Replace custom Wayland clipboard code with GTK3 APIs, r=stransky

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.

Comment 12

[Robert Mader [:rmader]]

(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#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.

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

[Martin Stránský [:stransky] (ni? me)]

(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#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.

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.

Comment 14

[Robert Mader [:rmader]]

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.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Okay, Thanks, I'll mark them abandoned then.

Comment 16

[Robert Mader [:rmader]]

: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

[Martin Stránský [:stransky] (ni? me)]

(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 for gtk_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.

Comment 18

[Robert Mader [:rmader]]

Attachment: Bug 1604048 - Make Wayland DnD behaviour closer match the one on X11, r=stransky

• Properly initialize mSelectedDragAction so GetSelectedDragAction()
  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

Comment 19

[Robert Mader [:rmader]]

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.

Comment 20

[Robert Mader [:rmader]]

Attachment: Bug 1604048 - Make Wayland DnD behaviour closer match X11's - part 1, r=stransky

• Properly initialize mSelectedDragAction of data_offers so GetSelectedDragAction()
  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

Comment 21

[Robert Mader [:rmader]]

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

Comment 22

[Robert Mader [:rmader]]

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

[Martin Stránský [:stransky] (ni? me)]

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 24

[Robert Mader [:rmader]]

Ups, closed the old one

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b717628c7e9
Make Wayland DnD behaviour closer match X11's - part 1, r=stransky

Comment 26

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1b717628c7e9

Comment 27

[walter.bishop]

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.

Comment 28

[Robert Mader [:rmader]]

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.