Bug 30188 – REGRESSION(r49203): Text Drag and Drop stopped working in latest nightly build

• Component: HTML Editing
• Status: RESOLVED FIXED

[CWG]

Selected Text from a Chatzy.com Chatroom will not drag and drop into a Tex-Edit window.  It always has in previous builds, and still does in straight Safari.

[Mark Rowe (bdash)]

I suspect this may be due to <http://trac.webkit.org/changeset/49203>.

[Mark Rowe (bdash)]

<rdar://problem/7285503>

[Mark Rowe (bdash)]

Confirmed that r49203 is at fault.

[Daniel Bates]

Created attachment 40837 [details]
Work in progress Patch

Work in progress. Waiting for build/layout test results to confirm.

[Daniel Bates]

Created attachment 40843 [details]
Patch

I know I may be given some grief in that I am treating the DragOperation enum as an unsigned. We should fix this because such operations are needed and done on variables of type DragOperation in other parts of the drag-and-drop code. The solution would be to follow a similar approach as in AppKit's NSDragging.h, that is typedef DragOperation as an unsigned and define an anonymous enum of DragOperations.

I suggest deferring this fix for a separate bug.

Also, this patch does not include a test case because since this issue is observed when dragging data inside the web browser to an external application a DRT test is not possible. Moreover, a non-trivial manual test case is not possible for the same reason.

[Eric Seidel (no email)]

We could extend DRT to support drags out of the WebView.  It just would be a pain.

[Eric Seidel (no email)]

Comment on attachment 40843 [details]
Patch

Why should the default be DragOperationNone?

Why shouldn't other code cause this to default to DragOperationAny?

You need a comment in the code to explain why this is right.

Also, I assume we already ahve a WebCore/manual-test for this?  If not, then we need one.

[Daniel Bates]

Not exactly sure why this is DragOperationNone other than this is not a DHTML drag. That is, there is no drag source operation specified.

The Mac specific code that I removed in bug #30107 performed what I added into this patch (but in the platform-specific Mac code).

What is the manual-test? Do you know off hand? I'll try to look myself. If we need one then the manual test would be something of the form "Open TextEdit and select text X and drag it into TextEdit. If it copies the test PASSED. Otherwise, the test FAILED."

(In reply to comment #7)
> (From update of attachment 40843 [details])
> Why should the default be DragOperationNone?
> 
> Why shouldn't other code cause this to default to DragOperationAny?
> 
> You need a comment in the code to explain why this is right.
> 
> Also, I assume we already ahve a WebCore/manual-test for this?  If not, then we
> need one.

[Eric Seidel (no email)]

You would have to look in WebCore/manual-tests yourself.  I don't know what tests are in there currently, no.

[Daniel Bates]

Never mind, looks like there is already a manual test case we can use for this issue: drag-out-of-background-window.html

(In reply to comment #8)
> Not exactly sure why this is DragOperationNone other than this is not a DHTML
> drag. That is, there is no drag source operation specified.
> 
> The Mac specific code that I removed in bug #30107 performed what I added into
> this patch (but in the platform-specific Mac code).
> 
> What is the manual-test? Do you know off hand? I'll try to look myself. If we
> need one then the manual test would be something of the form "Open TextEdit and
> select text X and drag it into TextEdit. If it copies the test PASSED.
> Otherwise, the test FAILED."
> 
> (In reply to comment #7)
> > (From update of attachment 40843 [details] [details])
> > Why should the default be DragOperationNone?
> > 
> > Why shouldn't other code cause this to default to DragOperationAny?
> > 
> > You need a comment in the code to explain why this is right.
> > 
> > Also, I assume we already ahve a WebCore/manual-test for this?  If not, then we
> > need one.

[Eric Seidel (no email)]

OK.  Ideally your ChangeLog should mention that this is covered by that manual test.

[Daniel Bates]

OK. On second read, what do you mean "Why shouldn't other code cause this to default to DragOperationAny?"

(In reply to comment #11)
> OK.  Ideally your ChangeLog should mention that this is covered by that manual
> test.

[Daniel Bates]

The reason I ask is that from what I gather tracing the drag-operation through EventHandler::handleDrag (where the source drag operation is passed to the DragController) <http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2286> AND <http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2318>, it looks like we can remove the condition that m_sourceDragOperation == DragOperationNone.

(In reply to comment #12)
> OK. On second read, what do you mean "Why shouldn't other code cause this to
> default to DragOperationAny?"
> 
> (In reply to comment #11)
> > OK.  Ideally your ChangeLog should mention that this is covered by that manual
> > test.

[Daniel Bates]

Created attachment 40851 [details]
Patch

Removed if condition (m_sourceDragOperation == DragOperationNone) since it appears it is sufficient to just test whether this is not a DHTML drag operation. As per Eric's comment, added notice to ChangeLog about manual test case: drag-out-of-background-window.html

[Daniel Bates]

Created attachment 40852 [details]
Patch

Cleaned up ChangeLog

[Eric Seidel (no email)]

Comment on attachment 40852 [details]
Patch

The empty comment line is not needed:
 654         //

I don't usually put < > around URLs.

I think this is OK to fix the bug, but I think we need to set this in a better, clearer place. it's so strange that this is set here, instead of in some earlier if.

Also, this code makes me wonder what other state we need to set for non-dhtml drags?

And why shouldn't m_sourceDragOperation default to DragOperationAny and then get set more restrictive in the DHTML case?  Again, this code is just ugly, and poorly designed here.  Your argument over IRC that we should take this simple fix and work to clean it up later is valid, but slightly saddening. :)

[Daniel Bates]

I'll make these changes before I land.

I agree, this is a bit ugly and I don't have a good reason why we should go with a more restrictive set of drag operations (this was from the code I removed in bug #30107 - maybe there is a reason for this, instead of just DragOperationAny, though the person who wrote it didn't comment why). I'll look into a better solution when I have some time tomorrow.

(In reply to comment #16)
> (From update of attachment 40852 [details])
> The empty comment line is not needed:
>  654         //
> 
> I don't usually put < > around URLs.
> 
> I think this is OK to fix the bug, but I think we need to set this in a better,
> clearer place. it's so strange that this is set here, instead of in some
> earlier if.
> 
> Also, this code makes me wonder what other state we need to set for non-dhtml
> drags?
> 
> And why shouldn't m_sourceDragOperation default to DragOperationAny and then
> get set more restrictive in the DHTML case?  Again, this code is just ugly, and
> poorly designed here.  Your argument over IRC that we should take this simple
> fix and work to clean it up later is valid, but slightly saddening. :)

[Daniel Bates]

Committed r49292: <http://trac.webkit.org/changeset/49292>