Bug 63004 - [Qt] Regression(60942) wrong default action for drag-and-drop.
Summary: [Qt] Regression(60942) wrong default action for drag-and-drop.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-06-20 12:07 PDT by Yael
Modified: 2011-06-21 12:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch. (4.21 KB, patch)
2011-06-20 12:16 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (2.92 KB, patch)
2011-06-20 20:06 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (3.21 KB, patch)
2011-06-21 11:21 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2011-06-20 12:07:20 PDT
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.
Comment 1 Yael 2011-06-20 12:16:36 PDT
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.
Comment 2 Igor Trindade Oliveira 2011-06-20 13:07:02 PDT
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.
Comment 3 Yael 2011-06-20 14:13:38 PDT
(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.
Comment 4 Yael 2011-06-20 14:15:20 PDT
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.
Comment 5 Yael 2011-06-20 20:06:03 PDT
Created attachment 97915 [details]
Patch.

Scale back on the code changes.
All the tests still pass as before.
Comment 6 Andreas Kling 2011-06-21 10:46:42 PDT
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.
Comment 7 Yael 2011-06-21 11:11:55 PDT
(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.
Comment 8 Yael 2011-06-21 11:21:12 PDT
Created attachment 98015 [details]
Patch.

Address Kling's comment.
Comment 9 Andreas Kling 2011-06-21 11:24:00 PDT
Comment on attachment 98015 [details]
Patch.

LGTM
Comment 10 WebKit Review Bot 2011-06-21 12:18:25 PDT
Comment on attachment 98015 [details]
Patch.

Clearing flags on attachment: 98015

Committed r89369: <http://trac.webkit.org/changeset/89369>
Comment 11 WebKit Review Bot 2011-06-21 12:18:30 PDT
All reviewed patches have been landed.  Closing bug.