Bug 57185 - drop event isn't fired for contentEditable in edit drag
: drop event isn't fired for contentEditable in edit drag
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Ryosuke Niwa
:
Depends on:
Blocks: 9915 76194 76198
  Show dependency treegraph
 
Reported: 2011-03-27 09:39 PDT by Alfonso Martínez de Lizarrondo
Modified: 2012-02-23 21:57 PST (History)
13 users (show)

See Also:


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 Details | Formatted Diff | Diff
fixes the bug (14.56 KB, patch)
2012-01-12 21:00 PST, Ryosuke Niwa
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alfonso Martínez de Lizarrondo 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.
Comment 2 Eric Seidel 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.
Comment 3 Ryosuke Niwa 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.
Comment 4 Ryosuke Niwa 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Eric Seidel 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.
Comment 7 Ryosuke Niwa 2012-01-12 21:00:15 PST
Created attachment 122366 [details]
fixes the bug
Comment 8 Ryosuke Niwa 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.
Comment 9 Adam Barth 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" ?
Comment 10 Ryosuke Niwa 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.
Comment 11 Ryosuke Niwa 2012-01-19 02:16:42 PST
Committed r105396: <http://trac.webkit.org/changeset/105396>
Comment 12 mitz@webkit.org 2012-02-23 21:57:49 PST
(In reply to comment #11)
> Committed r105396: <http://trac.webkit.org/changeset/105396>

This caused bug 79443.