Add CF_HDROP support back into Windows drag and drop data object (dragging images to some applications, e.g. PhotoShop, fails)

VERIFIED FIXED in mozilla1.9.1

Status

()

P2
enhancement
VERIFIED FIXED
11 years ago
5 years ago

People

(Reporter: bccarlso, Assigned: jimm)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {verified1.9.1})

Trunk
mozilla1.9.1
x86
Windows XP
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
wanted1.9.1 +
blocking1.9.0.1 -
blocking1.9.0.4 -
wanted1.9.0.x +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

With Firefox 2.0, I used to be able to drag images directly from Firefox (both off of a regular .html page or from .jpg URI image location) into the album art section in MP3Tag, a program for editing ID3 information on MP3s. I now have to drag from Firefox into a Windows Explorer window, and _then_ into MP3Tag.

Reproducible: Always

Steps to Reproduce:
1. Open MP3Tag
2. Visit page with image or direct image URL location
3. Drag (unsuccessfully) from Firefox into album art section (either in main pane or 'Extended Tags...' window)


Expected Results:  
The image should be allowed into its designated area in MP3Tag, as it was in Firefox 2.0
(Assignee)

Comment 1

11 years ago
Ben, does the mp3tag program save the file out or something usually? Can you give a bit more detail on what your trying to do? Not much changed in drag and drop for xp, although we did add drag image support.
(Reporter)

Comment 2

11 years ago
Jim, the program stores the image into the ID3 info for each MP3. (The program is free, so you can reproduce this if you'd like, but..) Let's say I want to use this album art: http://ecx.images-amazon.com/images/I/51MfHB97LtL._SS500_.jpg

In FF2, I could drag it from viewing it in Firefox into the pane in MP3Tag, it would allow me to insert the image that way, in either of the two spots where you see album art in this screenshot: http://farm3.static.flickr.com/2012/2104607798_ffe1d16096.jpg

You then hit OK, and the album is embedded in the ID3 tag. But with FF3, it doesn't allow you to do this. Try dragging the image in the first link into the status bar on the bottom of Firefox, you see that O with a / through it, you get the same thing in MP3Tag. Does that help?
(Assignee)

Comment 3

11 years ago
One datapoint - in Vista, I'm unable to drag anything into these windows. (from the desktop, ff3, or ie7). I'll take a look in an xp image.
(Assignee)

Comment 4

11 years ago
Actually, cancel that. Something was acting funny on my end, after I restarted the app it now appears to be working in Vista. I can also confirm in XP.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

11 years ago
Assignee: nobody → jmathies
Component: General → OS Integration
Version: unspecified → 3.0 Branch
(Assignee)

Comment 5

11 years ago
Looks like this came out of a patch a while back that removed CF_HDROP support from our data object. This app is only accepting that and without it, refuses to work with us.

(Assignee)

Updated

11 years ago
Blocks: 203307
(Assignee)

Comment 6

11 years ago
Looks like what we used to do was copy the file out of the cache, put it in a temp location and then hand the path to the requesting drop handler when we receive a CF_HDROP request. I'm not entirely sure we want to add all that code back in. It's quite a bit of bloat. The folks who develop the tagging app could address this by updating their drop code. However, this does work with other browsers so maybe it's a parity thing we want to support.
(Assignee)

Updated

11 years ago
Duplicate of this bug: 441217
(Assignee)

Comment 8

11 years ago
Looks like we'll need a fix...
(Assignee)

Updated

11 years ago
Flags: blocking1.9.0.1?
(Assignee)

Comment 9

11 years ago
Created attachment 326708 [details] [diff] [review]
cf_hdrop support patch v.1
(Assignee)

Comment 10

11 years ago
Created attachment 326715 [details] [diff] [review]
cf_hdrop support patch v.1 

turned off debug output plus some code and commenting cleanup.
Attachment #326708 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #326715 - Flags: review?(emaijala)
(Assignee)

Updated

11 years ago
Summary: dragging images into mp3tag → Add CF_HDROP support back into Windows drag and drop data object
QA Contact: general → os.integration
This missed 1.9.0.1, but definitely wanted for a branch patch.
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Keywords: regression

Comment 12

11 years ago
Comment on attachment 326715 [details] [diff] [review]
cf_hdrop support patch v.1 

The async implementation in bug 203307 fixed bug 245861 too. As far as I can see, that problem would resurface with this patch. We need to track the files and make sure they're cleaned up upon exit or such.
Attachment #326715 - Flags: review?(emaijala) → review-
(Assignee)

Comment 13

11 years ago
> The async implementation in bug 203307 fixed bug 245861 too. As far as I can
> see, that problem would resurface with this patch. We need to track the files
> and make sure they're cleaned up upon exit or such.

There is code in there that tracks, and I just tested to be sure, it's cleaning up the temp file after the drop operation completes. 
(Assignee)

Comment 14

11 years ago
- delete all files within the temp-dir (the system temp, not the firefox cache)
- open a site that contains at least one picture (a non-link picture)
- open a new tab and switch back to site whith the picture
- drag&drop this picture to the new tab 

Actually, I should have tested - this specific scenario does leave an image behind. :/ 

Comment 15

11 years ago
Most likely that mp3tag just does not support IStream drag and drop method. This is why functionality described in comment #0 stopped working. CF_HDROP is for local files, I know that small files are downloaded by the time user drags something, but for bigger files (or slow connection) it would fail. That was the original intention of the patch for bug 203307 - to allow user to drag big files or specify what to download.

Comment 16

11 years ago
I've contacted the developer of mp3tag, he said he'll be using CF_DIB instead of CF_HDROP in the next version, so there it makes no sence to restore it here :)
(Assignee)

Comment 17

11 years ago
>I've contacted the developer of mp3tag, he said he'll be using CF_DIB instead
>of CF_HDROP in the next version, so there it makes no sence to restore it here
>:)

Unfortunately at least one other major app suffers from this as well, so I'm still inclined to address it at some point.
Duplicate of this bug: 323555
Assignee: jmathies → nobody
Component: OS Integration → Widget: Win32
Product: Firefox → Core
QA Contact: os.integration → win32
Version: 3.0 Branch → Trunk
Assignee: nobody → jmathies
Summary: Add CF_HDROP support back into Windows drag and drop data object → Add CF_HDROP support back into Windows drag and drop data object (dragging images to some applications, e.g. PhotoShop, fails)
Flags: wanted1.9.1?

Comment 19

10 years ago
gavin, you said 

"Indeed, in a build with the patch from bug 440911, the drag to PhotoShop
succeeds."

how can i add the patch from this page to firefox 3.0 so it works for me also. can you host the version yourself?
Flags: blocking1.9.0.3?
(In reply to comment #19)
> how can i add the patch from this page to firefox 3.0 so it works for me also.
> can you host the version yourself?

I suppose I could, if you really need it urgently, but I really would suggest waiting until it's at least landed on the trunk (after which you can download a nightly). The current patch isn't ready yet because of comment 14.
(Assignee)

Comment 21

10 years ago
dan, you can try it, but this patch has a timing flaw so you might get sporadic results in your drag drops. 

Comment 22

10 years ago
i don't know what that even means, but sporadic results is definitely better than none at all and i'm definitely willing to test it out.

since i have no experience in decompiling, recompiling, patching, etc., can you host a compiled version of ff 3 with the patch in it so i can test it out? can you email me it or host it?
Flags: wanted1.9.1? → wanted1.9.1+
Priority: -- → P2
(Assignee)

Comment 23

10 years ago
Created attachment 335795 [details] [diff] [review]
cf_hdrop support patch v.2
Attachment #326715 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Severity: minor → major
(Assignee)

Comment 24

10 years ago
I'm not happy with the second patch but it does address the problem. The issue is that nsContentAreaDragDrop fires off a request for the content using an nsIWebBrowserPersist and then returns the path to the destination. The drag-drop nsDataObj then returns this to the target saying basically that the file is available. Once the receiver acknoledges receipt, the data object attempts to delete the file and is destroyed. 9 times out of 10 the file is not fully copied over yet, so the delete fails.

Communication between the data object and the content area is handled through a transferable. I haven't found a way to get a hold of the content area persist over in the data object, which seems to be the best solution if it was possible. In the second patch I wait for the delete operation to succeed and then let the nsDataObj destroy. However, if something were to go wrong and the file never lost it's lock, this would create a race condition that could freeze up the browser.

I'd welcome any feedback / suggestions.
Flags: blocking1.9.0.4?
Flags: blocking1.9.0.4+
Any update on this?
(Assignee)

Comment 26

10 years ago
>Any update on this?

No, I'll be working on this again next week. cf_hdrop support is addressed but I still need to find a better way of dealing with left over files.
Flags: in-litmus?
(Assignee)

Updated

10 years ago
Attachment #335795 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Severity: major → enhancement
(Assignee)

Updated

10 years ago
Blocks: 344248
(Assignee)

Updated

10 years ago
No longer blocks: 344248
(Assignee)

Updated

10 years ago
Blocks: 460969
(Assignee)

Comment 27

10 years ago
Created attachment 344099 [details] [diff] [review]
cf_hdrop support patch v.3

This addresses a few problems. Our old cf_hdrop support had a problem in that it relied on delay rendered content (in the form of a temp file) when images were dragged out of the content area. However cf_hdrop was not designed for use in this case - the specs for the format dictate the content must exist before the drag operation completes. Dragged content was being saved through a web persist handler which rarely completed until after the drag operation finished. This in turn caused problems for some apps which didn't react well to the file not being written out completely. Also, since the data object is responsible for removing temp files at the end of the drag, temp files were sometimes being left around due to write locks, creating a privacy issue.

cf_hdrop was removed completely in Fx3 as it was assumed most apps had upgraded to newer clipboard formats. But it turns out a number apps still rely on it in a few cases. Dragging a file into the main window of Photoshop is probably the best example.

This patch adds support back in for cf_hdrop for the few apps out there that still require it. Image content is retrieved by the data object as native image data, then using routines in the clipboard image utilities the image is converted to and written out as a bitmap in the temp dir.  After the drag session closes the data object then deletes the temp file. The resulting functionality is identical to the process we go through when Fx3 copy - pastes image data from the content area.
(Assignee)

Updated

10 years ago
Attachment #344099 - Flags: review?(emaijala)
No time for trunk testing before landing it for 1.9.0.4. Request branch approval when you get a tested, reviewed patch.
Flags: blocking1.9.0.4+ → blocking1.9.0.4-
(Assignee)

Comment 29

10 years ago
+	fileHdr.bfType		    = ((WORD) ('M' << 8) | 'B');
+	fileHdr.bfSize		    = GlobalSize (bits) + sizeof(fileHdr);
+	fileHdr.bfReserved1 	= 0;
+	fileHdr.bfReserved2 	= 0;
+	fileHdr.bfOffBits		  = (DWORD) (sizeof(fileHdr) + bmpHdr->biSize);

Note I'll clean this tab mess up before it lands.

Comment 30

10 years ago
Comment on attachment 344099 [details] [diff] [review]
cf_hdrop support patch v.3

Not particularly elegant, but it works :)
Attachment #344099 - Flags: review?(emaijala) → review+
(Assignee)

Comment 31

10 years ago
(In reply to comment #30)
> (From update of attachment 344099 [details] [diff] [review])
> Not particularly elegant, but it works :)

Yeah I wasn't particularly happy with what I had to do either. But I didn't find any other solutions.
(Assignee)

Updated

10 years ago
Keywords: regression → checkin-needed
(Assignee)

Updated

10 years ago
Flags: blocking1.9.1?
(Assignee)

Comment 32

10 years ago
See comment 27 for details on why this should get into 3.1.

Updated

10 years ago
Attachment #344099 - Flags: approval1.9.1b2?

Updated

10 years ago
Keywords: checkin-needed
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Attachment #344099 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 344099 [details] [diff] [review]
cf_hdrop support patch v.3

a=beltzner

While you're waiting for the tree to go green, make sure that this doesn't bust any tests before landing :)
Patch pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/f3b3edd87633
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081117 Minefield/3.1b2pre. I verified by dragging some images from Firefox to Photoshop CS4.
Status: RESOLVED → VERIFIED

Comment 36

10 years ago
I've just tried this using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081117 Minefield/3.1b2pre and the CF_HDROP is now present, except the image is not in the original format. If I drag a JPG from a web page, the file in the CF_HDROP is a .bmp file. Is this by design?
Keywords: fixed1.9.1
Duplicate of this bug: 470013

Updated

10 years ago
Keywords: verified1.9.1
removing fixed1.9.1 b/c these bugs are verified1.9.1, sorry for the spam.
Keywords: fixed1.9.1
Duplicate of this bug: 474194

Comment 40

9 years ago
Is the CF_HDROP mechanism the same as the WM_DROPFILES mechnism, or is there supposed to be some magic automatic compatibility trnslation going on in Windows to make CF_HDROP drag sources work as WM_DROPFILES sources, too?

Is there some known problem in the WM_DROPFILES support? At least dragging images from Firefox to a GTK+ client doesn't work (the GTK+ app gets the WM_DROPFILES message, but the file name returned in DragQueryFile() does not exist). (Dragging files from Explorer works fine.)
(Assignee)

Comment 41

9 years ago
(In reply to comment #40)
> Is the CF_HDROP mechanism the same as the WM_DROPFILES mechnism, or is there
> supposed to be some magic automatic compatibility trnslation going on in
> Windows to make CF_HDROP drag sources work as WM_DROPFILES sources, too?

Sounds like it. Windows may be querying our ole implementation for a file name and we aren't responding with something appropriate. Any suggestions on a simple GTK+ client we could use to test with?

Comment 42

9 years ago
Well, it's not simple, but GIMP is readily avaibale as an all-inclusive installer, and it accepts WM_DROPFILES style drops. From gimp-win.sf.net. Run it with the environment variable GDK_DEBUG set to "dnd" and the command-line argument --console-messages to get some hopefully easy to interpret debugging printout about DND in a console window. (Or to stdout, if you redirect it, then the --console-messages isn't necessary.)

When dragging an image from Firefox to GIMP, the DND debugging output looks like this:

gdk_window_register_dnd: 00130A60
gdk_window_register_dnd: 001109C6
gdk_window_register_dnd: 001409CA
WM_DROPFILES: 00130A60
gdk_drag_context_init 07AEAD00
... C:\Users\tml\AppData\Local\Temp\wngsw7xh.bmp: file:///C:/Users/tml/AppData/Local/Temp/wngsw7xh.bmp
gdk_selection_owner_get: DROPFILES_DND = 00000000
gdk_selection_convert: 00140A2E DROPFILES_DND text/uri-list
gdk_drop_reply
gdk_selection_property_get: 00140A2E text/uri-list format:8 length:55
gdk_drop_finish
gdk_drop_finish
gdk_drag_context_finalize 07AEAD00
gdk_property_delete: 00140A2E GDK_SELECTION
_gdk_selection_property_delete: 00140A2E (no-op)

and that file C:\Users\tml\AppData\Local\Temp\wngsw7xh.bmp does not exist.

When dragging a file from Explorer, the output looks like this:

gdk_window_register_dnd: 00050BD0
gdk_window_register_dnd: 00040C4E
gdk_window_register_dnd: 00040BD6
WM_DROPFILES: 00050BD0
gdk_drag_context_init 07A14D00
... C:\tmp\d.bmp: file:///C:/tmp/d.bmp
gdk_selection_owner_get: DROPFILES_DND = 00000000
gdk_selection_convert: 00100A12 DROPFILES_DND text/uri-list
gdk_drop_reply
gdk_selection_property_get: 00100A12 text/uri-list format:8 length:23
gdk_drop_finish
gdk_drop_finish
gdk_drag_context_finalize 07A14D00
gdk_property_delete: 00100A12 GDK_SELECTION
_gdk_selection_property_delete: 00100A12 (no-op)

I.e. to the WM_DROPFILES handler in GTK+ everything works the same way, but the file name received from DragQueryFile() is an existing one, the one dragged.
(Assignee)

Comment 43

9 years ago
Ok this makes sense, due to privacy concerns we only leave that file around until the drag operation completes, then we delete it. This gives apps like Photoshop a chance to open the file and display it. But it's not meant to stick around. 

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.cpp#626

We could maybe create a pref option for leaving these files around? But I'm not sure we would want it on by default.

(We should spin this out into a separate bug for discussion.)
(Assignee)

Comment 44

9 years ago
Actually, see bug 245861. ;)
(Assignee)

Comment 45

9 years ago
Tor, do files dragged from IE stick around? Just curious.
It's actually not just a privacy issue, but also a disk usage issue.. most of the time a user has no idea how to get to the tempdir.  But what we could do is delete files after some time elapses, or at app shutdown?  Doesn't windows have a flag that says "delete this file at next restart", too?
(Assignee)

Comment 47

9 years ago
(In reply to comment #46)
> It's actually not just a privacy issue, but also a disk usage issue.. most of
> the time a user has no idea how to get to the tempdir.  But what we could do is
> delete files after some time elapses, or at app shutdown?  Doesn't windows have
> a flag that says "delete this file at next restart", too?

There's a risk there too - applications like photoshop receive a file path we mark for deletion. If the user then edits the file and saves without moving it, we delete it.

It would be better to hand the data directly through CF_DIB to GTK, this requires they update their handling. We added support back in for CF_HDROP specifically because some apps have not yet upgraded their handling. In time the CF_HDROP code should become obsolete.
(In reply to comment #47)
> (In reply to comment #46)
> > It's actually not just a privacy issue, but also a disk usage issue.. most of
> > the time a user has no idea how to get to the tempdir.  But what we could do is
> > delete files after some time elapses, or at app shutdown?  Doesn't windows have
> > a flag that says "delete this file at next restart", too?
> 
> There's a risk there too - applications like photoshop receive a file path we
> mark for deletion. If the user then edits the file and saves without moving it,
> we delete it.

That's fine, I think -- much better than the current situation.  No different than editing a temporary file from IE or something.  CF_DIB doesn't preserve any metadata on the image, and doesn't work for non-images.. so I don't think it's a solution.

Comment 49

9 years ago
A problem on the GTK side is that at this moment WM_DROPFILES is the only kind of inter-process DND that it supports. Sad but true. Nobody has ever been sufficiently motivated to implement the OLE-based (or is that COM-based, I always mix up these acronyms) generic DND stuff. (Partly, at least for me, because it involves doing programming with those interfaces and vtables which is always somewhat tedious in plain C.)

But yeah, I really should try to gather inspiration some weekend and just do it.

The fact that the "file" being dropped is just a temporary one, and thus starting to edit it in an app like GIMP, perhaps saving it back even, would be silly indeed. I should ask what the users complaining in the GIMP and GTK+ bugzilla think should happen if it worked to drag-and-drop and image from Firefox onto GIMP. Should GIMP pretend it is just a normal local file? The ideal would be to open a drag-and-dropped image file so that it is not associated with a file name in the app, like a cut-and-pasted images. Hmm.

One workaround hack would be to make GTK recognize that the file being dropped is a temporary file (hmmm... check if it is in the user's "Temp"... horrible hack indeed), and in that case let GTK copy it to another file whose name then is given to the upper levels...

About IE in comment #45, unfortunately I can't test right now, as for some reason this IE8 doesn't even let me drag images, for some reason. Did MSFT remove this feature, dunno.

Comment 50

9 years ago
In Windows, when you create a file with CreateFile you could set FILE_FLAG_DELETE_ON_CLOSE and a sharing mode that only allows FILE_SHARE_READ. Then so long as you keep the file open, the other application can read it but cannot write it. When you close the file (or even if your process terminates unexpectedly) Windows will delete the file. Perhaps that would be more useful behaviour?

http://msdn.microsoft.com/en-us/library/aa363858(VS.85).aspx
(Assignee)

Comment 51

9 years ago
(In reply to comment #50)
> In Windows, when you create a file with CreateFile you could set
> FILE_FLAG_DELETE_ON_CLOSE and a sharing mode that only allows FILE_SHARE_READ.
> Then so long as you keep the file open, the other application can read it but
> cannot write it. When you close the file (or even if your process terminates
> unexpectedly) Windows will delete the file. Perhaps that would be more useful
> behaviour?
> 
> http://msdn.microsoft.com/en-us/library/aa363858(VS.85).aspx

That's certainly a better solution than what we currently have, although I wonder if it would fix the GTK issue? They don't appear to be opening the file upon receiving WM_DROPFILES, or windows is sending the message after the operation completes. A little testing might be in order.
(Assignee)

Comment 52

9 years ago
Looks like bug 494989 will address the temp file issues. Bug 338478 also adds drop file support.

Updated

7 years ago
Depends on: 728189

Updated

5 years ago
Duplicate of this bug: 485819
You need to log in before you can comment on or make changes to this bug.