drop event isn't fired for contentEditable in edit drag
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jan 2012 10:16:35 +0000 (10:16 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jan 2012 10:16:35 +0000 (10:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=57185

Reviewed by Adam Barth.

Source/WebCore:

Dispatch drop and dragend events after edit drag per HTML5 spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model

There are two major differences between the spec and WebKit's new behavior:

While the spec says we have to insert the dragged contents immediately after dispatching drop event
and delete the source in the default event handler of dragend event, doing so in WebKit is extremely
difficult because of the way we manage the selection. Instead, we continue to delete the source
and insert the dragged contents immediately after the drop event; this behavior matches that of Firefox 9.

When the dragged contents and the destination of the move is in the same text node, ReplaceSelectionCommand
may end up replacing it with a new text node. But this removal causes a problem when EventHandler uses
the node to dispatch dragend event because the node is "orphaned" from its parent at that point. To mitigate
this issue, we update the dragState's m_dragSrc when the node is orphaned by the edit drag. While this behavior
may differ from the spec and other browsers, not delivering dragend to the editing host seems strictly worse than
dispatching it at the slightly wrong target.

Tests: fast/events/moving-text-should-fire-drop-and-dragend-events-2.html
       fast/events/moving-text-should-fire-drop-and-dragend-events.html

* page/DragController.cpp:
(WebCore::DragController::performDrag): Dispatch drop event even when m_isHandlingDrag is true as long
as DragDestinationActionDHTML is an acceptable action.
(WebCore::DragController::concludeEditDrag): Call updateDragStateAfterEditDragIfNeeded after inserting
the dragged contents. This is necessary when ReplaceSelectionCommand or MoveSelectionCommand modifies
the source node while inserting the dragged contents.
* page/EventHandler.cpp:
(WebCore::EventHandler::performDragAndDrop): Clear the drag state only if drop event's default action
was prevented so that we dispatch dragevent event later.
(WebCore::EventHandler::updateDragStateAfterEditDragIfNeeded): Update dragState's m_dragSrc when the node
is orphaned. See above for the rationale.
* page/EventHandler.h:

LayoutTests:

Added tests ensure moving text in contenteditable regions fire dragstart, drop, and dragend events.

* fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt: Added.
* fast/events/moving-text-should-fire-drop-and-dragend-events-2.html: Added.
* fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt: Added.
* fast/events/moving-text-should-fire-drop-and-dragend-events.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@105396 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2.html [new file with mode: 0644]
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/DragController.cpp
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/EventHandler.h

index 6edaef9..42c7d64 100644 (file)
@@ -1,3 +1,17 @@
+2012-01-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        drop event isn't fired for contentEditable in edit drag
+        https://bugs.webkit.org/show_bug.cgi?id=57185
+
+        Reviewed by Adam Barth.
+
+        Added tests ensure moving text in contenteditable regions fire dragstart, drop, and dragend events.
+
+        * fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt: Added.
+        * fast/events/moving-text-should-fire-drop-and-dragend-events-2.html: Added.
+        * fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt: Added.
+        * fast/events/moving-text-should-fire-drop-and-dragend-events.html: Added.
+
 2012-01-19  Dominic Mazzoni  <dmazzoni@google.com>
 
         Unreviewed - re-enables tests on chromium-mac by making minor
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2-expected.txt
new file mode 100644 (file)
index 0000000..9787035
--- /dev/null
@@ -0,0 +1,8 @@
+This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination. You should see source:dragstart, destination:drop, and source:dragend in the log.
+
+Log:
+source:dragstart
+destination:drop
+source:dragend
+
+PASS
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2.html b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-2.html
new file mode 100644 (file)
index 0000000..09efc03
--- /dev/null
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination.
+You should see source:dragstart, destination:drop, and source:dragend in the log.</p>
+<div id="source" contenteditable><span>Drag and drop this <span id="target">target</span> from here</div>
+<div id="destination" contenteditable><span>to here, the_destination.</span></div>
+<pre id="log">
+Log:
+</pre>
+<script>
+
+var log = document.getElementById('log');
+var source = document.getElementById('source');
+var destination = document.getElementById('destination');
+var eventLogs = [];
+
+function attachLogger(element) {
+    var logger = function(event) {
+        var text = element.id + ':' + event.type;
+        eventLogs.push(text);
+        log.innerHTML += text + '\n';
+    }
+    element.addEventListener('dragstart', logger, false);
+    element.addEventListener('dragend', logger, false);
+    element.addEventListener('drop', logger, false);
+}
+
+attachLogger(source);
+attachLogger(destination);
+
+if (window.eventSender) {
+    layoutTestController.dumpAsText();
+
+    source.focus();
+    var target = document.getElementById('target');
+    getSelection().selectAllChildren(target);
+
+    function mouseMoveToCenterOfElement(element) {
+        eventSender.mouseMoveTo(element.offsetLeft + element.offsetWidth / 2, element.offsetTop + element.offsetHeight / 2);
+    }
+
+    eventSender.dragMode = true;
+    mouseMoveToCenterOfElement(target);
+    eventSender.mouseDown();
+    eventSender.leapForward(500);
+    mouseMoveToCenterOfElement(destination);
+    eventSender.mouseUp();
+
+    source.style.display = 'none';
+    destination.style.display = 'none';
+
+    log.innerHTML += '\n' + (eventLogs.join('') == ['source:dragstart', 'destination:drop', 'source:dragend'].join('') ? 'PASS' : 'FAIL');
+}
+
+</script>
+</body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events-expected.txt
new file mode 100644 (file)
index 0000000..41ae904
--- /dev/null
@@ -0,0 +1,8 @@
+This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination. You should see dragstart, drop, and dragend in the log.
+
+Log:
+dragstart
+drop
+dragend
+
+PASS
diff --git a/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events.html b/LayoutTests/fast/events/moving-text-should-fire-drop-and-dragend-events.html
new file mode 100644 (file)
index 0000000..1fc2331
--- /dev/null
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests dragging text within the same contenteditable element. To manually test, move the target below to the destination.
+You should see dragstart, drop, and dragend in the log.</p>
+<div contenteditable><span>Drag_and_drop_this_target to here, the_destination.</span></div>
+<pre id="log">
+Log:
+</pre>
+<script>
+
+var log = document.getElementById('log');
+var div = document.querySelector('div');
+var eventLogs = [];
+
+function logEvent(event) {
+    eventLogs.push(event.type);
+    log.innerText += event.type + '\n';
+}
+
+div.addEventListener('dragstart', logEvent, false);
+div.addEventListener('dragend', logEvent, false);
+div.addEventListener('drop', logEvent, false);
+
+if (window.eventSender) {
+    layoutTestController.dumpAsText();
+
+    div.focus();
+    getSelection().collapse(div, 0);
+    getSelection().modify('extend', 'forward', 'word');
+
+    var y = div.offsetTop + div.offsetHeight / 2;
+    eventSender.dragMode = true;
+    eventSender.mouseMoveTo(div.offsetLeft + div.firstChild.offsetWidth / 5, y);
+    eventSender.mouseDown();
+    eventSender.leapForward(500);
+    eventSender.mouseMoveTo(div.offsetLeft + div.firstChild.offsetWidth * 4 / 5, y);
+    eventSender.mouseUp();
+
+    div.style.display = 'none'; // The resultant text will be different depending on font metrics so hide it.
+
+    log.innerHTML += '\n' + (eventLogs.join('') == ['dragstart', 'drop', 'dragend'].join('') ? 'PASS' : 'FAIL');
+}
+
+</script>
+</body>
+</html>
\ No newline at end of file
index 1af04a4..e083763 100644 (file)
@@ -1,3 +1,43 @@
+2012-01-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        drop event isn't fired for contentEditable in edit drag
+        https://bugs.webkit.org/show_bug.cgi?id=57185
+
+        Reviewed by Adam Barth.
+
+        Dispatch drop and dragend events after edit drag per HTML5 spec:
+        http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model
+
+        There are two major differences between the spec and WebKit's new behavior:
+
+        While the spec says we have to insert the dragged contents immediately after dispatching drop event
+        and delete the source in the default event handler of dragend event, doing so in WebKit is extremely
+        difficult because of the way we manage the selection. Instead, we continue to delete the source
+        and insert the dragged contents immediately after the drop event; this behavior matches that of Firefox 9.
+
+        When the dragged contents and the destination of the move is in the same text node, ReplaceSelectionCommand
+        may end up replacing it with a new text node. But this removal causes a problem when EventHandler uses
+        the node to dispatch dragend event because the node is "orphaned" from its parent at that point. To mitigate
+        this issue, we update the dragState's m_dragSrc when the node is orphaned by the edit drag. While this behavior
+        may differ from the spec and other browsers, not delivering dragend to the editing host seems strictly worse than
+        dispatching it at the slightly wrong target.
+
+        Tests: fast/events/moving-text-should-fire-drop-and-dragend-events-2.html
+               fast/events/moving-text-should-fire-drop-and-dragend-events.html
+
+        * page/DragController.cpp:
+        (WebCore::DragController::performDrag): Dispatch drop event even when m_isHandlingDrag is true as long
+        as DragDestinationActionDHTML is an acceptable action.
+        (WebCore::DragController::concludeEditDrag): Call updateDragStateAfterEditDragIfNeeded after inserting
+        the dragged contents. This is necessary when ReplaceSelectionCommand or MoveSelectionCommand modifies
+        the source node while inserting the dragged contents.
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::performDragAndDrop): Clear the drag state only if drop event's default action
+        was prevented so that we dispatch dragevent event later.
+        (WebCore::EventHandler::updateDragStateAfterEditDragIfNeeded): Update dragState's m_dragSrc when the node
+        is orphaned. See above for the rationale.
+        * page/EventHandler.h:
+
 2012-01-18  Kinuko Yasuda  <kinuko@chromium.org>
 
         Cleanup: Move chrome-specific filesystem type handling code (for FileSystem API) under chromium directory
index 5d834df..da74a0a 100644 (file)
@@ -201,19 +201,21 @@ bool DragController::performDrag(DragData* dragData)
 {
     ASSERT(dragData);
     m_documentUnderMouse = m_page->mainFrame()->documentAtPoint(dragData->clientPosition());
-    if (m_isHandlingDrag) {
-        ASSERT(m_dragDestinationAction & DragDestinationActionDHTML);
+    if (m_dragDestinationAction & DragDestinationActionDHTML) {
         m_client->willPerformDragDestinationAction(DragDestinationActionDHTML, dragData);
         RefPtr<Frame> mainFrame = m_page->mainFrame();
+        bool preventedDefault = false;
         if (mainFrame->view()) {
             // Sending an event can result in the destruction of the view and part.
             RefPtr<Clipboard> clipboard = Clipboard::create(ClipboardReadable, dragData, mainFrame.get());
             clipboard->setSourceOperation(dragData->draggingSourceOperationMask());
-            mainFrame->eventHandler()->performDragAndDrop(createMouseEvent(dragData), clipboard.get());
-            clipboard->setAccessPolicy(ClipboardNumb);    // invalidate clipboard here for security
+            preventedDefault = mainFrame->eventHandler()->performDragAndDrop(createMouseEvent(dragData), clipboard.get());
+            clipboard->setAccessPolicy(ClipboardNumb); // Invalidate clipboard here for security
+        }
+        if (m_isHandlingDrag || preventedDefault) {
+            m_documentUnderMouse = 0;
+            return true;
         }
-        m_documentUnderMouse = 0;
-        return true;
     }
 
     if ((m_dragDestinationAction & DragDestinationActionEdit) && concludeEditDrag(dragData)) {
@@ -477,6 +479,7 @@ bool DragController::concludeEditDrag(DragData* dragData)
     VisibleSelection dragCaret = m_page->dragCaretController()->caretPosition();
     m_page->dragCaretController()->clear();
     RefPtr<Range> range = dragCaret.toNormalizedRange();
+    RefPtr<Element> rootEditableElement = innerFrame->selection()->rootEditableElement();
 
     // For range to be null a WebKit client must have done something bad while
     // manually controlling drag behaviour
@@ -519,6 +522,11 @@ bool DragController::concludeEditDrag(DragData* dragData)
             applyCommand(ReplaceSelectionCommand::create(m_documentUnderMouse.get(), createFragmentFromText(range.get(), text),  ReplaceSelectionCommand::SelectReplacement | ReplaceSelectionCommand::MatchStyle | ReplaceSelectionCommand::PreventNesting));
     }
 
+    if (rootEditableElement) {
+        if (Frame* frame = rootEditableElement->document()->frame())
+            frame->eventHandler()->updateDragStateAfterEditDragIfNeeded(rootEditableElement.get());
+    }
+
     return true;
 }
 
index d42b128..550f919 100644 (file)
@@ -1911,15 +1911,18 @@ void EventHandler::cancelDragAndDrop(const PlatformMouseEvent& event, Clipboard*
     clearDragState();
 }
 
-void EventHandler::performDragAndDrop(const PlatformMouseEvent& event, Clipboard* clipboard)
+bool EventHandler::performDragAndDrop(const PlatformMouseEvent& event, Clipboard* clipboard)
 {
     Frame* targetFrame;
+    bool preventedDefault = false;
     if (targetIsFrame(m_dragTarget.get(), targetFrame)) {
         if (targetFrame)
-            targetFrame->eventHandler()->performDragAndDrop(event, clipboard);
+            preventedDefault = targetFrame->eventHandler()->performDragAndDrop(event, clipboard);
     } else if (m_dragTarget.get())
-        dispatchDragEvent(eventNames().dropEvent, m_dragTarget.get(), event, clipboard);
-    clearDragState();
+        preventedDefault = dispatchDragEvent(eventNames().dropEvent, m_dragTarget.get(), event, clipboard);
+    if (preventedDefault)
+        clearDragState();
+    return preventedDefault;
 }
 
 void EventHandler::clearDragState()
@@ -2783,7 +2786,14 @@ void EventHandler::dragSourceEndedAt(const PlatformMouseEvent& event, DragOperat
     // that consecutive mousemove events don't reinitiate the drag and drop.
     m_mouseDownMayStartDrag = false;
 }
-    
+
+void EventHandler::updateDragStateAfterEditDragIfNeeded(Element* rootEditableElement)
+{
+    // If inserting the dragged contents removed the drag source, we still want to fire dragend at the root editble element.
+    if (dragState().m_dragSrc && !dragState().m_dragSrc->inDocument())
+        dragState().m_dragSrc = rootEditableElement;
+}
+
 // returns if we should continue "default processing", i.e., whether eventhandler canceled
 bool EventHandler::dispatchDragSrcEvent(const AtomicString& eventType, const PlatformMouseEvent& event)
 {
index 9d05b3e..547bfe5 100644 (file)
@@ -127,7 +127,8 @@ public:
 #if ENABLE(DRAG_SUPPORT)
     bool updateDragAndDrop(const PlatformMouseEvent&, Clipboard*);
     void cancelDragAndDrop(const PlatformMouseEvent&, Clipboard*);
-    void performDragAndDrop(const PlatformMouseEvent&, Clipboard*);
+    bool performDragAndDrop(const PlatformMouseEvent&, Clipboard*);
+    void updateDragStateAfterEditDragIfNeeded(Element* rootEditableElement);
 #endif
 
     void scheduleHoverStateUpdate();