Bug 24731 – [Win] dropEffect is always set to "copy" at dragend

• Component: New Bugs
• Status: RESOLVED FIXED

[Sebastian Markbåge]

On drag and drop operations the dropEffect of dataTransfer is always set to "copy" at the dragend event. This makes it impossible to know whether or not the drag operation was canceled or should result in a move or link.

This seems to be Windows specific but it does occur in Chrome 1 and 2, Safari 4 and the nightly build.

The Mac versions works as expected and dropEffect is set to the last set value.

[Jessie Berlin]

Created attachment 31992 [details]
Test case for reading the value of event.dataTransfer.dropEffect in dragEnd

[Jessie Berlin]

Looks like this might be the culprit:

STDMETHODIMP WebDropSource::QueryContinueDrag(BOOL fEscapePressed, DWORD grfKeyState)
{

[....]

//FIXME: We need to figure out how to find out what actually happened in the drag <rdar://problem/5015961>
frame->eventHandler()->dragSourceEndedAt(generateMouseEvent(m_webView.get(), false), DragOperationCopy);

in \win\WebDropSource.cpp

[Adam Roben (:aroben)]

This code from Gecko seems relevant: <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDragService.cpp#331>.

[Adam Roben (:aroben)]

<rdar://problem/5015961>

[Jessie Berlin]

Created attachment 33466 [details]
Current state of investigating.

I am uploading this file so others can see where I have been going and where I am getting stuck.

Right now, I can't seem to make the changes to the value of pdwEffect stick in between calls the Windows DoDragDrop method (http://msdn.microsoft.com/en-us/library/ms678486(VS.85).aspx) to DragOver (http://msdn.microsoft.com/en-us/library/ms680129(VS.85).aspx) and Drop (http://msdn.microsoft.com/en-us/library/ms687242(VS.85).aspx).

I will next be uploading the values that I have seen when I have set breakpoints throughout the code.

THIS IS NOT A PATCH WAITING FOR REVIEW.

[Jessie Berlin]

Created attachment 33467 [details]
Output of putting in some breakpoints in Visual Studio and running the attached test case

[Daniel Bates]

Created attachment 38908 [details]
Patch with test cases

Fixes this issue by implementing support for DHTML drag-and-drop operations in the Windows builds (these operations are already supported in the Mac OS X build).

The layout tests run via Dump Render Tree (DRT) only work in the Mac OS X build of DRT because DRT does not return the correct drop effect for a simulated mouse click in the Windows build. This should be addressed in another bug.

However, you can manually run the layout tests and compare to the corresponding expected file.

[Eric Seidel (no email)]

I wonder how much overlap there is here with bug 26700.

[Jens Alfke]

My pending patch for bug 26700 (which has been out for review for more than a week now!) should address these problems and more. It includes a new layout test that checks the values of effectAllowed and dropEffect on every possible drag callback.

I'd recommend closing this as a dup of 26700.

[Daniel Bates]

I just built rev. 47975 under Windows both with and without the patch for bug #26700, and the patch does not appear to resolve the issue*.

Among the issues with drag-and-drop in the Windows build is that the keyboard state (i.e. modifier keys) are never looked at! Regardless of what DragController returns or the Clipboard state. That is, instead of determining the correct drop effect (i.e. move, copy, link) from the modifiers keys held down during the drag, hard coded values are used for the drop effect (either DragOperationCopy or DragOperationNone - see http://trac.webkit.org/browser/trunk/WebKit/win/WebDropSource.cpp#L112).

Briefly looking at Jens patch, it does not seem to address these issues.

*I should note that I get a build error when building rev. 47975, but it seems to be only with respect to building Dump Render Tree. Just in case, I will try to checkout a clean copy of rev. 47719, used in the patch for bug #26700, build and check to confirm.

(In reply to comment #9)
> My pending patch for bug 26700 (which has been out for review for more than a
> week now!) should address these problems and more. It includes a new layout
> test that checks the values of effectAllowed and dropEffect on every possible
> drag callback.
> 
> I'd recommend closing this as a dup of 26700.

[Jens Alfke]

True, there is no Windows-specific code in my patch, so it's not going to address that. This isn't a dup, then. But could you check whether there are other fixes in your patch that are already covered by mine, on the perhaps-optimistic assumption that mine gets landed first?

[Daniel Bates]

I'll check. Notice, I only touch the Windows-specific files. So, hopefully there won't be much of a conflict.

(In reply to comment #11)
> True, there is no Windows-specific code in my patch, so it's not going to
> address that. This isn't a dup, then. But could you check whether there are
> other fixes in your patch that are already covered by mine, on the
> perhaps-optimistic assumption that mine gets landed first?

[Eric Seidel (no email)]

Comment on attachment 38908 [details]
Patch with test cases

wow.  What a copy/paste disaster.  Please re-write your tests to use some form of abstraction so that we don't just have 15 subtly altered boiler-plate tests.

Also tests are much nicer when written as js-only/dom only tests.
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

[Jens Alfke]

You might be able to use the layout test I added in my patch (fast/events/drag-dropeffect.html), which seems to do similar things to your tests, but all in one. It might not fully pass without my patch, though.

[Daniel Bates]

Will do.

On Jens suggestion, I might be able to use his test case. Otherwise, I'll re-write my test cases.

(In reply to comment #13)
> (From update of attachment 38908 [details])
> wow.  What a copy/paste disaster.  Please re-write your tests to use some form
> of abstraction so that we don't just have 15 subtly altered boiler-plate tests.
> 
> Also tests are much nicer when written as js-only/dom only tests.
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

[Daniel Bates]

Got sidetracked with another bug. I saw Jen's patch for bug #26700 was committed. I believe I can refactor my code a bit now that Jens patch is in. Also, condensing the test cases. Will post shortly.

(In reply to comment #15)
> Will do.
> 
> On Jens suggestion, I might be able to use his test case. Otherwise, I'll
> re-write my test cases.
> 
> (In reply to comment #13)
> > (From update of attachment 38908 [details] [details])
> > wow.  What a copy/paste disaster.  Please re-write your tests to use some form
> > of abstraction so that we don't just have 15 subtly altered boiler-plate tests.
> > 
> > Also tests are much nicer when written as js-only/dom only tests.
> > http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

[Daniel Bates]

Created attachment 39479 [details]
General test case

This is the condensed test case I came up with.

[Daniel Bates]

Created attachment 39483 [details]
Path with test cases

Here's the fix.

[Daniel Bates]

Created attachment 39501 [details]
Ouput from running general test on Mac

Here is the Mac output.

[Daniel Bates]

Created attachment 39502 [details]
Output of general test on Windows

Here is the output of the general test under Windows. This test was done by hand since we need to fix bug #28902 to support DRT drag-and-drop tests under Windows.

[Daniel Bates]

Created attachment 39503 [details]
Diff of Mac general test output to Win general test output

Notice, the Mac and Windows test differ in the handling of cases when effectAllowed="none" and effectAllowed="uninitialized". The patch I uploaded follows the HTML 5 spec., but the current Mac version does not.

There are three options: 1) File another bug to bring the Mac version up to HTML 5 spec (I think Jen was attempting to do this in bug 26700), 2) Bring my patch inline with the Mac implementation. or 3) Let the Windows patch be a bit of ahead of the Mac version, skip the tests for effectAllowed="none" and effectAllowed="uninitialized" for now and we will do (1) or some Mac workaround to finally put DHTML drag-and-drop to rest.

Since this issue (and possible duplicates bug reports related to this issue) have been around for a long time, I am leaning towards (3). What are people's thoughts?

[Daniel Bates]

Created attachment 39563 [details]
Patch with manual test case

I renamed the test case file drag-and-drop.html and moved into WebCore/manual_test until we 1) fix the Mac build to handle the effectAllowed="uninitialized" and effectAllowed="none" cases according to the HTML 5 spec. and 2) Add drag-and-drop support to Windows DRT (see bug #28902).

[Eric Seidel (no email)]

It would be better to check in a test with failing results on Mac and skipped on Windows, than to have a manual test.  Manual tests are mostly ignored as far as I can tell.

[Daniel Bates]

Created attachment 39570 [details]
Patch with test case

On Eric's and Mark's suggestion, I moved the layout test back to LayoutTests/fast/events and modified the file platform/win/Skipped so that the test is skipped under the Windows DRT.

The layout tests fails some of the test cases on the Mac because of the aforementioned reasons regarding effectsAllowed="uninitialized" and effectsAllowed="none".

I didn't test this on Gtk or Qt, since I don't have a Gtk or Qt build.

[Eric Seidel (no email)]

Comment on attachment 39570 [details]
Patch with test case

style:
 4569 DragOperation WebView::keyStateToDragOperation(DWORD grfKeyState) {

Why are we using a DragOperation enum to store/represent a mask?  Seems we should be using an "unsigned" no?
 4581         operation = (DragOperation)(DragOperationGeneric | DragOperationCopy);

What does this name mean? grfKeyState

We probably need a bug documenting how we fix this:
 854     // FIXME: This variable is part of a workaround. The drop effect (pdwEffect) passed to Drop is incorrect. 

Personally I find the "testFailed" "testPassed" output provided to you by fast/js/resources/js-test-pre.js more readable than custom solutions.  One could even make this a true js-test/script-test like the other recent drag-drop tests.  Certainly not a dealbreaker though.  http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

It's difficult for me to read your test output, so it's difficult for me to tell what the state of this fix it.  It looks like there are a few test failures (which is fine, they just need to be more obvious).

[Eric Seidel (no email)]

Comment on attachment 39570 [details]
Patch with test case

I guess I'll r- this because it's difficult enough for me to read the test that I can't tell if this is right or not.  Kinda a lame reason, but I think it's still valid.

[Daniel Bates]

(In reply to comment #25)
> (From update of attachment 39570 [details])
> style:
>  4569 DragOperation WebView::keyStateToDragOperation(DWORD grfKeyState) {
> 
> Why are we using a DragOperation enum to store/represent a mask?  Seems we
> should be using an "unsigned" no?
>  4581         operation = (DragOperation)(DragOperationGeneric |
> DragOperationCopy);

Ok. Will change. This form was taken from the original file.

> What does this name mean? grfKeyState

|grfKeyState| is the current state of the keyboard modifier keys.

This was left in from original file and conforms to the same variable passed to other WebView routines (such as WebView::DragEnter, WebView::DragOver, etc). Not sure what the name means, but it also conforms with the MSDN drag-and-drop docs, such as http://msdn.microsoft.com/en-us/library/ms680129(VS.85).aspx.

> We probably need a bug documenting how we fix this:
>  854     // FIXME: This variable is part of a workaround. The drop effect
> (pdwEffect) passed to Drop is incorrect. 

Ok, will file a new bug.

> Personally I find the "testFailed" "testPassed" output provided to you by
> fast/js/resources/js-test-pre.js more readable than custom solutions.  One
> could even make this a true js-test/script-test like the other recent drag-drop
> tests.  Certainly not a dealbreaker though. 
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
> 
> It's difficult for me to read your test output, so it's difficult for me to
> tell what the state of this fix it.  It looks like there are a few test
> failures (which is fine, they just need to be more obvious).

Ok. I'll strip the "debug" messages from the test case and just make it say "testFailed" or "testPassed".

[Eric Seidel (no email)]

(In reply to comment #27)
> Ok. I'll strip the "debug" messages from the test case and just make it say
> "testFailed" or "testPassed".

No. :)  "testFailed" and "testPassed" are functions used by most of our modern layout tests. :)  See fast/js/resources/js-test-pre.js.

[Daniel Bates]

Created attachment 39581 [details]
Patch with test case

Changed WebView::keyStateToDragOperation so that it returns a valid DropOperation. Cleaned up layout test. Filed bug #29264 to address drop effect issue and documented in WebView. Using workaround in this patch.

[Daniel Bates]

Created attachment 39582 [details]
Patch with test case

Oops, found tabs instead of spaces in layout test. Changed tabs to spaces.

[Daniel Bates]

Created attachment 40185 [details]
Updated test case

I updated the test case to work under Firefox (for comparison). Will merge this test case into the patch for this bug shortly.

[Daniel Bates]

Created attachment 40187 [details]
Patch with test case

Merged updated test case with patch.

[Adam Roben (:aroben)]

Comment on attachment 40187 [details]
Patch with test case

> +    // FIXME: This variable is part of a workaround. The drop effect (pdwEffect) passed to Drop is incorrect. 
> +    // We set this variable in DragEnter and DragOver so that it can be used in Drop to set the correct drop effect. 
> +    // Thus, on return from DoDragDrop we have the correct pdwEffect for the drag-and-drop operation.
> +    // (see https://bugs.webkit.org/show_bug.cgi?id=29264)

I think this comment needs to be expanded upon. Who's bug are we working around?

[Daniel Bates]

I filed a separate bug for this https://bugs.webkit.org/show_bug.cgi?id=29264.

In short, in WebView::Drop, we execute the assignment: *pdwEffect = DROPEFFECT_NONE <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4683>. But now the caller of DoDragDrop(*) knows nothing about the actual drop effect because when method WebView::Drop returns, pdwEffect is always set to DROPEFFECT_NONE!

Expected result: the actual drop effect that will occur as a result of the drop. Is it a copy? a move? a link? none? WebView::Drop needs to set pdwEffect accordingly.

(*) DoDragDrag is a function that, as part of its event loop, calls WebView::Drop. When DoDragDrop returns, the pdwEffect parameter is the last set value (and WebView::Drop sets it last). See <http://msdn.microsoft.com/en-us/library/ms678486%28VS.85%29.aspx> for more details.

(In reply to comment #33)
> (From update of attachment 40187 [details])
> > +    // FIXME: This variable is part of a workaround. The drop effect (pdwEffect) passed to Drop is incorrect. 
> > +    // We set this variable in DragEnter and DragOver so that it can be used in Drop to set the correct drop effect. 
> > +    // Thus, on return from DoDragDrop we have the correct pdwEffect for the drag-and-drop operation.
> > +    // (see https://bugs.webkit.org/show_bug.cgi?id=29264)
> 
> I think this comment needs to be expanded upon. Who's bug are we working
> around?

[Adam Roben (:aroben)]

(In reply to comment #34)
> (In reply to comment #33)
> > (From update of attachment 40187 [details] [details])
> > > +    // FIXME: This variable is part of a workaround. The drop effect (pdwEffect) passed to Drop is incorrect. 
> > > +    // We set this variable in DragEnter and DragOver so that it can be used in Drop to set the correct drop effect. 
> > > +    // Thus, on return from DoDragDrop we have the correct pdwEffect for the drag-and-drop operation.
> > > +    // (see https://bugs.webkit.org/show_bug.cgi?id=29264)
> > 
> > I think this comment needs to be expanded upon. Who's bug are we working
> > around?
> 
> I filed a separate bug for this https://bugs.webkit.org/show_bug.cgi?id=29264.

Great, thanks for filing it.

> In short, in WebView::Drop, we execute the assignment: *pdwEffect =
> DROPEFFECT_NONE
> <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4683>.
> But now the caller of DoDragDrop(*) knows nothing about the actual drop effect
> because when method WebView::Drop returns, pdwEffect is always set to
> DROPEFFECT_NONE!
> 
> Expected result: the actual drop effect that will occur as a result of the
> drop. Is it a copy? a move? a link? none? WebView::Drop needs to set pdwEffect
> accordingly.

Yes, I understood this part. What I didn't understand was whose bug we're working around with m_lastDropEffect. Bug 29264 doesn't really explain this either. Do you think this is a bug in WebKit? Or a bug in Windows? Or a bug in DumpRenderTree? Or a bug somewhere else?

[Daniel Bates]

This is a bug in WebKit.

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > (From update of attachment 40187 [details] [details] [details])
> > > > +    // FIXME: This variable is part of a workaround. The drop effect (pdwEffect) passed to Drop is incorrect. 
> > > > +    // We set this variable in DragEnter and DragOver so that it can be used in Drop to set the correct drop effect. 
> > > > +    // Thus, on return from DoDragDrop we have the correct pdwEffect for the drag-and-drop operation.
> > > > +    // (see https://bugs.webkit.org/show_bug.cgi?id=29264)
> > > 
> > > I think this comment needs to be expanded upon. Who's bug are we working
> > > around?
> > 
> > I filed a separate bug for this https://bugs.webkit.org/show_bug.cgi?id=29264.
> 
> Great, thanks for filing it.
> 
> > In short, in WebView::Drop, we execute the assignment: *pdwEffect =
> > DROPEFFECT_NONE
> > <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4683>.
> > But now the caller of DoDragDrop(*) knows nothing about the actual drop effect
> > because when method WebView::Drop returns, pdwEffect is always set to
> > DROPEFFECT_NONE!
> > 
> > Expected result: the actual drop effect that will occur as a result of the
> > drop. Is it a copy? a move? a link? none? WebView::Drop needs to set pdwEffect
> > accordingly.
> 
> Yes, I understood this part. What I didn't understand was whose bug we're
> working around with m_lastDropEffect. Bug 29264 doesn't really explain this
> either. Do you think this is a bug in WebKit? Or a bug in Windows? Or a bug in
> DumpRenderTree? Or a bug somewhere else?

[Eric Seidel (no email)]

Comment on attachment 40187 [details]
Patch with test case

Style:
+WebCore::PlatformMouseEvent generateMouseEvent(WebView* webView, bool isDrag);
(no arg names when they don't help)

Style:
 4618 DragOperation WebView::keyStateToDragOperation(DWORD grfKeyState) {

You could remove these nodes when running under DRT after the test completes so as to simplify the test output:
 effectAllowed  
 8 
 9 Drop the red square onto me.
 10 
 11 Expects dropEffect 
 12 Items that can be dragged to the drop target:
 13 
 14 Square

Extra line?
93         else
 94             shouldBeEqualToString('event.dataTransfer.dropEffect', 'none');
 95             
 96     }

I would have probably written these differently:

 102         if (chosenDropEffect == "copy" && (allowedDropEffect == "copy" || allowedDropEffect == "copyLink" 
 103                                             || allowedDropEffect == "copyMove" || allowedDropEffect == "uninitialized"
 104                                             || allowedDropEffect == "all"))

if (chosenDropEffect == "copy" && ["copy", "copyLink", "copyMove", "unitialized", "all"].contains(allowedDropEffect))

I think that woudl work at least.

You should land bug 28902 and then you don't need this change:
 # Windows DRT drag-and-drop does not work correctly (https://bugs.webkit.org/show_bug.cgi?id=28902)
 109 fast/events/drag-and-drop.html
or?

Adam or Oliver are really the right people to review this.

[Daniel Bates]

Created attachment 40400 [details]
Patch with test case

Now that bug #28902 has been fixed, the layout test can run under Windows DRT.

Updated the patch based on the Eric's suggestions. Also, I some changes to the ChangeLog based on Adam's suggestions on the patch for bug #28902.

[Daniel Bates]

I misspoke, see my comment for bug #29264.

(In reply to comment #36)
> This is a bug in WebKit.

[Daniel Bates]

With Jessie's permission, I am re-assigning this bug to me.

[Adam Roben (:aroben)]

Comment on attachment 40400 [details]
Patch with test case

> +++ WebKit/win/WebDropSource.h	(working copy)
> @@ -30,8 +30,12 @@
>  #include "COMPtr.h"
>  #include <objidl.h>
>  
> +#include <WebCore/PlatformMouseEvent.h>
> +
>  class WebView;
>  
> +WebCore::PlatformMouseEvent generateMouseEvent(WebView*, bool isDrag);

Can we just forward-declare PlatformMouseEvent instead of including its header?

> +    // Conforms to Microsoft's key combinations as documented for 
> +    // IDropTarget::DragOver. Note, |grfKeyState| is the current 
> +    // state of the keyboard modifier keys on the keyboard. See:
> +    // http://msdn.microsoft.com/en-us/library/ms680129(VS.85).aspx
> +    //
> +    // By default, we return the source drag operation to conform with the Mac OS X build.
> +    
> +    DragOperation operation = m_page->dragController()->sourceDragOperation();
> +    if (operation == DragOperationNone)
> +        operation = DragOperationCopy;

This shouldn't have to be done in platform-specific code. Is this what's keeping Mac from matching HTML5? At the very least there should be a FIXME here. It would also be good to file a bug about the issue.

> +    WebCore::DragOperation keyStateToDragOperation(DWORD grfKeyState);

I think this can be a const member function.

> +        bool isUIDelegate = false;
>          if (SUCCEEDED(m_webView->uiDelegate(&ui))) {
>              COMPtr<IWebUIDelegatePrivate> uiPrivate;
>              if (SUCCEEDED(ui->QueryInterface(IID_IWebUIDelegatePrivate, (void**)&uiPrivate)))
>                  if (SUCCEEDED(uiPrivate->doDragDrop(m_webView, dataObject.get(), source.get(), okEffect, &effect)))
> -                    return;
> +                    isUIDelegate = true;
>          }
>  
> -        DoDragDrop(dataObject.get(), source.get(), okEffect, &effect);
> +        DragOperation operation = DragOperationNone;
> +        if (isUIDelegate || DoDragDrop(dataObject.get(), source.get(), okEffect, &effect) == DRAGDROP_S_DROP) {

Could we just make the UI delegate callback return the same values as DoDragDrop? DRT is the only implementor of this callback, so it should be easy to change.

I'd like to figure out what's going on with sourceDragOperation before r+ing this.

[Daniel Bates]

(In reply to comment #41)
> (From update of attachment 40400 [details])
> > +++ WebKit/win/WebDropSource.h	(working copy)
> > @@ -30,8 +30,12 @@
> >  #include "COMPtr.h"
> >  #include <objidl.h>
> >  
> > +#include <WebCore/PlatformMouseEvent.h>
> > +
> >  class WebView;
> >  
> > +WebCore::PlatformMouseEvent generateMouseEvent(WebView*, bool isDrag);
> 
> Can we just forward-declare PlatformMouseEvent instead of including its header?

Sure.

> 
> > +    // Conforms to Microsoft's key combinations as documented for 
> > +    // IDropTarget::DragOver. Note, |grfKeyState| is the current 
> > +    // state of the keyboard modifier keys on the keyboard. See:
> > +    // http://msdn.microsoft.com/en-us/library/ms680129(VS.85).aspx
> > +    //
> > +    // By default, we return the source drag operation to conform with the Mac OS X build.
> > +    
> > +    DragOperation operation = m_page->dragController()->sourceDragOperation();
> > +    if (operation == DragOperationNone)
> > +        operation = DragOperationCopy;
> 
> This shouldn't have to be done in platform-specific code. Is this what's
> keeping Mac from matching HTML5? At the very least there should be a FIXME
> here. It would also be good to file a bug about the issue.
> 

No, this is not keeping the Mac from matching HTML5. These lines emulate similar functionality as WebHTMLView::draggingSourceOperationMaskForLocal:
(which is called at the start of a Cocoa drag operation).

> > +    WebCore::DragOperation keyStateToDragOperation(DWORD grfKeyState);
> 
> I think this can be a const member function.

I'll change.

> 
> > +        bool isUIDelegate = false;
> >          if (SUCCEEDED(m_webView->uiDelegate(&ui))) {
> >              COMPtr<IWebUIDelegatePrivate> uiPrivate;
> >              if (SUCCEEDED(ui->QueryInterface(IID_IWebUIDelegatePrivate, (void**)&uiPrivate)))
> >                  if (SUCCEEDED(uiPrivate->doDragDrop(m_webView, dataObject.get(), source.get(), okEffect, &effect)))
> > -                    return;
> > +                    isUIDelegate = true;
> >          }
> >  
> > -        DoDragDrop(dataObject.get(), source.get(), okEffect, &effect);
> > +        DragOperation operation = DragOperationNone;
> > +        if (isUIDelegate || DoDragDrop(dataObject.get(), source.get(), okEffect, &effect) == DRAGDROP_S_DROP) {
> 
> Could we just make the UI delegate callback return the same values as
> DoDragDrop? DRT is the only implementor of this callback, so it should be easy
> to change.

I'll look into this.

> 
> I'd like to figure out what's going on with sourceDragOperation before r+ing
> this.

[Daniel Bates]

Comment on attachment 40400 [details]
Patch with test case

Clearing the review flag while I work on an updated patch based on Adam's comments.

[Adam Roben (:aroben)]

(In reply to comment #42)
> (In reply to comment #41)
> > (From update of attachment 40400 [details] [details])
> > > +    // Conforms to Microsoft's key combinations as documented for 
> > > +    // IDropTarget::DragOver. Note, |grfKeyState| is the current 
> > > +    // state of the keyboard modifier keys on the keyboard. See:
> > > +    // http://msdn.microsoft.com/en-us/library/ms680129(VS.85).aspx
> > > +    //
> > > +    // By default, we return the source drag operation to conform with the Mac OS X build.
> > > +    
> > > +    DragOperation operation = m_page->dragController()->sourceDragOperation();
> > > +    if (operation == DragOperationNone)
> > > +        operation = DragOperationCopy;
> > 
> > This shouldn't have to be done in platform-specific code. Is this what's
> > keeping Mac from matching HTML5? At the very least there should be a FIXME
> > here. It would also be good to file a bug about the issue.
> > 
> 
> No, this is not keeping the Mac from matching HTML5. These lines emulate
> similar functionality as WebHTMLView::draggingSourceOperationMaskForLocal:
> (which is called at the start of a Cocoa drag operation).

OK. I still think a FIXME + bug would be good.

[Daniel Bates]

Created attachment 41010 [details]
Patch with test cases

Updated patch now that bugs #30102, #30107, and #30175 have been resolved.

Also made modifications as suggested by Adam.

Need to compile and test this on Windows. I hope to do this tomorrow (10/12).

[Daniel Bates]

Created attachment 41057 [details]
Patch with test case

[Adam Roben (:aroben)]

Comment on attachment 41057 [details]
Patch with test case

> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog	(revision 49429)
> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,38 @@
> [phone]  Daniel Bates  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=24731
> +        And
> +        rdar://problem/5015961
> +        
> +        Implements support for DHTML drag-and-drop operations (i.e. ondragstart, ondragend)
> +        in the Windows build so that it conforms to the Mac OS X build. Hence, dropEffect is
> +        correctly set.
> +        
> +        The WebView and WebDropSource drag-and-drop functions, as called by function
> +        DoDragDrop in its event loop, neither used the drop effect as specified by
> +        event.dataTransfer.dropEffect nor respected event.dataTransfer.effectsAllowed.
> +        Instead, these functions defaulted to some hardcoded drop effect and set of
> +        allowed drop effects, respectively.
> +
> +        Tests: fast/events/drag-and-drop.html
> +
> +        * WebCoreSupport/WebDragClient.cpp:
> +        (WebDragClient::startDrag):
> +        * WebDropSource.cpp:
> +        (WebDropSource::QueryContinueDrag): Moved call to EventHandler::dragSourceEndedAt
> +        into method WebDragClient::startDrag.
> +        * WebDropSource.h:
> +        * WebView.cpp:
> +        (WebView::keyStateToDragOperation): Fixes <rdar://problem/5015961>. Determines
> +        appropriate drop effect from state of keyboard and allowed effects
> +        m_page->dragController()->sourceDragOperation().
> +        (WebView::DragEnter):
> +        (WebView::DragOver):
> +        (WebView::Drop):
> +        * WebView.h:
> +
>  2009-10-09  Adam Barth  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> Index: WebKit/win/WebDropSource.cpp
> ===================================================================
> --- WebKit/win/WebDropSource.cpp	(revision 49291)
> +++ WebKit/win/WebDropSource.cpp	(working copy)
> @@ -106,10 +106,6 @@ STDMETHODIMP WebDropSource::QueryContinu
>  {
>      if (fEscapePressed || !(grfKeyState & (MK_LBUTTON|MK_RBUTTON))) {
>          m_dropped = !fEscapePressed;
> -        if (Page* page = m_webView->page())
> -            if (Frame* frame = page->mainFrame()) 
> -                //FIXME: We need to figure out how to find out what actually happened in the drag <rdar://problem/5015961>
> -                frame->eventHandler()->dragSourceEndedAt(generateMouseEvent(m_webView.get(), false), fEscapePressed ? DragOperationNone : DragOperationCopy);
>          return fEscapePressed? DRAGDROP_S_CANCEL : DRAGDROP_S_DROP;
>      } else if (Page* page = m_webView->page())
>          if (Frame* frame = page->mainFrame()) 
> Index: WebKit/win/WebDropSource.h
> ===================================================================
> --- WebKit/win/WebDropSource.h	(revision 49291)
> +++ WebKit/win/WebDropSource.h	(working copy)
> @@ -32,6 +32,12 @@
>  
>  class WebView;
>  
> +namespace WebCore {
> +class PlatformMouseEvent;
> +} // namespace WebCore

I like that you're following our coding style for namespaces in headers. But I think we normally use a different style for a "non-main" namespace. Unfortunately this isn't documented anywhere! I think our de facto style in this case is to indent the "class" line and leave off the comment after the closing brace.

> @@ -172,16 +172,26 @@ void WebDragClient::startDrag(DragImageR
>          }
>  
>          DWORD okEffect = draggingSourceOperationMaskToDragCursors(m_webView->page()->dragController()->sourceDragOperation());
> -        DWORD effect;
> +        DWORD effect = DROPEFFECT_NONE;
>          COMPtr<IWebUIDelegate> ui;
> +        HRESULT hr = DRAGDROP_S_CANCEL;
>          if (SUCCEEDED(m_webView->uiDelegate(&ui))) {
>              COMPtr<IWebUIDelegatePrivate> uiPrivate;
>              if (SUCCEEDED(ui->QueryInterface(IID_IWebUIDelegatePrivate, (void**)&uiPrivate)))
> -                if (SUCCEEDED(uiPrivate->doDragDrop(m_webView, dataObject.get(), source.get(), okEffect, &effect)))
> -                    return;
> +                hr = uiPrivate->doDragDrop(m_webView, dataObject.get(), source.get(), okEffect, &effect);
> +        } else
> +            hr = DoDragDrop(dataObject.get(), source.get(), okEffect, &effect);

This has the same bug as the last version of this patch where you're requiring all implementors of IWebUIDelegatePrivate to implement doDragDrop. I think all you need to do is change the "else" to "if (hr == E_NOTIMPL)". That way implementors of IWebUIDelegatePrivate can just return E_NOTIMPL from doDragDrop and WebKit will fall back to the default behavior.

Once we get this doDragDrop thing fixed, I think this is ready to go!

[Daniel Bates]

Created attachment 41082 [details]
Patch with test cases

Updated patch based on Adam's comments.

[Adam Roben (:aroben)]

Comment on attachment 41082 [details]
Patch with test cases

r=me!

[Daniel Bates]

Comment on attachment 41082 [details]
Patch with test cases

Clearing flags on attachment: 41082

Committed r49651: <http://trac.webkit.org/changeset/49651>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Sebastian Markbåge]

(In reply to comment #51)
> All reviewed patches have been landed.  Closing bug.

This has been fixed for dragEnd event. However, HTML5 also defines that the dropEffect is available on the drop event as well. Currently it seems to always be set to undefined at drop. Suggest: reopen or new bug report?

[Jessie Berlin]

(In reply to comment #52)
> (In reply to comment #51)
> > All reviewed patches have been landed.  Closing bug.
> 
> This has been fixed for dragEnd event. However, HTML5 also defines that the
> dropEffect is available on the drop event as well. Currently it seems to always
> be set to undefined at drop. Suggest: reopen or new bug report?

I would suggest a new bug report with a test case that isolates checking the dropEffect on the drop event.