Bug 25916 - WebKit should expose files from the pasteboard in drop events
Summary: WebKit should expose files from the pasteboard in drop events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on: 25915 25924 25907
Blocks: 26711 26712
  Show dependency treegraph
 
Reported: 2009-05-21 01:24 PDT by Eric Seidel (no email)
Modified: 2010-12-04 02:37 PST (History)
10 users (show)

See Also:


Attachments
Expose files in the clipboard in ondrop events (18.41 KB, patch)
2009-05-21 01:40 PDT, Eric Seidel (no email)
mjs: review-
Details | Formatted Diff | Diff
Updated patch to not break other builds. (25.15 KB, patch)
2009-06-08 14:26 PDT, Eric Seidel (no email)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-05-21 01:24:55 PDT
WebKit should expose files from the pasteboard in drop events

This would make it possible for applications like gmail to implement
drag and drop of attachments onto email messages.

We will need to follow the HTML5 drag and drop security model:
http://www.w3.org/TR/html5/editing.html#security-risks-in-the-drag-and-drop-model

Internet Explorer shows historical documentation of supporting
getData('File') as a way of exposing files on the pasteboard.  The current version of their docs:
http://msdn.microsoft.com/en-us/library/ms537658(VS.85).aspx
has removed this reference (likely IE never implemented it)
I have a printed copy on my desk from 2008 describing getData('File') in IE.

Mozilla has equivalent file access:
event.dataTransfer.mozGetDataAt("application/x-moz-file", 0);
which also does not return a typed value.
https://developer.mozilla.org/En/DragDrop/Recommended_Drag_Types#Dragging_Files
Comment 1 Eric Seidel (no email) 2009-05-21 01:40:53 PDT
Created attachment 30530 [details]
Expose files in the clipboard in ondrop events

 12 files changed, 314 insertions(+), 3 deletions(-)
Comment 2 Maciej Stachowiak 2009-05-22 02:13:30 PDT
Comment on attachment 30530 [details]
Expose files in the clipboard in ondrop events

Conceptually, I think this is awesome. It would be great to have this feature. And the Mac implementation looks fine. r- for the following reasons:

1) Implementations for other platforms are missing, in fact it looks like it will break all other builds as-is.

2) I think we should bring this up on the whatwg list ASAP, maybe even before comitting anything, to make sure we end up aligned with other browsers on this.
Comment 3 Eric Seidel (no email) 2009-06-08 14:25:31 PDT
Ok. Happy to update this to not break other builds.  I also of course will watch the bots when it lands and fix any errors I missed.  Since WebKit doesn't have try-servers I wont know until I land if this will break things like wx or gtk.
Comment 4 Eric Seidel (no email) 2009-06-08 14:26:37 PDT
Ian is aware of the intent to add this.  I don't personally participate in the what-wg list, but I'm sure others concerned about this issue do.
Comment 5 Eric Seidel (no email) 2009-06-08 14:26:44 PDT
Created attachment 31064 [details]
Updated patch to not break other builds.

 22 files changed, 381 insertions(+), 9 deletions(-)
Comment 6 Adam Barth 2009-06-09 12:33:12 PDT
What's the status of this bug?  Do we need to talk more with whatwg or should we review this patch?
Comment 7 Adam Barth 2009-06-09 12:40:06 PDT
(In reply to comment #6)
> What's the status of this bug?  Do we need to talk more with whatwg or should
> we review this patch?

For the record, I think we should go ahead with this patch.  :)
Comment 8 Eric Seidel (no email) 2009-06-10 01:26:37 PDT
Btw, HTMLInputElement.files was added in http://trac.webkit.org/changeset/34702.  input.files does not seem to be covered by any spec:
http://www.w3.org/TR/file-upload/
http://www.w3.org/TR/html5/forms.html#the-input-element

But I'm glad we have it all the same. :)
Comment 9 Eric Seidel (no email) 2009-06-10 01:30:49 PDT
Actually  input.files is a moz invention:
https://developer.mozilla.org/En/NsIDOMFileList
Comment 10 Eric Seidel (no email) 2009-06-10 01:44:17 PDT
Adam has kindly posted to the WhatWG:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-June/020302.html

Still looking for content review... (Although my sense from before was that the patch was fine it just was gonna break builds).
Comment 11 Maciej Stachowiak 2009-06-10 05:21:49 PDT
To be clear, we don't need a full official draft spec to land this. I brought it up mainly to make sure we didn't skip that step, and that developers of other browser engines had a chance to bring up concerns with the specifics of the API. Seems like that was done sufficiently.

What's the behavior on platforms where the API is not implemented? Will 0 get safely turned into null? The code looks ok to me on quick overview but since I'm on vacation it's probably best to have someone give the official stamp of approval. I would suggest Sam, Adele or Oliver as a good reviewer.
Comment 12 Eric Seidel (no email) 2009-06-10 09:57:37 PDT
Yes, 0 is safely turned into null. (This is only ever called from the wrapping bindings.)  The Mac code used to return 0 on permission failure until I changed it to return an empty list.  (I can change all the others to return an empty list instead of 0 as well.)
Comment 13 Maciej Stachowiak 2009-06-19 11:57:47 PDT
Comment on attachment 31064 [details]
Updated patch to not break other builds.

I'm going to say r=me with the following caveats:

1) Please file follow-up bugs for platforms that don't implement the back end for this yet, at least for ports that have a public buildbot.

2) I think for platforms that don't support file drop yet, it might be better to avoid having a "files" property at all on the Clipboard object, instead of having the property and always returning null. So I think it would be good to give the feature an ENABLE define.
Comment 14 Eric Seidel (no email) 2009-06-24 23:53:02 PDT
(In reply to comment #13)
> (From update of attachment 31064 [details] [review])
> I'm going to say r=me with the following caveats:
> 
> 1) Please file follow-up bugs for platforms that don't implement the back end
> for this yet, at least for ports that have a public buildbot.

Sure thing!  I've filed bug 25916 bug 25917 and bug 25918 for the necessary ports.

> 2) I think for platforms that don't support file drop yet, it might be better
> to avoid having a "files" property at all on the Clipboard object, instead of
> having the property and always returning null. So I think it would be good to
> give the feature an ENABLE define.

Hum.  I disagree.  I don't think this should be guarded by ENABLE defines.  That's a lot of extra work to hide this property while the other ports implement the necessary plumbing.

Returning null makes it pretty clear that files are not available.  It doesn't make it clear that it's due to a missing feature of those ports, but I think that given this is an experimental DOM feature of WebKit to begin with, it's OK that the null is ambiguous.  We can add ENABLE defines if it becomes clear later that some port wants to ship with this disabled.  Our current build system potpourri makes it very heavy-weight to add ENABLEs w/o good cause. :)
Comment 15 Eric Seidel (no email) 2009-06-24 23:54:11 PDT
Sorry.  Bug 26710 bug 26711 and bug 26712
Comment 16 Eric Seidel (no email) 2009-06-25 02:06:13 PDT
s/without good cause/without strong need/
Comment 17 Eric Seidel (no email) 2009-06-25 02:24:39 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/http/tests/security/clipboard/clipboard-file-access-expected.txt
	A	LayoutTests/http/tests/security/clipboard/clipboard-file-access.html
	A	LayoutTests/http/tests/security/clipboard/resources/TEMPLATE.html
	A	LayoutTests/http/tests/security/clipboard/resources/apple.gif
	A	LayoutTests/http/tests/security/clipboard/resources/clipboard-file-access.js
	A	LayoutTests/http/tests/security/clipboard/resources/mozilla.gif
	M	WebCore/ChangeLog
	M	WebCore/dom/Clipboard.h
	M	WebCore/dom/Clipboard.idl
	M	WebCore/platform/chromium/ClipboardChromium.cpp
	M	WebCore/platform/chromium/ClipboardChromium.h
	M	WebCore/platform/gtk/ClipboardGtk.cpp
	M	WebCore/platform/gtk/ClipboardGtk.h
	M	WebCore/platform/mac/ClipboardMac.h
	M	WebCore/platform/mac/ClipboardMac.mm
	M	WebCore/platform/qt/ClipboardQt.cpp
	M	WebCore/platform/qt/ClipboardQt.h
	M	WebCore/platform/win/ClipboardWin.cpp
	M	WebCore/platform/win/ClipboardWin.h
	M	WebCore/platform/wx/ClipboardWx.cpp
	M	WebCore/platform/wx/ClipboardWx.h
Committed r45168
http://trac.webkit.org/changeset/45168
Comment 18 Dimitri Glazkov (Google) 2009-06-25 08:52:25 PDT
Chromium build fix landed as http://trac.webkit.org/changeset/45176.