Bug 57185

Summary: drop event isn't fired for contentEditable in edit drag
Product: WebKit Reporter: Alfonso Martínez de Lizarrondo <amla70>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, annevk, ayg, darin, dbates, dcheng, enrica, eric, ian, mitz, oliver, rniwa, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 9915, 76194, 76198    
Attachments:
Description Flags
Sample testcase
none
work in progress
none
fixes the bug abarth: review+

Alfonso Martínez de Lizarrondo
Reported 2011-03-27 09:39:25 PDT
Created attachment 87076 [details] Sample testcase The only way to catch the onDrop event for contentEditable elements is to listen for ondragover and cancel that event, but in that case the normal behavior of allowing drag&drop is broken. In https://bugs.webkit.org/show_bug.cgi?id=3582#c5 it's stated that this is the expected behavior by the spec, but that's hardly useful for real usage. Firefox and IE both fire the onDrop event when something is really dropped into the content. An example of why we want to listen for that event: http://dev.ckeditor.com/ticket/7422 in order to provide Undo/Redo management we must track the changes to the document. Due to this bug, in Webkit we can opt to not cancel the onDragOver event and so the onDrop isn't fired, or cancel it and then the user can't drag anything.
Attachments
Sample testcase (937 bytes, text/html)
2011-03-27 09:39 PDT, Alfonso Martínez de Lizarrondo 		no flags
Details
work in progress (13.95 KB, patch)
2012-01-12 17:14 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixes the bug (14.56 KB, patch)
2012-01-12 21:00 PST, Ryosuke Niwa 		abarth: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2012-01-11 12:04:58 PST
This is causing us to fail http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop_harness.htm?url=types_attribute
Eric Seidel (no email)
Comment 2 2012-01-11 12:07:44 PST
Appears this also causes us to fail http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop/setData_dragstart.htm I suspect most of the IETC DnD tests hit this bug.
Ryosuke Niwa
Comment 3 2012-01-11 22:44:43 PST
I think we ought to be firing drop event in this case per current HTML5 spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model Look at the step 2 after the sentence "Otherwise, the drag operation might be a success; run these substeps:"; this step fires drop event and is executed for drag & drop operations within editable regions before DOM mutations are made.
Ryosuke Niwa
Comment 4 2012-01-12 15:38:18 PST
It turned out that this is a really hard bug to fix :( The spec requires deletion to happen inside the default event handler of dragend event instead of atomically happening immediately after inserting the dragged content once the drop event is fired. However, inserting the content will "orphan" the original selection and we won't be able to delete the dragged content in some cases; in particular, this happens in the attached case where the dragged content and the destination coincide in the same text node. Recovering the selection after the fact is particularly challenging because when ReplaceSelectionCommand::doApply is called, the frame's selection is changed to that of the drag caret's (i.e. destination's). Even if we had stored indexForVisiblePosition, we'll need to adjust the index when the drag source appears after the destination.
Ryosuke Niwa
Comment 5 2012-01-12 17:14:07 PST
Created attachment 122342 [details] work in progress This regresses undo after drag & drop. I think the current spec is flawed. It's extremely hard for us to split deletion and insertion of contents during drag and still get all editing behaviors (selection, undo, etc...) right. Given that Firefox 9 does it "atomically" in the dragend event, I'm inclined to copy that behavior instead.
Eric Seidel (no email)
Comment 6 2012-01-12 18:59:40 PST
Anne would be someone to talk to re: the spec flaws. :) You should also consider filing a bug with the w3c explaining your disagreement.
Ryosuke Niwa
Comment 7 2012-01-12 21:00:15 PST
Created attachment 122366 [details] fixes the bug
Ryosuke Niwa
Comment 8 2012-01-12 21:02:06 PST
There are so many bugs around this feature and I wasn't able to fix them all but I think this is a good step forward.
Adam Barth
Comment 9 2012-01-17 01:35:14 PST
Comment on attachment 122366 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=122366&action=review This was not nearly as scary as it looked at first. I'm not an expert here. Please let me know if you'd prefer to have someone more expert in this area review your patch. > Source/WebCore/page/EventHandler.cpp:1915 > + bool result = false; Should this be called "preventedDefault" rather than "result" ?
Ryosuke Niwa
Comment 10 2012-01-19 00:21:54 PST
Comment on attachment 122366 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=122366&action=review >> Source/WebCore/page/EventHandler.cpp:1915 >> + bool result = false; > > Should this be called "preventedDefault" rather than "result" ? Will fix before landing.
Ryosuke Niwa
Comment 11 2012-01-19 02:16:42 PST
Committed r105396: <http://trac.webkit.org/changeset/105396>
mitz
Comment 12 2012-02-23 21:57:49 PST
(In reply to comment #11) > Committed r105396: <http://trac.webkit.org/changeset/105396> This caused bug 79443.
Note You need to log in before you can comment on or make changes to this bug.