Bug 63004 – [Qt] Regression(60942) wrong default action for drag-and-drop.

• Component: DOM
• Status: RESOLVED FIXED

[Yael]

The fix introduced in r60942 is not correct, it causes QtWebKit to ignore the drop action when the proposed drag operation is not defined.

The real issues were
1. QDrag does not have an equivalent to DragOperationEvery.
2. We were not calling setDropAction() on the dropEvent.

A patch is coming soon.

[Yael]

Created attachment 97836 [details]
Patch.

Added special handling for the case that dragOperation is not initialized.
Save the last dropOperation and pass it to the dropEvent, so that it can be accepted by QDrag.
Call event->accepted() and not event->acceptProposedAction(), because the later ignores the dropAction specified in JavaScript.

Tested with the test page attached to https://bugs.webkit.org/show_bug.cgi?id=40401 and did not see any issue.
Also manually tested all combinations of LayoutTests/fast/events/drag-and-drop.html and they all pass.

[Igor Trindade Oliveira]

It would be great to have a LayoutTest specially because you said in ChangeLog:

"Call event->accepted() and not event->acceptProposedAction(), because the later ignores the dropAction specified in JavaScript."

This behavior should be testable in LayoutTests.

(In reply to comment #1)
> Created an attachment (id=97836) [details]
> Patch.
> 
> Added special handling for the case that dragOperation is not initialized.
> Save the last dropOperation and pass it to the dropEvent, so that it can be accepted by QDrag.
> Call event->accepted() and not event->acceptProposedAction(), because the later ignores the dropAction specified in JavaScript.
> 
> Tested with the test page attached to https://bugs.webkit.org/show_bug.cgi?id=40401 and did not see any issue.
> Also manually tested all combinations of LayoutTests/fast/events/drag-and-drop.html and they all pass.

[Yael]

(In reply to comment #2)
> It would be great to have a LayoutTest specially because you said in ChangeLog:
> "Call event->accepted() and not event->acceptProposedAction(), because the later ignores the dropAction specified in JavaScript."
> This behavior should be testable in LayoutTests.
> (In reply to comment #1)
> > Created an attachment (id=97836) [details] [details]
> > Patch.
> > 
> > Added special handling for the case that dragOperation is not initialized.
> > Save the last dropOperation and pass it to the dropEvent, so that it can be accepted by QDrag.
> > Call event->accepted() and not event->acceptProposedAction(), because the later ignores the dropAction specified in JavaScript.
> > 
> > Tested with the test page attached to https://bugs.webkit.org/show_bug.cgi?id=40401 and did not see any issue.
> > Also manually tested all combinations of LayoutTests/fast/events/drag-and-drop.html and they all pass.

Yes, it would have been great to have a layout test.
Please see https://bugs.webkit.org/show_bug.cgi?id=31332#c7 why I do not have one.

[Yael]

Please note that without my patch, running the test LayoutTests/fast/events/drag-and-drop.html will fail, so the issue is not about adding more tests, but about figuring out how to hook up DRT with QDrag.

[Yael]

Created attachment 97915 [details]
Patch.

Scale back on the code changes.
All the tests still pass as before.

[Andreas Kling]

Comment on attachment 97915 [details]
Patch.

Huh. Looks like QWebPagePrivate::m_lastDropAction was unused before this change.

Will dragMoveEvent() always be called before dropEvent()? If not, we'll be setting the drop action to an uninitialized value.

[Yael]

(In reply to comment #6)
> (From update of attachment 97915 [details])
> Huh. Looks like QWebPagePrivate::m_lastDropAction was unused before this change.
> Will dragMoveEvent() always be called before dropEvent()? If not, we'll be setting the drop action to an uninitialized value.

QDrag always sends a dragEvent when it starts, but for sake of clarity I will add the initialization.

[Yael]

Created attachment 98015 [details]
Patch.

Address Kling's comment.

[Andreas Kling]

Comment on attachment 98015 [details]
Patch.

LGTM

[WebKit Review Bot]

Comment on attachment 98015 [details]
Patch.

Clearing flags on attachment: 98015

Committed r89369: <http://trac.webkit.org/changeset/89369>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.