Bug 24731

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

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

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

<rdar://problem/5015961>

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.

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

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.

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

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.

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.

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?

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?

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

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.

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

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

Created attachment 39479 [details] General test case This is the condensed test case I came up with.

Created attachment 39483 [details] Path with test cases Here's the fix.

Created attachment 39501 [details] Ouput from running general test on Mac Here is the Mac output.

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.

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?

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).

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.

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.

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).

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.

(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".

(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.

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.

Created attachment 39582 [details] Patch with test case Oops, found tabs instead of spaces in layout test. Changed tabs to spaces.

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.

Created attachment 40187 [details] Patch with test case Merged updated test case with patch.

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?

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?

(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?

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?

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.

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.

I misspoke, see my comment for bug #29264. (In reply to comment #36) > This is a bug in WebKit.

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

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.

(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.

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.

(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.

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).

Created attachment 41057 [details] Patch with test case

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 @@ > +2009-10-11 Daniel Bates <dbates@webkit.org> > + > + 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 <abarth@webkit.org> > > 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!

Created attachment 41082 [details] Patch with test cases Updated patch based on Adam's comments.

Comment on attachment 41082 [details] Patch with test cases r=me!

Comment on attachment 41082 [details] Patch with test cases Clearing flags on attachment: 41082 Committed r49651: <http://trac.webkit.org/changeset/49651>

All reviewed patches have been landed. Closing bug.

(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?

(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.