Bug 33055 – [Qt] Drag & drop layout tests fail even when run manually

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yael]

These tests fail even when they are run in QtLauncher. The work to enable them in DumpRenderTree is still ongoing in https://bugs.webkit.org/show_bug.cgi?id=31332.

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

After a drag&drop operation fails, the drag operation is not cleaned-up, and even dropping files is failing.

[Yael]

Created attachment 45671 [details]
Patch.

Fix Drag&Drop operation in Qt port.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45671 [details] without any errors.

[Kenneth Rohde Christiansen]

Comment on attachment 45671 [details]
Patch.


> +static inline Qt::DropActions dragOpToDropActions(unsigned op)
> +{
> +    Qt::DropActions result = Qt::IgnoreAction;
> +    if (op & DragOperationCopy)
> +        result = Qt::CopyAction;
> +    if (op & DragOperationMove)
> +        result |= Qt::MoveAction;
> +    if (op & DragOperationGeneric)
> +        result |= Qt::MoveAction;
> +    if (op & DragOperationLink)
> +        result |= Qt::LinkAction;
> +    return result;
> +}

please write out operation in the method name.

> +
> +static inline DragOperation dropActionToDragOp(Qt::DropActions action)
> +{
> +    DragOperation result = DragOperationNone;
> +    if (action & Qt::CopyAction)
> +        result = DragOperationCopy;
> +    if (action & Qt::LinkAction)
> +        result = DragOperationLink;
> +    if (action & Qt::MoveAction)
> +        result = DragOperationMove;
> +    return result;
> +}

same

> +
>  DragDestinationAction DragClientQt::actionMaskForDrag(DragData*)
>  {
>      return DragDestinationActionAny;
> @@ -58,7 +86,7 @@
>  {
>  }
>  
> -void DragClientQt::startDrag(DragImageRef, const IntPoint&, const IntPoint&, Clipboard* clipboard, Frame*, bool)
> +void DragClientQt::startDrag(DragImageRef, const IntPoint&, const IntPoint&, Clipboard* clipboard, Frame* frame, bool)
>  {
>  #ifndef QT_NO_DRAGANDDROP
>      QMimeData* clipboardData = static_cast<ClipboardQt*>(clipboard)->clipboardData();
> @@ -66,10 +94,16 @@
>      QWidget* view = m_webPage->view();
>      if (view) {
>          QDrag *drag = new QDrag(view);
> -        if (clipboardData->hasImage())
> +        if (clipboardData && clipboardData->hasImage())
>              drag->setPixmap(qvariant_cast<QPixmap>(clipboardData->imageData()));
> +        DragOperation dragOperationMask = DragOperationEvery;
> +        clipboard->sourceOperation(dragOperationMask);
>          drag->setMimeData(clipboardData);
> -        drag->start();
> +        Qt::DropAction actualDropAction = drag->exec(dragOpToDropActions(dragOperationMask));

You only seem to use the dragOpToDropActions here and dragOperationMask always seems to be the same. Any reason for having that method then?

> +
> +        // Send dragEnd event
> +        PlatformMouseEvent me(m_webPage->view()->mapFromGlobal(QCursor::pos()), QCursor::pos(), LeftButton, MouseEventMoved, 0, false, false, false, false, 0);
> +        frame->eventHandler()->dragSourceEndedAt(me, dropActionToDragOp(actualDropAction));
>      }
>  #endif
>  }
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 52663)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,32 @@
> [phone]  Yael Aharon  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Drag & drop layout tests fail even when run manually
> +        https://bugs.webkit.org/show_bug.cgi?id=33055
> +
> +        No new tests. Fix 3 layout tests when run manually.
> +        fast/events/drag-and-drop.html
> +        fast/events/drag-and-drop-dataTransfer-types-nocrash.html
> +        fast/events/drag-and-drop-fire-drag-dragover.html
> +        Running these tests in DRT will be fixed in 31332.
> +
> +        * Api/qwebpage.cpp:
> +        (dropActionToDragOp):
> +        (dragOpToDropAction):
> +        (QWebPagePrivate::dragEnterEvent):
> +        (QWebPagePrivate::dragMoveEvent):
> +        (QWebPagePrivate::dropEvent):
> +        Accept drag events even if they are not over a drop target. 
> +        This is to ensure that drag events will continue to be delivered.
> +
> +        * Api/qwebpage_p.h:
> +        * WebCoreSupport/DragClientQt.cpp:
> +        (WebCore::dragOpToDropActions):
> +        (WebCore::dropActionToDragOp):
> +        (WebCore::DragClientQt::startDrag):
> +        Send dragEnd event.
> +
>  2009-12-30  Laszlo Gombos  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> Index: WebKit/qt/Api/qwebpage.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebpage.cpp	(revision 52606)
> +++ WebKit/qt/Api/qwebpage.cpp	(working copy)
> @@ -345,7 +345,7 @@
>      if (actions & Qt::CopyAction)
>          result |= DragOperationCopy;
>      if (actions & Qt::MoveAction)
> -        result |= DragOperationMove;
> +        result |= (DragOperationMove | DragOperationGeneric);

Maybe add a comment here as the change is not obvious

>      if (actions & Qt::LinkAction)
>          result |= DragOperationLink;
>      return (DragOperation)result;
> @@ -358,6 +358,8 @@
>          result = Qt::CopyAction;
>      else if (actions & DragOperationMove)
>          result = Qt::MoveAction;
> +    else if (actions & DragOperationGeneric)
> +        result = Qt::MoveAction;

Same here

>      else if (actions & DragOperationLink)
>          result = Qt::LinkAction;
>      return result;
> @@ -1091,8 +1093,9 @@
>                        dropActionToDragOp(ev->possibleActions()));
>      Qt::DropAction action = dragOpToDropAction(page->dragController()->dragEntered(&dragData));
>      ev->setDropAction(action);
> -    if (action != Qt::IgnoreAction)
> -        ev->accept();
> +    // We must accept this event in order to receive the drag move events that are sent
> +    // while the drag and drop action is in progress.
> +    ev->accept();
>  #endif
>  }
>  
> @@ -1132,9 +1135,11 @@
>      DragData dragData(ev->mimeData(), ev->pos(), QCursor::pos(),
>                        dropActionToDragOp(ev->possibleActions()));
>      Qt::DropAction action = dragOpToDropAction(page->dragController()->dragUpdated(&dragData));
> +    m_lastDropAction = action;
>      ev->setDropAction(action);
> -    if (action != Qt::IgnoreAction)
> -        ev->accept();
> +    // We must accept this event in order to receive the drag move events that are sent
> +    // while the drag and drop action is in progress.
> +    ev->accept();
>  #endif
>  }
>  
> @@ -1143,8 +1148,7 @@
>  #ifndef QT_NO_DRAGANDDROP
>      DragData dragData(ev->mimeData(), ev->pos().toPoint(),
>              QCursor::pos(), dropActionToDragOp(ev->possibleActions()));
> -    Qt::DropAction action = dragOpToDropAction(page->dragController()->performDrag(&dragData));
> -    if (action != Qt::IgnoreAction)
> +    if (page->dragController()->performDrag(&dragData))
>          ev->accept();
>  #endif
>  }
> @@ -1152,10 +1156,11 @@
>  void QWebPagePrivate::dropEvent(QDropEvent* ev)
>  {
>  #ifndef QT_NO_DRAGANDDROP
> +    // Overwrite the defaults set byQDragManager::defaultAction()

missing space byQDragManager

> +    ev->setDropAction(m_lastDropAction);

You don't need to clear  this m_lastDropAction somewhere?

>      DragData dragData(ev->mimeData(), ev->pos(), QCursor::pos(),
> -                      dropActionToDragOp(ev->possibleActions()));
> -    Qt::DropAction action = dragOpToDropAction(page->dragController()->performDrag(&dragData));
> -    if (action != Qt::IgnoreAction)
> +                      dropActionToDragOp(Qt::DropAction(ev->dropAction())));
> +    if (page->dragController()->performDrag(&dragData))
>          ev->accept();
>  #endif
>  }
> Index: WebKit/qt/Api/qwebpage_p.h
> ===================================================================
> --- WebKit/qt/Api/qwebpage_p.h	(revision 52606)
> +++ WebKit/qt/Api/qwebpage_p.h	(working copy)
> @@ -180,6 +180,7 @@
>      QWidget* inspectorFrontend;
>      QWebInspector* inspector;
>      bool inspectorIsInternalOnly; // True if created through the Inspect context menu action
> +    Qt::DropAction m_lastDropAction;
>  
>      static bool drtRun;
>  };
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 52663)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> [phone]  Yael Aharon  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Drag & drop layout tests fail even when run manually
> +        https://bugs.webkit.org/show_bug.cgi?id=33055
> +
> +        No new tests. Fix 3 layout tests when run manually.
> +        fast/events/drag-and-drop.html
> +        fast/events/drag-and-drop-dataTransfer-types-nocrash.html
> +        fast/events/drag-and-drop-fire-drag-dragover.html
> +        Running these tests in DRT will be fixed in 31332.
> +
> +        * page/qt/DragControllerQt.cpp:
> +        (WebCore::DragController::cleanupAfterSystemDrag):
> +        Cleanup the drag operation if it failed to complete,
> +        Otherwise, new drag operations will not be possible.
> +
>  2009-12-30  Laszlo Gombos  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> Index: WebCore/page/qt/DragControllerQt.cpp
> ===================================================================
> --- WebCore/page/qt/DragControllerQt.cpp	(revision 52606)
> +++ WebCore/page/qt/DragControllerQt.cpp	(working copy)
> @@ -66,6 +66,7 @@
>  
>  void DragController::cleanupAfterSystemDrag()
>  {
> +    dragEnded();
>  }
>  
>  }

[Yael]

(In reply to comment #3)
> (From update of attachment 45671 [details])
> You only seem to use the dragOpToDropActions here and dragOperationMask always
> seems to be the same. Any reason for having that method then?
That is not true, clipboard->sourceOperation(dragOperationMask) is updating the value of the mask

>> +    ev->setDropAction(m_lastDropAction);
>
>You don't need to clear  this m_lastDropAction somewhere?
The flag is updated when a DragMove event is received, and it is used in the DropEvent event. You are always guaranteed at least on DragMove event.

[Yael]

Created attachment 45904 [details]
Patch v2

Update the patch to rename methods and add comments as requested in comment #3 .

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45904 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 45904 [details]
Patch v2

Clearing flags on attachment: 45904

Committed r52815: <http://trac.webkit.org/changeset/52815>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.