Bug 168424 – [GTK] Drag and drop is always moving the content even if copy is requested

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Tomas Popela]

Drag and drop is always moving the content even if copy is requested (by pressing the Control key when dropping).

[Tomas Popela]

Created attachment 301753 [details]
Patch

[Build Bot]

Comment on attachment 301753 [details]
Patch

Attachment 301753 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3133120

New failing tests:
editing/pasteboard/drag-drop-copy-content.html

[Build Bot]

Created attachment 301759 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301753 [details]
Patch

Attachment 301753 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3133173

New failing tests:
editing/pasteboard/drag-drop-copy-content.html

[Build Bot]

Created attachment 301760 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

Comment on attachment 301753 [details]
Patch

So I see two problems here:

 (a) The test is failing for Mac WK1. Does Mac have the equivalent of ctrl+drag? If so, the test should be marked as failing until it can be implemented for Mac. If not, I guess it should be skipped.
 (b) GDK supports lots of other drag actions. DEFAULT, COPY, MOVE, LINK, PRIVATE, ASK. Is this the only additional one that we want to support in WebKit? Should we file bugs to implement the others?

[Tomas Popela]

(In reply to comment #6)
>  (a) The test is failing for Mac WK1. Does Mac have the equivalent of
> ctrl+drag? If so, the test should be marked as failing until it can be
> implemented for Mac. If not, I guess it should be skipped.

If you see the test you will see that they do have, it's pressing alt key while dropping. I'm going to fix it for the Mac WK1 (once I will find how to run the test locally on Mac using WK1).

>  (b) GDK supports lots of other drag actions. DEFAULT, COPY, MOVE, LINK,
> PRIVATE, ASK. Is this the only additional one that we want to support in
> WebKit? Should we file bugs to implement the others?

I really don't know. I think the just MOVE and COPY is fine (and I think that we are advocating support for these two for the web view).

[Tomas Popela]

(In reply to comment #7)
> (In reply to comment #6)
> >  (a) The test is failing for Mac WK1. Does Mac have the equivalent of
> > ctrl+drag? If so, the test should be marked as failing until it can be
> > implemented for Mac. If not, I guess it should be skipped.
> 
> If you see the test you will see that they do have, it's pressing alt key
> while dropping. I'm going to fix it for the Mac WK1 (once I will find how to
> run the test locally on Mac using WK1).

OK, it looks like WK1 doesn't support it on Mac (if tried in MiniBrowser -1), but it works on WK2 if tried through Safari. I will skip the test on WK1 then.

[Tomas Popela]

Created attachment 301906 [details]
Patch

Marked the layout test as failing in the mac WK1 and created a bug 168503 for it. Previously I wrote that it does not work on WK1, but that was because I was connected through VNC to the mac machine, and the alt key doesn't work there.

[Michael Catanzaro]

Comment on attachment 301906 [details]
Patch

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

LGTM. I'd like Carlos to review it too.

> LayoutTests/editing/pasteboard/drag-drop-copy-content.html:32
> +    if (isGtk() || isEfl())

EFL is gone now, so remove this.

[Carlos Garcia Campos]

Comment on attachment 301906 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Mark the editing/pasteboard/drag-drop-copy-content.html as failing
> +        as WTR doesn't know how to perform drag and drop in WK2. Also the
> +        test does not pass on the mac WK1, created a bug for it.

So, we are adding a test that doesn't pass in any platform?

> LayoutTests/editing/pasteboard/drag-drop-copy-content-expected.txt:3
> +FAIL: input

Shouldn't this be PASS? According to the test when it fails it says FAIL: expected value="Text", actual value=, no?

> LayoutTests/editing/pasteboard/drag-drop-copy-content.html:7
> +function debug(msg) {

This is the actual test result, not a debug message no? I would call this log instead of debug if that is the case.

> LayoutTests/editing/pasteboard/drag-drop-copy-content.html:14
> +function editingTest() {

I would call this runTest()

> LayoutTests/editing/pasteboard/drag-drop-copy-content.html:42
> +    testRunner.notifyDone();

I'm not sure we need to use wait/notify in a test like this.

> LayoutTests/editing/pasteboard/drag-drop-copy-content.html:47
> +<p>This tests the generic drag and drop where the copy of the content is
> +requested (pressing Control on WebKitGTK+ or Alt on Mac when dropping the
> +content) instead of the move.</p>

Since DND can't be tested by WTR in WK2, ity would be useful to have a manual way to test this, explained here. Other test do that and it's useful to check that the test works using MiniBrowser.

[Tomas Popela]

(In reply to comment #11)
> > LayoutTests/ChangeLog:10
> > +        Mark the editing/pasteboard/drag-drop-copy-content.html as failing
> > +        as WTR doesn't know how to perform drag and drop in WK2. Also the
> > +        test does not pass on the mac WK1, created a bug for it.
> 
> So, we are adding a test that doesn't pass in any platform?

Yes, but it will start to pass once the bugs are fixed.

> > LayoutTests/editing/pasteboard/drag-drop-copy-content-expected.txt:3
> > +FAIL: input
> 
> Shouldn't this be PASS? According to the test when it fails it says FAIL:
> expected value="Text", actual value=, no?

It definitely needs to be PASS and I had it like that, but it probably got replaced by run-webkit-tests. :/

> > LayoutTests/editing/pasteboard/drag-drop-copy-content.html:7
> > +function debug(msg) {
> 
> This is the actual test result, not a debug message no? I would call this
> log instead of debug if that is the case.
> 
> > LayoutTests/editing/pasteboard/drag-drop-copy-content.html:14
> > +function editingTest() {
> 
> I would call this runTest()
> 
> > LayoutTests/editing/pasteboard/drag-drop-copy-content.html:42
> > +    testRunner.notifyDone();
> 
> I'm not sure we need to use wait/notify in a test like this.

The test is based on editing/pasteboard/drag-drop-input-textarea.html and it does include these as well, but I will rework it.

[Tomas Popela]

Created attachment 302123 [details]
Patch

[Tomas Popela]

Comment on attachment 302123 [details]
Patch

Clearing flags on attachment: 302123

Committed r212881: <http://trac.webkit.org/changeset/212881>

[Tomas Popela]

All reviewed patches have been landed.  Closing bug.

[Tomas Popela]

There was a followup fix to this:

http://trac.webkit.org/projects/webkit/changeset/213638