Bug 191228 – Plain text drag in contenteditable is always DragOperationCopy, never DragOperationMove

• Component: HTML Editing
• Status: RESOLVED FIXED

[Jonathan Hammer]

Created attachment 353783 [details]
Xcode project to reproduce issue

Dragging plain text inside a contenteditable element always results in the text being copied (instead of the text being moved as intended). This only happens with legacy WebView. Please see attached Xcode project to reproduce the issue.

I believe the cause of the problem is as follows:

1. DragController::startDrag calls `beginDrag`
2. `beginDrag` calls `cleanupAfterSystemDrag` after beginning the drag (this seems strange)
3. `cleanupAfterSystemDrag` calls `dragEnded`
4. `dragEnded` sets m_dragInitiator = nullptr
5. As the mouse is moved and the drag is updated, `dragEnteredOrUpdated` is called, which calls into `tryDocumentDrag`
6. `tryDocumentDrag` calls into `dragIsMove`
7. `dragIsMove` always return false because m_documentUnderMouse != m_dragInitiator (because m_dragInitiator was set to nullptr after the drag was started)

I think the root problem is in step #2. `beginDrag` should not call `cleanupAfterSystemDrag`. Interestingly, that line of code is marked with the following comment:

    // FIXME: This shouldn't be needed.

Removing that call to `cleanupAfterSystemDrag` fixes the issue for me.

[Radar WebKit Bug Importer]

<rdar://problem/45786830>

[Wenson Hsieh]

(In reply to Jonathan Hammer from comment #0)
> Created attachment 353783 [details]
> Xcode project to reproduce issue
> 
> Dragging plain text inside a contenteditable element always results in the
> text being copied (instead of the text being moved as intended). This only
> happens with legacy WebView. Please see attached Xcode project to reproduce
> the issue.
> 
> I believe the cause of the problem is as follows:
> 
> 1. DragController::startDrag calls `beginDrag`
> 2. `beginDrag` calls `cleanupAfterSystemDrag` after beginning the drag (this
> seems strange)
> 3. `cleanupAfterSystemDrag` calls `dragEnded`
> 4. `dragEnded` sets m_dragInitiator = nullptr
> 5. As the mouse is moved and the drag is updated, `dragEnteredOrUpdated` is
> called, which calls into `tryDocumentDrag`
> 6. `tryDocumentDrag` calls into `dragIsMove`
> 7. `dragIsMove` always return false because m_documentUnderMouse !=
> m_dragInitiator (because m_dragInitiator was set to nullptr after the drag
> was started)
> 
> I think the root problem is in step #2. `beginDrag` should not call
> `cleanupAfterSystemDrag`. Interestingly, that line of code is marked with
> the following comment:
> 
>     // FIXME: This shouldn't be needed.
> 
> Removing that call to `cleanupAfterSystemDrag` fixes the issue for me.

From a quick look over the code, your assessment sounds reasonable. Since drag state is cleaned up after the dragging session ends, it seems wrong to clear it out prematurely here.

It's also not obvious to me why this is here in the first place — one guess is that it was simply copied over from DragController::doSystemDrag, which uses a different macOS API to start a drag session (i.e. -dragImage:…:, as opposed to the more modern -beginDraggingSessionWithItems:…: that DragController::beginDrag utilizes). However, in the case where -dragImage:…: is used to begin the drag, we'll spin the runloop such that we don't return to DragController::doSystemDrag until the drag session is actually finished. I'll need to double check, but I don't think this is the case for -beginDraggingSessionWithItems:…:.

If that's true, it should be safe to remove this cleanup call, as long as we ensure that drag state cleanup is performed after dragging concludes when using the DragController::beginDrag codepath.

[Wenson Hsieh]

> actually finished. I'll need to double check, but I don't think this is the
> case for -beginDraggingSessionWithItems:…:.

After a bit of debugging, this appears to be true.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #3)
> > actually finished. I'll need to double check, but I don't think this is the
> > case for -beginDraggingSessionWithItems:…:.
> 
> After a bit of debugging, this appears to be true.

(i.e. -beginDraggingSessionWithItems:…: does not spin the runloop, whereas -dragImage:…: does)

[Jonathan Hammer]

Thank you for the quick reply and for taking the time to look at this!

> It's also not obvious to me why this is here in the first place — one guess
> is that it was simply copied over from DragController::doSystemDrag, which
> uses a different macOS API to start a drag session

Yes, I was curious why the call to cleanupAfterSystemDrag was there in the first place. Your explanation makes sense and seems likely.

> If that's true, it should be safe to remove this cleanup call, as long as we
> ensure that drag state cleanup is performed after dragging concludes when
> using the DragController::beginDrag codepath.

If it helps, I can confirm that the drag state is cleaned up when using the DragController::beginDrag codepath. -[WebHTMLView draggingSession:endedAtPoint:operation:] calls DragController::dragEnded to clean up the state.

[Jonathan Hammer]

Created attachment 354065 [details]
Patch

[Wenson Hsieh]

(In reply to Jonathan Hammer from comment #6)
> Created attachment 354065 [details]
> Patch

Thanks for the patch, Jonathan!

While this change looks reasonable, we'd also need to write a regression test for it. It should be possible to using existing infrastructure for testing drag and drop (LayoutTests/fast/events/drag-and-drop-*.html) to test this, but I haven't done the investigation to see whether it'll just work. Ideally we can just add a test that moves text within a textarea or contenteditable element and checks that the text isn't duplicated.

[Jonathan Hammer]

(In reply to Wenson Hsieh from comment #7)
> While this change looks reasonable, we'd also need to write a regression
> test for it. It should be possible to using existing infrastructure for
> testing drag and drop (LayoutTests/fast/events/drag-and-drop-*.html) to test
> this, but I haven't done the investigation to see whether it'll just work.
> Ideally we can just add a test that moves text within a textarea or
> contenteditable element and checks that the text isn't duplicated.

Thanks, Wenson. I'm not sure why my patch didn't include the ChangeLog diff (sorry, newbie here). Here's what I wrote in the changelog:

+        This change was tested manually, but I did not write a test case. Ideally, the 
+        DragAndDropSimulator could be used to test this code, but the simulator is WK2 only and
+        this change affects only WK1. I don't have the resources to build a drag simulator for 
+        WK1 at this time.

Per your suggestion, I'll take a look at LayoutTests/fast/events/drag-and-drop-*.html. Thanks!

[Wenson Hsieh]

(adding a couple more folks who are familiar with drag and drop)

[Jonathan Hammer]

I took a look into adding a regression test similar to other tests found at LayoutTests/fast/events/drag-and-drop-*.html. Here is what I found in process of writing the test:

1. The test I wrote used eventSender to fake a drag via a series of mouse down/leap-forward/move-to/up calls. The test did not pass under WK2 (because <https://bugs.webkit.org/show_bug.cgi?id=68552>) but that's OK, because this bug only affects WK1 and thus only needs to pass under WK1.

2. When running the test on WK1 *before* applying my proposed patch, I was surprised to find that the bug did not reproduce and the test passed. This was confusing because the bug reproduces when the drag is performed manually (see attached Xcode project from earlier).

3. I took a deep dive into DumpRenderTree to figure out why. It turns out that EventSender queues up the fake mouse events and only replays them once mouseUp() is called. The events are replayed synchronously (i.e. the run loop is not spun after each event -- see +[EventSendingController replaySavedEvents]).

4. The fact that the events are replayed synchronously means that the buggy codepath is not exercised and the bug never manifests. This is because, under DRT, the drag is over and finished by the time beginDrag calls cleanupAfterSystemDrag. However, in real world usage, the drag would still be in progress when beginDrag calls cleanupAfterSystemDrag (thus triggering the bug).

===

If my understanding is correctly, I think there could be two ways to resolve this:

1. Fix DRT so that +[EventSendingController replaySavedEvents] plays back one event each time through the run loop, instead of replaying them all at once

2. Instead of writing a LayoutTest, just write a ManualTest for this bug

My preference would be for #2. Messing with DRT seems too risky for a bug as small as this.

[Ryosuke Niwa]

Comment on attachment 354065 [details]
Patch

Thanks for making a patch but we'd need a change log entry and a test.
See https://webkit.org/contributing-code/

I think we can probably write an API test for this instead of a manual test.

[Jonathan Hammer]

Created attachment 354195 [details]
Patch

New proposed patch attached. The patch is the same as last time with two additions:

1. Layout test now included (my previous comments about problems replaying events were uninformed -- it turns out `eventSender.dragMode = false` does the trick)

2. ChangeLog now included

[Jonathan Hammer]

Created attachment 354196 [details]
Patch

Converted tabs to spaces

[Wenson Hsieh]

Comment on attachment 354196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354196&action=review

Nice! r=me, provided that EWS is happy.

> LayoutTests/fast/events/drag-and-drop-move-not-copy.html:18
> +   var range = document.createRange();

Do we need to create and set up this `range` here? It seems that setting base and extent below should be sufficient to make a selection.

[Wenson Hsieh]

Comment on attachment 354196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354196&action=review

> Source/WebCore/ChangeLog:9
> +        Test: fast/events/drag-and-drop-move-not-copy.html

Minor nit - we usually put this "Test:" line after the ChangeLog description, on a separate line. (You can see the other entries in this file for reference).

[Jonathan Hammer]

Thanks for spotting those. I will submit an updated patch.

[Jonathan Hammer]

Created attachment 354200 [details]
Patch

1. Removed unnecessary code from the test
2. Fixed ChangeLog formatting

[EWS Watchlist]

Comment on attachment 354200 [details]
Patch

Attachment 354200 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9904156

New failing tests:
fast/events/drag-and-drop-move-not-copy.html

[EWS Watchlist]

Created attachment 354206 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Jonathan Hammer]

Created attachment 354210 [details]
Patch

Test should be skipped on AppleWin port because drag and drop doesn't work in DRT (according to the TestExpectations file: <rdar://5621244>)

[WebKit Commit Bot]

Comment on attachment 354210 [details]
Patch

Clearing flags on attachment: 354210

Committed r237986: <https://trac.webkit.org/changeset/237986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.