vaadin / flow-components

Java counterpart of Vaadin Web Components
82 stars 63 forks source link

Using a draggable element inside the Dialog to drag the dialog sometimes breaks DnD #6338

Open F-Kamp opened 4 weeks ago

F-Kamp commented 4 weeks ago

Description

Using the class "draggable" on an element inside a dialog to drag the dialog sporadically results in faulty behavior. After 1-20 movements, the drag operation doesn't end when I release the primary mouse button.

The dialog keeps being dragged with the cursor although no mouse button is pressed. After clicking again, the drag operation ends.

I have tested the behavior with Chrome, Edge and Firefox. Only Chrome and Edge seem to be affected by this bug. I don't know if the same bug occurs when using a touch screen; I've only tested it with a mouse.

I've also tested dragging an empty dialog directly (without an element with class "draggable"). That seemed to work without any problems.

Edit: I've investigated a bit more. After adding mousedown and mouseup listeners, it has become apparent that sometimes the mouseup event is missing:

2024-06-05 07:54:11,442 INFO [stdout] (default task-1) mousedown triggered, total count: 1 2024-06-05 07:54:11,682 INFO [stdout] (default task-1) mouseup triggered, total count: 1

[...]

2024-06-05 07:54:21,126 INFO [stdout] (default task-1) mousedown triggered, total count: 14 2024-06-05 07:54:21,427 INFO [stdout] (default task-1) mouseup triggered, total count: 14

2024-06-05 07:54:21,847 INFO [stdout] (default task-1) mousedown triggered, total count: 15 <-------- mouse up is missing, dialog sticks to mouse although no button is pressed. Event counters are now out of sync

2024-06-05 07:54:25,088 INFO [stdout] (default task-1) mousedown triggered, total count: 16 2024-06-05 07:54:25,177 INFO [stdout] (default task-1) mouseup triggered, total count: 15

2024-06-05 07:54:27,158 INFO [stdout] (default task-1) mousedown triggered, total count: 17 2024-06-05 07:54:27,413 INFO [stdout] (default task-1) mouseup triggered, total count: 16

I've extended the example to include the event listeners.

Expected outcome

The drag operation should always end when I release the mouse button.

Minimal reproducible example

@Route("dialogDemo")
public class DialogBugDemo extends VerticalLayout {
    @Serial
    private static final long serialVersionUID = -8827012377302580777L;

    public DialogBugDemo() {

        var testDialog = new Dialog();
        testDialog.setModal(false);
        testDialog.setCloseOnEsc(false);
        testDialog.setCloseOnOutsideClick(false);
        testDialog.setDraggable(true);

        var draggableDiv = new Div();
        draggableDiv.addClassName("draggable");
        draggableDiv.setWidth("10em");
        draggableDiv.setHeight("10em");
        draggableDiv.getElement().getStyle().set("background", "cyan");

        var mouseUpCounter = new AtomicInteger();
        var mouseDownCounter = new AtomicInteger();

        draggableDiv.getElement().addEventListener("mousedown", event -> {
            var msg = event.getType() + " triggered, total count: " + mouseDownCounter.incrementAndGet();
            System.out.println(msg);
            Notification.show(msg, 5000, Notification.Position.TOP_START);
        });

        draggableDiv.getElement().addEventListener("mouseup", event -> {
            var msg = event.getType() + " triggered, total count: " + mouseUpCounter.incrementAndGet();
            System.out.println(msg);
            Notification.show(msg, 5000, Notification.Position.TOP_START);
        });

        testDialog.add(draggableDiv);

        testDialog.open();
    }
}

Steps to reproduce

  1. Navigate to the view from the example
  2. Drag the dialog around a few times (click and hold mouse button, move, release button)

After 1-20 movements the dialog sticks to the cursor although the mouse button has been released.

Environment

Vaadin version(s): 24.3.11 OS: Windows 11

Browsers

Chrome, Edge

DiegoCardoso commented 3 weeks ago

Hi @F-Kamp, thanks for the ticket and for providing all the details.

However, I have tested this with the draggable Dialog in multiple places and with the snippet you provided, but I couldn't reproduce this issue. The only time I could verify a misalignment in the events was when I moved the mouse away from the viewport while dragging the dialog. Since the mouseup happened outside, it doesn't get triggered. But even in that case, moving the mouse back wouldn't case the dialog to stick onto it with the button released. I tested in a macOS machine, so I wonder if the OS has something to do with this issue.

F-Kamp commented 3 weeks ago

Hello @DiegoCardoso, thanks for the feedback.

Which browser did you use?

As I mentioned, I was only able to observe this bug while using Edge or Chrome and not while using Firefox. So it could be related to the Chromium engine (and maybe even only the Windows implementation of it).

DiegoCardoso commented 3 weeks ago

I tested on Arc, which uses Chromium under the hood.

F-Kamp commented 2 weeks ago

My colleague just tested this using Edge on Fedora Linux and was able to reproduce the error. So it's not only Chromium on Windows which is affected.

F-Kamp commented 2 weeks ago

I've just tested it with Vaadin 24.4.1. The error still occurs.

Here's a short demonstration with Edge: At second 3 you can see that Edge opens some kind of context menu, although I didn't do anything different than the previous 12 drag operations. During second 6 you can see the browser invalid cursor for a few milliseconds. After that the dialog is stuck to the mouse although no button is pressed.

https://github.com/vaadin/flow-components/assets/169134343/cc247e21-b013-429a-9542-2231d83db2ba

========================================================== Here's the demo using Chrome. At second 2 you can see the browser invalid cursor and the dialog becomes sticky until the next click at second 8.

https://github.com/vaadin/flow-components/assets/169134343/0338cf82-b1fe-40e9-ad16-397fc005f33a

@DiegoCardoso Is there anything else we can provide to help you analyze the issue? Any comment regarding a possible workaround / fix?

DiegoCardoso commented 2 weeks ago

I am still trying to reproduce it locally, I've tried with Chrome and Edge, but no luck so far.

What I can see from your recordings (in both cases) is that just before the dialog gets stuck to the pointer, the pointer changes to 🚫 :

image

And then, after that, it starts following the pointer.

F-Kamp commented 2 weeks ago

Is it possible for you to test on a different OS? We observed the bug with Windows 10, Windows 11 and Linux Fedora.

Also, let me repeat my questions from my last post: Is there anything else we can provide to help you analyze the issue? Any comment regarding a possible workaround / fix?

DiegoCardoso commented 2 weeks ago

Are you testing this issue in a new Vaadin application? Does it have any DnD configuration being set somewhere? I am intrigued with the 🚫 sign and if that might be an important factor for this bug to arise.

F-Kamp commented 2 weeks ago

The demo view is part of our application since I noticed the bug during the development of a new feature. There's no custom DnD configuration in our application but to be sure that there's no influence from our existing application, I will retry it in a fresh starter project and let you know whether I can still reproduce it.

F-Kamp commented 2 weeks ago

The same bug can be reproduced using the starter project. The invalid cursor icon is visible at second 5.

https://github.com/vaadin/flow-components/assets/169134343/d75163bb-26f7-4de8-aa43-dc001b3f67c2

my-app-demo.zip

Edit: Fixed uploaded source code.

DiegoCardoso commented 1 week ago

I can now say that I have been able to reproduce the bug.

Tried on Edge (running in Windows from a VM) and the easiest way to reproduce it in this view is to move the dialog at least once (so the notifications appear), then from the text in a notification card, hold the mouse button and move it through the dialog draggable div and then release the button. The next time, you try to drag the dialog by the draggable div, the issue will happen. Interestingly, when the drag works fine when done from the empty area in the dialog. Only when it is done from the draggable div it happens.

F-Kamp commented 1 week ago

Hello @DiegoCardoso, thank you for remaining persistent in your tries to reproduce the error.

Based on your observations, could you give us a rough ETA on a possible fix? Weeks/months/years?

I know it looks like a minor issue from your point of view (and usually I would agree) but we are currently in the process of discussing a graphical editor with a customer. The graphical editor relies heavily on the DnD behavior of dialogs so I would like to be able to tell them whether this is something that they might have to live with for the foreseeable future.

DiegoCardoso commented 1 week ago

I think I am onto something here.

It seems that, in some cases, there's some sort of conflict between the native draggable behavior and the logic responsible for moving the dialog. I am yet to understand what exactly causes it. But I got some clue from this article, which seems to fix it, at least in my first few tests.

Can you verify that this workaround solves it in your end?

draggableDiv.getElement().executeJs("this.addEventListener('dragstart', (e) => e.preventDefault())")
DiegoCardoso commented 1 week ago

About the ETA, it's hard to say. If this issue can be fixed by the provided solution, then it could be a matter of a week or so. If it requires more investigation, it will need to be evaluated how it compares to other issue in our backlog.

F-Kamp commented 1 week ago

Sadly no, the workaround doesn't work. I can still reproduce the same error behavior with the invalid cursor symbol.

DiegoCardoso commented 1 week ago

What about changing the event to mousedown (or even adding another one) and calling preventDefault()?

F-Kamp commented 1 week ago

Preventing default mousedown handling did the trick!

Preventing default dragstart handling is not required. It's enough to add the following line to stop this error from happening:

draggableDiv.getElement().executeJs("this.addEventListener('mousedown', (e) => e.preventDefault())");
DiegoCardoso commented 1 week ago

That's the only thing I could find that differs the dragging behavior when it's done from the empty area compared to an element with draggable class. IIRC, the main reason we don't prevent default on the latter is to allow double-click word selection. When event.preventDefault() is called in a mousedown, then the selection is also not allowed in any form.