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

REOPENED
Unassigned

Status

()

Core
Drag and Drop
--
critical
REOPENED
15 years ago
3 years ago

People

(Reporter: w, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dataloss})

Trunk
x86
Windows NT
dataloss
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
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

14 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

14 years ago
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.

Updated

14 years ago
Assignee: firefox → neil.parkwaycc.co.uk

Comment 4

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

Comment 5

14 years ago
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

14 years ago
Created attachment 150377 [details] [diff] [review]
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

14 years ago
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

13 years ago
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

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

Comment 10

11 years ago
Created attachment 233781 [details] [diff] [review]
patch v0

how about this?

default to link/copy, but can override with CONTROL/SHIFT keys.
Attachment #233781 - Flags: review?(dean_tessman)

Comment 11

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
(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

11 years ago
(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

11 years ago
(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

11 years ago
Created attachment 234215 [details] [diff] [review]
patch v0.1

Updated patch. Had to rejig the logic cf. comment 13 to match old logic.
Attachment #233781 - Attachment is obsolete: true
Attachment #234215 - Flags: review?(dean_tessman)
Attachment #233781 - Flags: review?(dean_tessman)

Comment 18

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

ere, can we get your thoughts on this?
Attachment #234215 - Flags: superreview?(emaijala)

Comment 19

11 years ago
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.
Attachment #234215 - Flags: superreview?(emaijala)

Updated

11 years ago
Attachment #234215 - Flags: superreview?(roc)
Comment on attachment 234215 [details] [diff] [review]
patch v0.1

I think this is a good idea. Prevents data loss.
Attachment #234215 - Flags: superreview?(roc) → superreview+

Comment 21

11 years ago
dean, can you give r+ please?

Comment 22

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

sure, why not?
Attachment #234215 - Flags: review?(dean_tessman) → review+

Comment 23

11 years ago
can someone please checkin for me? thanks.

Updated

11 years ago
Assignee: neil → son.le0
Whiteboard: [checkin needed]
Is bug 224414 FIXED now?

mozilla/widget/src/windows/nsNativeDragTarget.cpp 	1.36
Blocks: 224414
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: dataloss
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha

Comment 25

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

Yes. This patch fixes it.
(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

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

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

(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

11 years ago
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.

Updated

11 years ago
Depends on: 356429

Updated

11 years ago
Depends on: 356441

Comment 30

11 years ago
This appears to mess up drag and drop on the bookmarks sidebar as well as the customization panel.
I suppose this patch should probably backed out for the moment, given the regressions, unless you have a fix in mind.

Comment 32

11 years ago
(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.
Whiteboard: [checkin needed (backout)]
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [checkin needed (backout)]
Attachment #234215 - Attachment is obsolete: true
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.
Depends on: 373215
QA Contact: pmac → drag-drop

Comment 35

4 years ago
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

4 years ago
Created attachment 8416998 [details]
Test tool for DROPEFFECT_MOVE bug

Change #define victim before compiling.

Intellisense warnings regarding HANDLE_MSG are a VS bug; code will compile correctly.
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
Assignee: son.le0 → nobody
Target Milestone: mozilla1.9alpha1 → ---
You need to log in before you can comment on or make changes to this bug.