191400 - returning DRAG_MOVE, not DRAG_NONE, after html drag to browser window

Status: REOPENED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[w]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.2.1) Gecko/20021130

Files deleted when dragged to mozilla from Eclipse's resource views.

(not true of netscape or IE).

See eclipse bug at 
   http://bugs.eclipse.org/bugs/show_bug.cgi?id=30543

Reproducible: Always

Steps to Reproduce:
1. Running NT 4.0 SP6, Mozilla 1.2.1 
2. Install and launch Eclipse 2.0.2 from http://www.eclipse.org
3. Create file.html in the resource view
4. Drag file.html to a Mozilla open browser window
Actual Results:  
file.html displays in browser window, but file.html deleted from resource view
and filesystem.



Expected Results:  
Open the file, but return DRAG_NONE feedback so eclipse does not delete it.

Once again: see eclipse bug at 
   http://bugs.eclipse.org/bugs/show_bug.cgi?id=30543

Comment 1

[Knut Radloff]

Note that IE gives DROP_LINK feedback which seems to make a little more sense 
than DROP_MOVE. However, the important part is not to return DROP_MOVE to the 
drag source.

Comment 2

[Jon Henry]

Confirmed on WinXP, 20031209 build of Firebird and Thunderbird.

A similar (probably the same issue) bug exists in Thunderbird, bug 224414. When
dragging a file into the compose window, the default action is to move the file
rather than copy it. Since dragging-and-dropping an attachment from an FTP site
seems to not be accepted by Thunderbird, the file is simply deleted. Outlook
Express defaults to the "copy" mode (the cursor with a little plus sign in the
corner) when attachments are dragged onto it, so it avoids this problem.
Although the chance of many people trying this is probably low, it's definitely
a dataloss issue.

Comment 3

[Jon Henry]

Would it ever be the case that we want to use the move action rather than, say,
link or copy? It seems like it's just asking for trouble. I can make a patch to
change the default from move to something safer, if that's something that would
be accepted.

Comment 4

[Jon Henry]

*** Bug 234886 has been marked as a duplicate of this bug. ***

Comment 5

[:deanis74]

1. DRAG_MOVE being returned.

I think this is in nsNativeDragTarget::GetGeckoDragAction.  I'm not sure because
I don't have a build in front of me.  It makes sense that it's hitting this line:

  *aGeckoAction = nsIDragService::DRAGDROP_ACTION_MOVE;

http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsNativeDragTarget.cpp#139

2. Note that IE gives DROP_LINK feedback which seems to make a little more sense 
than DROP_MOVE.

This very well could be in the same function.

Comment 6

[Jon Henry]

Attachment: Possible patch

You're right Dan, this is a patch I made a month or so ago that seems to work.
It makes the default DRAG_COPY rather than DRAG_MOVE, but with modifier keys
you can get any behavior.

Comment 7

[:deanis74]

This only gives DROPEFFECT_LINK if Control and Shift are pressed.  Don't we want
that as the default feedback if canLink is true?

Comment 8

[Mike Cowperthwaite]

John Henry: if that patch no longer compiles, could you update it?
And then please request a review for the attachment -- I guess you could start 
with Neil, since he's got the bug assigned to him.

Comment 9

[[e-mail]]

Sounds more like ere or dean's cup of tea, actually.

Comment 10

[Son Le]

Attachment: patch v0

how about this?

default to link/copy, but can override with CONTROL/SHIFT keys.

Comment 11

[:deanis74]

Comment on attachment 233781 [details] [diff] [review]
patch v0

I like that this makes the behavior more like IE and Windows, but does it actually fix the originally-reported bug?  I was just speculating what the cause was.

Comment 12

[Son Le]

I haven't tested this using Eclipse 2.0.2, but have confirmed that it does fix bug 224414. 

The way I understand it, on the drop event a drop target indicates the action it will undertake (DROPEFFECT_MOVE/COPY/LINK) so the drop source can respond if it wants to. 

In the some apps (such as Eclipse and Win Explorer), on a DROPEFFECT_MOVE it deletes the original source as it assumes the drop target will "own" the object. However all that happens is it is displayed.

Actually this patch will only change the default behaviour to be potentially less destructive. If the user performs a DROPEFFECT_MOVE action (ie. holds done shift), the same thing will still occur.

I guess the other option is to remove the DROPEFFECT_MOVE action altogether.

Comment 13

[:deanis74]

You should be able to combine this into one 'if' statement now, shouldn't you?

if (canLink)
  ...
else if (grfKeyState & MK_CONTROL)
  ...
else if (mCanMove && (grfKeyState & MK_SHIFT))
  ...

That is, if people agree that this is the way we want to go.

Comment 14

[Mike Cowperthwaite]

(In reply to comment #12)
> I guess the other option is to remove the DROPEFFECT_MOVE action altogether.

I don't see a case for ever using _MOVE on a drag to any Mozilla window, but maybe I'm not thinking about it hard enough.  Possibly, maybe, someone might like to 'move' a file from their disk to a mail attachment, but I don't think that case is worth supporting, given the potential for losing the file.

Comment 15

[Son Le]

(In reply to comment #13)
> You should be able to combine this into one 'if' statement now, shouldn't you?

Not really. What if you don't press shift/control and can't link? Then nothing will happen,... unless you add a final 'else' for COPY.

(In reply to comment #14)
> I don't see a case for ever using _MOVE on a drag to any Mozilla window, but
> maybe I'm not thinking about it hard enough.

I tend to agree, I don't think mozilla should accept MOVE actions by default. However, I can imagine some future enhancement/extension where you might want to do this.

Comment 16

[:deanis74]

(In reply to comment #15)
> (In reply to comment #13)
> > You should be able to combine this into one 'if' statement now, shouldn't you?
> 
> Not really. What if you don't press shift/control and can't link? Then nothing
> will happen,... unless you add a final 'else' for COPY.

Sorry, I missed that from the patch.  You would need the final 'else' still.

   } else {
     *aGeckoAction = nsIDragService::DRAGDROP_ACTION_COPY;
     *pdwEffect    = DROPEFFECT_COPY;
   }

Comment 17

[Son Le]

Attachment: patch v0.1

Updated patch. Had to rejig the logic cf. comment 13 to match old logic.

Comment 18

[Son Le]

Comment on attachment 234215 [details] [diff] [review]
patch v0.1

ere, can we get your thoughts on this?

Comment 19

[Ere Maijala (slow)]

Comment on attachment 234215 [details] [diff] [review]
patch v0.1

There could be a case when an extension or another xul app would want capability to MOVE a file, so I wouldn't remove that completely. I think it's enough to make it require a modifier to do so. Might be of benefit to check other platforms though, and if they don't support move we might not really need it on Windows either. 

I'm not a super-reviewer, sorry.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 234215 [details] [diff] [review]
patch v0.1

I think this is a good idea. Prevents data loss.

Comment 21

[Son Le]

dean, can you give r+ please?

Comment 22

[:deanis74]

Comment on attachment 234215 [details] [diff] [review]
patch v0.1

sure, why not?

Comment 23

[Son Le]

can someone please checkin for me? thanks.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Is bug 224414 FIXED now?

mozilla/widget/src/windows/nsNativeDragTarget.cpp 	1.36

Comment 25

[Son Le]

(In reply to comment #24)
> Is bug 224414 FIXED now?

Yes. This patch fixes it.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #25)
> (In reply to comment #24)
> > Is bug 224414 FIXED now?
> 
> Yes. This patch fixes it.

Can you resolve it accordingly, then? :)

Comment 27

[Son Le]

(In reply to comment #26)
> Can you resolve it accordingly, then? :)

Sorry, no can do - don't have permission. 

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #27)
> (In reply to comment #26)
> > Can you resolve it accordingly, then? :)
> 
> Sorry, no can do - don't have permission. 

Oh! I resolved it, I'll see if I can get someone to give you editbugs permissions.

Comment 29

[Mike Cowperthwaite]

This has caused a regression: the standard drag-message-to-folder operation in Thunderbird (and I assume in Seamonkey MailNews as well) is now Copy instead of Move: bug 356429.

Comment 30

[Destian]

This appears to mess up drag and drop on the bookmarks sidebar as well as the customization panel.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

I suppose this patch should probably backed out for the moment, given the regressions, unless you have a fix in mind.

Comment 32

[Son Le]

(In reply to comment #31)
> I suppose this patch should probably backed out for the moment, given the
> regressions, unless you have a fix in mind.

Yeah, sounds like a good idea as I don't have too much free time right now.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Backed out.

Comment 34

[Mark Banner (:standard8)]

Given that changing the default drag action caused problems in other areas of the code, would it be possible to define a method for specifying/overriding the default drag action by the drag target?

Would we be able to apply this to all toolkits? For instance, I would be able to fix bug 349044 in a nice way.

Comment 35

[ch-bugzilla]

I'm a little shocked that this bug is seemingly back after over ten years, but I just ran into it in FF29 when writing code that implements file move via drag and drop.

To reproduce, write source code to:

1. Create an IDataObject containing a file.

2. Use DoDragDrop with DROPEFFECT_MOVE set to offer the IDataObject for move via drag and drop.

3. Compile and execute under debugger.

4. Drag and drop onto Firefox.

5. When DoDragDrop returns, DROPEFFECT_MOVE will be set in pdwEffect.

6. If test application honors DROPEFFECT_MOVE as is specified in MSDN documentation (i.e. by deleting the source file), the file that was dragged to Firefox will be deleted by the test application.

This results in unexpected, unwanted, and irrecoverable destruction of user data.

Note: this bug cannot be replicated by dragging files from Explorer.

I will attach a compilable test application.

Comment 36

[ch-bugzilla]

Attachment: Test tool for DROPEFFECT_MOVE bug

Change #define victim before compiling.

Intellisense warnings regarding HANDLE_MSG are a VS bug; code will compile correctly.

Comment 37

[Mats Palmgren (inactive)]

If anyone wants to investigate, I think our DND code on Windows lives around here:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeDragTarget.cpp?rev=1e7a720a0f63#222

Comment 38

[Peter Wong]

Firefox use IDataObject.QueryGetData and CFSTR_PREFERREDDROPEFFECT to determine the preferred drop effect.
If drag source's preferred drop effect is DROPEFFECT_NONE and allow DROPEFFECT_MOVE and DROPEFFECT_COPY,
Firefox prefer DROPEFFECT_MOVE over DROPEFFECT_COPY.

It is a rare case that when you drag text or file from other program to a browser, you want the text or file to be delete in the source program. So I think firefox should give preference to DROPEFFECT_COPY over DROPEFFECT_MOVE when dragging from other programs to firefox.

If it is not good for user experience then we should just ignore CFSTR_PREFERREDDROPEFFECT.

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 40

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.