Support CF_HDROP format for drag & dropped files

RESOLVED FIXED in mozilla16

Status

()

Core
Widget: Win32
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Timwi, Assigned: bbondy)

Tracking

7 Branch
mozilla16
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110928134238

Steps to reproduce:

• Open a RAR file in WinRAR (version )
• Drag a file (HTML, plain-text, or image) from WinRAR onto Firefox


Actual results:

Nothing happens.


Expected results:

Firefox should open the file.

Other applications I tried — various text editors including Notepad — all worked fine.
(Reporter)

Comment 1

6 years ago
... WinRAR (version 4.01, 32-bit).

Comment 2

6 years ago
Wouldn't this be a WinRAR bug, not a Firefox bug?
(Reporter)

Comment 3

6 years ago
I don’t see how that follows. Dragging files from WinRAR into all other applications seems to work; Firefox is special. Dragging files from other applications (e.g. Explorer) into Firefox also works, which also makes WinRAR special. Only this specified combination of WinRAR+Firefox fails. The bug could lie with either of the two. The bug should be investigated to determine whether it is indeed WinRAR’s bug.

Comment 4

6 years ago
I've reproduced this incompatibility between WinRAR 4.01 and Firefox Nightly 10.0a1 (2011-10-09).

Dragging *from Explorer* succeeds to both Notepad and Firefox.
Dragging *to notepad* succeeds from both Explorer and WinRAR.

Hard to say whether it's Firefox's or WinRAR's fault without investigating.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

6 years ago
I looked into this and winrar uses CF_HDROP
http://msdn.microsoft.com/en-us/library/windows/desktop/bb776902%28v=vs.85%29.aspx#CF_HDROP

CF_HDROP is a global memory object pointing to a DROPFILES structure
http://msdn.microsoft.com/en-us/library/windows/desktop/bb773269%28v=vs.85%29.aspx

The DROPFILES structure containsa list of files.

I can take this bug eventually.
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Component: Shell Integration → Widget: Win32
Product: Firefox → Core
QA Contact: shell.integration → win32
(Assignee)

Updated

6 years ago
Summary: Dragging a file from WinRAR directly to Firefox does nothing (other programs work) → Support CF_HDROP format for drag & dropped files
(Assignee)

Comment 6

5 years ago
I looked a bit more into this and the problem is no longer reproducible with winrar 4.11+.
You can reproduce it with winrar 4.01 though.

The difference of where 4.11 works but 4.01 fails is here:
view\src\nsViewManager.cpp

> // don't cancel the event or set the effectAllowed or dropEffect to
> // "none". This way, if the event is just ignored, no drop will be
> // allowed.
> PRUint32 dropEffect = nsIDragService::DRAGDROP_ACTION_NONE;
> if (nsEventStatus_eConsumeNoDefault == *aStatus) {
> // if the event has a dataTransfer set, use it.
> if (dragEvent->dataTransfer) {
> // get the dataTransfer and the dropEffect that was set on it

*aStatus is set to nsEventStatus_eConsumeNoDefault when it works (with 4.11), but is set to nsEventStatus_eIgnore when it doesn't (with 4.01).


I should also mention that winrar 4.01 and 4.11 work with Firefox 3.6.
I think this code was done when HTML5 drag and drop was added in bug 356295. 
I don't know this code so CC'ing some people who may have some ideas.

Comment 7

5 years ago
Created attachment 623661 [details]
drop test

That isn't the issue. The test here shows that no data is being received on older versions. When dragging from version 4.11, a file and url type is available.
(Assignee)

Comment 8

5 years ago
Using clipspy the CF_HDROP looks the same from a drop with winrar 4.01 and 4.11 so I don't really understand why. 

If we could go into:
> if (nsEventStatus_eConsumeNoDefault == *aStatus) {
Then it would find the dragEvent->dataTransfer data and it would work. 

Also I'm not sure why, but as mentioned in winrar 4.01 it works when you drag to Firefox 3.6 and probably some versions later than that.  

It doesn't hit any drop code by the way, just the drag over.

Updated

5 years ago
Attachment #623661 - Attachment is patch: false
Attachment #623661 - Attachment mime type: text/plain → text/html

Comment 9

5 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Using clipspy the CF_HDROP looks the same from a drop with winrar 4.01 and
> 4.11 so I don't really understand why. 
> 
> If we could go into:
> > if (nsEventStatus_eConsumeNoDefault == *aStatus) {
> Then it would find the dragEvent->dataTransfer data and it would work. 

I'm saying that it wouldn't find any data, since there isn't any data to find. dragEvent->dataTransfer is empty. (If you try the test I posted, dragging onto the 'dragover here' box displays em empty set for me) The browser code sees no file to drop, so doesn't cancel the event. The code you're referencing runs after the dragover event has finished firing. You want to be looking for the problem in code that generates the data/types before the dragover event fires.

What you need to do is look where the native data is being processed and transformed into the data we use. On Windows, it looks like this happens in nsClipboard::GetDataFromDataObject, or somewhere thereabouts.
(Assignee)

Comment 10

5 years ago
OK thanks Neil, I'll take a look.
(Assignee)

Comment 11

5 years ago
Created attachment 623740 [details] [diff] [review]
Patch v1.

So when we're dragging over the browser, the call to DragQueryFileW (which obtains the file count when 0xFFFFFFFF is passed for the index) returns 0.
But in nsClipboard::GetNativeDataOffClipboard, it does return the correct count of dropped files.  If you're dragging 2 files from winrar 4.01 here it will returns a count of 2. 

So to fix this I just pass 1 when 0 is returned from within drag in nsDragService::GetNumDropItems, we don't use that count at all that I know of, as long as it is more than 0 the code in nsClipboard::GetNativeDataOffClipboard will be hit.
I tried to hg annotate to see what caused this regression but it leads to the CVS import.
Attachment #623740 - Flags: review?(neil)
(Assignee)

Comment 12

5 years ago
feedback ping for the patch :)
(Assignee)

Comment 13

5 years ago
Hi Neil (neil@parkwaycc.co.uk),

Is the approach unfavorable, or perhaps you are not the appropriate person for this review?  It's not a high priority task but since it's been a month I thought I'd ask. Thanks!
Sorry, you were unlucky with the timing of your original request, and I've only just got my review queue down to a point where I can think about looking at this.
Comment on attachment 623740 [details] [diff] [review]
Patch v1.

Seems reasonable to me. (I couldn't work out why WinRAR was trying to drop 0 files though.)

>+        if (*aNumItems == 0) {
>+          *aNumItems = 1;
>+        }
>       }
>       else
>         *aNumItems = 1;
>     }
>     else
>       *aNumItems = 1;
>   }
Nit: file style seems to be to avoid {}s ;-)
Attachment #623740 - Flags: review?(neil) → review+
(In reply to Brian R. Bondy from comment #12)
> feedback ping for the patch :)

Also I have all of my auto-CCs turned off.
(Assignee)

Comment 17

5 years ago
> Sorry, you were unlucky with the timing....

No problem, it wasn't high priority so I figured you'd get to it eventually. Thanks for the review!
(Assignee)

Comment 18

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b4eb0d2cafc0
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/b4eb0d2cafc0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.