Closed Bug 440911 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Widget: Win32, enhancement, P2)

x86
Windows XP
enhancement

Tracking

()

VERIFIED FIXED
mozilla1.9.1

People

(Reporter: bccarlso, Assigned: jimm)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 3 obsolete files)

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
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.
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?
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.
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: nobody → jmathies
Component: General → OS Integration
Version: unspecified → 3.0 Branch
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.
Blocks: 203307
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.
Looks like we'll need a fix...
Flags: blocking1.9.0.1?
Attached patch cf_hdrop support patch v.1 (obsolete) (deleted) — Splinter Review
Attached patch cf_hdrop support patch v.1 (obsolete) (deleted) — Splinter Review
turned off debug output plus some code and commenting cleanup.
Attachment #326708 - Attachment is obsolete: true
Attachment #326715 - Flags: review?(emaijala)
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 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-
> 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.
- 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. :/
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.
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 :)
>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.
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?
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.
dan, you can try it, but this patch has a timing flaw so you might get sporadic results in your drag drops.
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
Attached patch cf_hdrop support patch v.2 (obsolete) (deleted) — Splinter Review
Attachment #326715 - Attachment is obsolete: true
Severity: minor → major
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?
>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?
Attachment #335795 - Attachment is obsolete: true
Severity: major → enhancement
Blocks: 344248
No longer blocks: 344248
Blocks: 460969
Attached patch cf_hdrop support patch v.3 (deleted) — Splinter Review
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.
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-
+ 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 on attachment 344099 [details] [diff] [review] cf_hdrop support patch v.3 Not particularly elegant, but it works :)
Attachment #344099 - Flags: review?(emaijala) → review+
(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.
Flags: blocking1.9.1?
See comment 27 for details on why this should get into 3.1.
Attachment #344099 - Flags: approval1.9.1b2?
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 :)
Status: NEW → RESOLVED
Closed: 16 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
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: verified1.9.1
removing fixed1.9.1 b/c these bugs are verified1.9.1, sorry for the spam.
Keywords: fixed1.9.1
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.)
(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?
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.
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.)
Actually, see bug 245861. ;)
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?
(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.
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.
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
(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.
Looks like bug 494989 will address the temp file issues. Bug 338478 also adds drop file support.
Depends on: 728189
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: