Bug 28902 – Windows DumpRenderTree should properly set the drop effect for a drag-and-drop operation

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

[Daniel Bates]

The Windows DumpRenderTree (DRT) nullifies the drop effect when simulating a drag-and-drop operation. Such functionality is supported in the Mac OS X DRT. We should support this functionality so that we can test drag-and-drop operations under Windows, such as in the layout tests of bug #24731.

Notice, in UIDelegate::doDragDrop the variable |performedEffect| is set to 0 = DROPEFFECT_NONE (see http://msdn.microsoft.com/en-us/library/ms693457(VS.85).aspx). Instead, we should pass this variable through the drap-and-drop machinery in UIDelegate so that we can set it to the correct drop effect.

[Daniel Bates]

Created attachment 40183 [details]
Patch

No test case attached because it uses the same test case as included in the patch for bug #24731.

This patch can be applied before the patch for bug #24731, but there will be no noticeable effect because we use a hard-coded value in the Windows drag-and-drop code anyway: <http://trac.webkit.org/browser/trunk/WebKit/win/WebDropSource.cpp?rev=45515#L112>.

[Daniel Bates]

CC'ed Eric as he might be interested in this since he is interested in fixing drag-and-drop support for files in Win DRT (bug #25924).

[Daniel Bates]

Created attachment 40184 [details]
Updated test case

I updated the test case to work under Firefox (for comparison). Will merge this test case into the patch after I get something to eat.

[Daniel Bates]

Comment on attachment 40184 [details]
Updated test case

Oops, attached to the wrong bug (so hungry that I am not thinking straight ;-))

[Adam Roben (:aroben)]

Comment on attachment 40183 [details]
Patch

> +            // Reset |didDragEnter| so that another drag started within the same frame works properly.
> +            //
> +            // Notice, WebView::DragEnter (i.e. webViewDropTarget->DragEnter) initializes the field
> +            // |WebView::m_dragData|, which is checked in WebView::DragOver (i.e. webViewDropTarget->DragOver).
> +            //
> +            // If |didDragEnter| == false, then |WebView::m_dragData| is uninitialized, and by
> +            // <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4657>
> +            // WebView::DragOver returns the drop effect DROPEFFECT_NONE, which is incorrect.
> +            // Hence, we set |didDragEnter| = false on a mouse up to reset the DRT drag-and-drop
> +            // machinery.

I don't understand this comment. You say that the behavior of WebView::DragOver "is incorrect". Is there a bug filed about this incorrectness? If so, it would probably be better to reference that bug instead of the Trac link you have now. What is the correct behavior? Maybe this will all become clear when I see the bug.

[Daniel Bates]

Notice, Win DRT emulates the drag-and-drop event loop implemented in DoDragDrop <http://msdn.microsoft.com/en-us/library/ms678486%28VS.85%29.aspx>. For any drag-and-drop operation that ends with a successful drop on the target, the correct call flow in Win DRT is: webViewDropTarget->DragEnter, webViewDropTarget->DragOver, webViewDropTarget->Drop (*).

Suppose there are two such drag-and-drop operations A, B on the same page. Tracing through the EventSender code, the call flow for A will be: (*) (and will set |didDragEnter| to true). But the call flow for B will be: webViewDropTarget->DragOver, webViewDropTarget->Drop (because |didDragEnter| is still true!; that is, it is never set to false after A completes). This is incorrect. The call flow should be the same for both operations and it should be (*).

There is no bug filed about this. A better idea is to file a new bug for this issue with the fix (setting didDragEnter to false), and remove this change from the fix for this bug (#28902). Let me know your thoughts.

(In reply to comment #5)
> (From update of attachment 40183 [details])
> > +            // Reset |didDragEnter| so that another drag started within the same frame works properly.
> > +            //
> > +            // Notice, WebView::DragEnter (i.e. webViewDropTarget->DragEnter) initializes the field
> > +            // |WebView::m_dragData|, which is checked in WebView::DragOver (i.e. webViewDropTarget->DragOver).
> > +            //
> > +            // If |didDragEnter| == false, then |WebView::m_dragData| is uninitialized, and by
> > +            // <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4657>
> > +            // WebView::DragOver returns the drop effect DROPEFFECT_NONE, which is incorrect.
> > +            // Hence, we set |didDragEnter| = false on a mouse up to reset the DRT drag-and-drop
> > +            // machinery.
> 
> I don't understand this comment. You say that the behavior of WebView::DragOver
> "is incorrect". Is there a bug filed about this incorrectness? If so, it would
> probably be better to reference that bug instead of the Trac link you have now.
> What is the correct behavior? Maybe this will all become clear when I see the
> bug.

[Adam Roben (:aroben)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 40183 [details] [details])
> > > +            // Reset |didDragEnter| so that another drag started within the same frame works properly.
> > > +            //
> > > +            // Notice, WebView::DragEnter (i.e. webViewDropTarget->DragEnter) initializes the field
> > > +            // |WebView::m_dragData|, which is checked in WebView::DragOver (i.e. webViewDropTarget->DragOver).
> > > +            //
> > > +            // If |didDragEnter| == false, then |WebView::m_dragData| is uninitialized, and by
> > > +            // <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4657>
> > > +            // WebView::DragOver returns the drop effect DROPEFFECT_NONE, which is incorrect.
> > > +            // Hence, we set |didDragEnter| = false on a mouse up to reset the DRT drag-and-drop
> > > +            // machinery.
> > 
> > I don't understand this comment. You say that the behavior of WebView::DragOver
> > "is incorrect". Is there a bug filed about this incorrectness? If so, it would
> > probably be better to reference that bug instead of the Trac link you have now.
> > What is the correct behavior? Maybe this will all become clear when I see the
> > bug.
> 
> Notice, Win DRT emulates the drag-and-drop event loop implemented in DoDragDrop
> <http://msdn.microsoft.com/en-us/library/ms678486%28VS.85%29.aspx>. For any
> drag-and-drop operation that ends with a successful drop on the target, the
> correct call flow in Win DRT is: webViewDropTarget->DragEnter,
> webViewDropTarget->DragOver, webViewDropTarget->Drop (*).
> 
> Suppose there are two such drag-and-drop operations A, B on the same page.
> Tracing through the EventSender code, the call flow for A will be: (*) (and
> will set |didDragEnter| to true). But the call flow for B will be:
> webViewDropTarget->DragOver, webViewDropTarget->Drop (because |didDragEnter| is
> still true!; that is, it is never set to false after A completes). This is
> incorrect. The call flow should be the same for both operations and it should
> be (*).

OK, so it sounds like your comment is explaining a bug that currently exists but that will be fixed by this patch. I think a much more concise comment would make this clearer. In fact, I'd recommend just removing everything but the first sentence of the comment you have in your patch.

> There is no bug filed about this. A better idea is to file a new bug for this
> issue with the fix (setting didDragEnter to false), and remove this change from
> the fix for this bug (#28902). Let me know your thoughts.

I think it's OK to leave this change as part of this patch.

Thanks for clearing this up for me!

[Daniel Bates]

Created attachment 40213 [details]
Patch

Cleaned up comment based on Adam's suggestion.

[Adam Roben (:aroben)]

Comment on attachment 40213 [details]
Patch

> +        * DumpRenderTree/win/DraggingInfo.h: Added field |m_dropEffect| to store performed drop effect.

I haven't seen this "|variableName|" style in ChangeLogs/comments in WebKit before. I'm not saying I'm opposed, just pointing out that you're going against the grain here.

r=me

[Daniel Bates]

I'll change the formatting before I land.

(In reply to comment #9)
> (From update of attachment 40213 [details])
> > +        * DumpRenderTree/win/DraggingInfo.h: Added field |m_dropEffect| to store performed drop effect.
> 
> I haven't seen this "|variableName|" style in ChangeLogs/comments in WebKit
> before. I'm not saying I'm opposed, just pointing out that you're going against
> the grain here.
> 
> r=me

[Daniel Bates]

Committed r48899: <http://trac.webkit.org/changeset/48899>