Bug 31332 – [Qt] Failing Drag-and-drop tests

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Daniel Bates]

The following drag-and-drop tests fail under the Qt build:

fast/events/drag-parent-node.html
fast/events/drag-and-drop.html
fast/events/drag-and-drop-dataTransfer-types-nocrash.html
fast/events/drag-and-drop-fire-drag-dragover.html

Note these same tests fail in the GTK build as well, see bug #30576.

[Chang Shu]

I am interested in doing some research on this issue.

[Andreas Kling]

http/tests/misc/drag-over-iframe-invalid-source-crash.html now fails as well (times out)

A proper fix for this is being looked into.

[Yael]

Seems to me that the timeout in DnD tests is due to a deadlock. QDrag->start() is a synchronous call. It creates a new event loop and will return only after the mouse is released. We are blocked from continue executing the JavaScript, so the MouseUp event is never received.

[Yael]

Created attachment 45573 [details]
Patch.

With this patch, and enabling the new flag TEST_DRAG_AND_DROP in EventSenderQt.cpp, these 4 tests are passing:
fast/events/drag-parent-node.html
fast/events/drag-and-drop.html
fast/events/drag-and-drop-dataTransfer-types-nocrash.html
fast/events/drag-and-drop-fire-drag-dragover.html

Also, drag-and-drop works much more reliably in QtLauncher. (You cannot initiate a drag from QGVLauncher, only drop).

The problem is that QDrag depends on the system cursor to really move, and I did not think we want running DRT to have a side effect of moving the system cursor.

Qt experts, I would love to hear your thoughts about the QDrag problem.

[Yael]

Created attachment 45667 [details]
Patch.

I forgot to mention in comment #4 that in order for QDrag to work, the view also needs to be visible. This is reflected in the updated patch.

[Antonio Gomes]

(In reply to comment #5)
> Created an attachment (id=45667) [details]
> Patch.
> 
> I forgot to mention in comment #4 that in order for QDrag to work, the view
> also needs to be visible. This is reflected in the updated patch.

although I believe the abreviation in "::sendOrQueEvent" is intentional and not a typo, it'd be better to be avoided, so sendOrQueEvent -> sendOrQueueEvent

[Yael]

After fixing 33055 (DnD behavior in QtWebKit) and 33114 (DRT's leapForward implementation) I am not sure how to proceed. If we set QWebView in DRT to be visible, and we really move the system cursor with the mouse move events, DnD layout tests pass. But of course we do not want to make these changes.

[Robert Hogan]

Hi Yael,

I've been looking at this and I agree that drag and drop can't work in the layout tests without displaying the mainview. However I don't think it is failing because it has to move the system cursor.

First of all, I believe EventSenderQt is not properly set up to interact with QDrag->exec() in trunk at the moment. I've made some small changes to it in the attached patch and I believe this allows the final mouseRelease() events to get through to QDrag. This can be tested with the fairly simple drag test fast/events/drag-parent-node.html.

The problem the patch fixes is that both the mouseMove() and the mouseRelease() event need to be posted before EventSenderQt's event loop is created. As it stands the event loop was getting created with the mouseMove() event with the result that the QDrag event loop could never receive the following MouseButtonRelease event.

With this patch I was able to put a breakpoint in qdnd_x11.cpp in Qt and watch the eventFilter() there catch the final MouseButtonRelease() event in the above test. So I know, even if my approach is wrongheaded in some other way, that at least DnD in Qt is now getting the events it should.

So the patch fixes tests that are interested in ondragstart and ondragend events but the real trick is getting Qt DnD in headless mode to send QtWebKit drop events. And this is where the problem is in my view.

When qdnd_x11.cpp catches the MouseButtonRelease event it always assumes the drag has been cancelled because 'willDrop' is always false. It is always false, because it can only be set to true in xdndHandleStatus() and that function is only called if QDragManager::move() detects the window under the cursor is XdndAware.

This can be hacked around up to a point, see the calls I've added in DragClientQt, but because such hacks would always have to guess (mostly incorrectly) the resulting drop action of the drag event even that won't solve enough tests to be a worthwhile workaround.

So to get this working properly in DRT we need to somehow fool QDragManager into treating the area under the cursor as always XdndAware and so let QDrag emit a drop event when the mouse button is released.

Does my analysis seem coherent to you?

[Robert Hogan]

Created attachment 61167 [details]
Getting Mouse Release Events to QDrag

[Yael]

Hi Robert,
I know that moving the system cursor in EventSenderQt is not an OK solution. But, I also saw the problems you have described in QDragManager, and moving the cursor solved them for me. 

I am not sure what others think about adding test code to WebKit, in order to pass tests. Personally, I would prefer to try and avoid that.

[Robert Hogan]

(In reply to comment #10)
> Hi Robert,
> I know that moving the system cursor in EventSenderQt is not an OK solution. But, I also saw the problems you have described in QDragManager, and moving the cursor solved them for me. 

I'm not disagreeing with that at all - I was just clarifying out loud that the reason it works is because QDragManager uses the QCursor position to find the window underneath it. 

So as you said originally, the window needs to be displayed. I'm adding the reason for it and wanted to be sure that I was on the right track.

> 
> I am not sure what others think about adding test code to WebKit, in order to pass tests. Personally, I would prefer to try and avoid that.

Absolutely. I think there is a simple reason that this problem is unsolvable (in full) unless we display the window during these tests: the Qt::DropAction returned by QDrag::exec() cannot be simulated no matter what we do, it derives from XEvents created by moving the system cursor across the display. Not all drag tests rely on a correct Qt::DropAction - we could always guess Qt::CopyAction for example - but to pass all of them we would definitely need to know for a fact what it actually is.

So we come back to your question: can we display the window during these tests?

If the answer is no, then we will always have to skip some of these. However, we could just send generic drop and drag events to QWebView from EventSenderQt and this would allow us to pass a subset of the tests. Which is a bit rubbish but seems to be the only available alternative.

[Eric Seidel (no email)]

In the past other ports have had to use real events, or show windows in order to pass tests. Then later on folks came along with a eureka moment and found other ways to implement the same.  I'm not sure if the same will heppen for Qt, but it sounds like a partial solution (whichever you choose), even one which can only be run on the bots (because it does things like move the real cursor) seems useful.

[Csaba Osztrogonác]

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

[Allan Sandfeld Jensen]

Created attachment 198289 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 198319 [details]
Patch

Send both dragEnter and dragMove on the first dragging move. Helps pass 5 more tests.

[Jocelyn Turcotte]

Comment on attachment 198319 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/DragClientQt.cpp:118
> +    if (m_dragData)
> +        delete m_dragData;

This looks like a good fit for OwnPtr or QScopedPointer, which would also avoid leaking m_dragData when the DragClient is destroyed.

> Source/WebKit/qt/WebCoreSupport/DragClientQt.cpp:121
> +    m_dragData = static_cast<ClipboardQt*>(clipboard)->clipboardData();
> +    m_dropActions = dragOperationsToDropActions(clipboard->sourceOperation());
> +    static_cast<ClipboardQt*>(clipboard)->invalidateWritableData();

Placing the invalidateWritableData() call right under the assignment and leave an empty line would improve readability.

> Source/WebKit/qt/WebCoreSupport/DragClientQt.cpp:144
> +    if (m_dragData)
> +        return m_dragData;
> +    return 0;

Isn't this the same as "return m_dragData;"?

> Source/WebKit/qt/WebCoreSupport/DragClientQt.cpp:151
> +    if (m_dragData)
> +        return m_dropActions;
> +    return Qt::IgnoreAction;

If you set m_dropActions(Qt::IgnoreAction) in the constructor, this would also be equivalent to simply doing "return m_dropAction;". It could also be inlined in the header.

> Tools/DumpRenderTree/qt/EventSenderQt.cpp:650
> +void EventSender::sendEventInOrder(QEvent* event)

What is defining what have to be sent directly with sendEvent or must go through sendEventInOrder?

> Tools/DumpRenderTree/qt/EventSenderQt.cpp:653
> +        QTest::qWait(m_delay);

Humm, I don't like using QTest outside of tests, but it seems like it did work like that for a while.

> Tools/DumpRenderTree/qt/EventSenderQt.h:130
> +    int m_delay;

Since you brought it in a wider scope, it would be nice to define its purpose with a sharper name. Something like m_sendEventDelay, m_nextEventDelay, m_dragEventDelay or m_eventLeapForwardDelay would make it more obvious.

[Jocelyn Turcotte]

Comment on attachment 198319 [details]
Patch

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

> Tools/DumpRenderTree/qt/EventSenderQt.cpp:658
> +    sendEvent(m_page->view(), event);
> +    delete event;

Also, since you're not posting events anymore, you could pass all events on the stack to sendEventInOrder and avoid the confusion already in the code on whether or not the QEvent is getting owned by sendEventInOrder vs. sendEvent.

[Jocelyn Turcotte]

Comment on attachment 198319 [details]
Patch

Mark for iteration/feedback.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.