693263 - Support CF_HDROP format for drag & dropped files

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Timwi]

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.

Comment 1

[Timwi]

... WinRAR (version 4.01, 32-bit).

Comment 2

[David Vo [:auscompgeek]]

Wouldn't this be a WinRAR bug, not a Firefox bug?

Comment 3

[Timwi]

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

[Roman]

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.

Comment 5

[Brian R. Bondy [:bbondy]]

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.

Comment 6

[Brian R. Bondy [:bbondy]]

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

[Neil Deakin]

Attachment: 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.

Comment 8

[Brian R. Bondy [:bbondy]]

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.

Comment 9

[Neil Deakin]

(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.

Comment 10

[Brian R. Bondy [:bbondy]]

OK thanks Neil, I'll take a look.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: 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.

Comment 12

[Brian R. Bondy [:bbondy]]

feedback ping for the patch :)

Comment 13

[Brian R. Bondy [:bbondy]]

Hi Neil ([e-mail]),

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!

Comment 14

[[e-mail]]

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 15

[[e-mail]]

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 ;-)

Comment 16

[[e-mail]]

(In reply to Brian R. Bondy from comment #12)
> feedback ping for the patch :)

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

Comment 17

[Brian R. Bondy [:bbondy]]

> 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!

Comment 18

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b4eb0d2cafc0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4eb0d2cafc0