Bug 191228 - Plain text drag in contenteditable is always DragOperationCopy, never DragOperationMove
Summary: Plain text drag in contenteditable is always DragOperationCopy, never DragOpe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Local Build
Hardware: Mac macOS 10.14
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-03 14:50 PDT by Jonathan Hammer
Modified: 2018-11-08 07:36 PST (History)
7 users (show)

See Also:


Attachments
Xcode project to reproduce issue (31.85 KB, application/zip)
2018-11-03 14:50 PDT, Jonathan Hammer
no flags Details
Patch (999 bytes, patch)
2018-11-06 22:47 PST, Jonathan Hammer
rniwa: review-
Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2018-11-07 18:57 PST, Jonathan Hammer
no flags Details | Formatted Diff | Diff
Patch (5.31 KB, patch)
2018-11-07 19:03 PST, Jonathan Hammer
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2018-11-07 19:31 PST, Jonathan Hammer
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews203 for win-future (12.70 MB, application/zip)
2018-11-07 21:22 PST, EWS Watchlist
no flags Details
Patch (6.10 KB, patch)
2018-11-07 23:29 PST, Jonathan Hammer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Hammer 2018-11-03 14:50:21 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-11-03 14:53:27 PDT
<rdar://problem/45786830>
Comment 2 Wenson Hsieh 2018-11-03 15:06:36 PDT
(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.
Comment 3 Wenson Hsieh 2018-11-03 15:27:08 PDT
> 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.
Comment 4 Wenson Hsieh 2018-11-03 15:27:51 PDT
(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)
Comment 5 Jonathan Hammer 2018-11-03 15:31:06 PDT
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.
Comment 6 Jonathan Hammer 2018-11-06 22:47:18 PST
Created attachment 354065 [details]
Patch
Comment 7 Wenson Hsieh 2018-11-07 08:18:05 PST
(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.
Comment 8 Jonathan Hammer 2018-11-07 09:57:21 PST
(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!
Comment 9 Wenson Hsieh 2018-11-07 12:44:10 PST
(adding a couple more folks who are familiar with drag and drop)
Comment 10 Jonathan Hammer 2018-11-07 13:21:17 PST
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.
Comment 11 Ryosuke Niwa 2018-11-07 13:27:55 PST
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.
Comment 12 Jonathan Hammer 2018-11-07 18:57:43 PST
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
Comment 13 Jonathan Hammer 2018-11-07 19:03:08 PST
Created attachment 354196 [details]
Patch

Converted tabs to spaces
Comment 14 Wenson Hsieh 2018-11-07 19:05:48 PST
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.
Comment 15 Wenson Hsieh 2018-11-07 19:13:12 PST
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).
Comment 16 Jonathan Hammer 2018-11-07 19:29:44 PST
Thanks for spotting those. I will submit an updated patch.
Comment 17 Jonathan Hammer 2018-11-07 19:31:03 PST
Created attachment 354200 [details]
Patch

1. Removed unnecessary code from the test
2. Fixed ChangeLog formatting
Comment 18 EWS Watchlist 2018-11-07 21:22:07 PST
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
Comment 19 EWS Watchlist 2018-11-07 21:22:18 PST
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
Comment 20 Jonathan Hammer 2018-11-07 23:29:24 PST
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>)
Comment 21 WebKit Commit Bot 2018-11-08 07:36:15 PST
Comment on attachment 354210 [details]
Patch

Clearing flags on attachment: 354210

Committed r237986: <https://trac.webkit.org/changeset/237986>
Comment 22 WebKit Commit Bot 2018-11-08 07:36:17 PST
All reviewed patches have been landed.  Closing bug.