User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release) Build ID: 20130618035212 Steps to reproduce: We're trying to use the clipboardData in a paste event now that it's been fixed in Firefox 22, but when pasting an image (not an image file) on Windows, the types and files on clipboardData are empty. I set the event handler up and hit PrtScrn and Ctr+V to paste. Tested on a textarea and then on a contenteditable div. The image actually shows up in the div, but in the event.clipboardData there's no sign of it. Have personally ONLY tested this on Firefox on Windows. Actual results: clipboardData.files.length is 0 clipboardData.types.length is 0 clipboardData.mozItemCount is 0 Expected results: The image should show up as a "image/png" in clipboardData.types, and calling clipboardData.getData("image/png") should return the blob: url for it ... Alternatively, "Files" should show up in clipboardData.types, and a (fake) file should be available in in clipboardData.files, with type "image/png" ... Note: Chromium handles image pastes through their "items" array as a image/png object that you have to call getAsFile() on.

Note, according to this StackOverflow article [1], previous to 22 it was possible to set focus to a contenteditable div during the paste event in order to extract images from there, however this definitely does not work now (in Firefox 22). [1]: http://stackoverflow.com/questions/14151018/can-one-hack-paste-image-support-into-a-textarea-in-firefox/14423247

This probably belongs in Core / DOM: Core & HTML like 407983

Yes, we don't support pasting images yet. Note though that neither the clipboard or drag/drop specifications specify how images should be handled, or what form they should take.

How is this application then be able to paste images on FF24, http://stackoverflow.com/questions/6393280/paste-image-into-rich-text-like-gmail ? Perhaps there's already a way to do this.

Update: (from bug 921359) http://jsbin.com/EPOWitI/6/edit http://jsbin.com/iXIFOZe/1/edit http://jsbin.com/ARIYuqE/1/edit Same thing with FF26+ Normally, even if the specifications don't mention it, the behaviour should be like the one Chrome has implemented. It's a very good feature to have for web apps that deal with images and data.

Specifications says: http://www.w3.org/TR/clipboard-apis/#processing-model "If the current clipboard part contains data in another supported binary or text-based format: - Determine the MIME type of the data - Add one entry to the DataTransferItemList with drag data item kind set to file, drag data item type string set to the corresponding MIME type" I think if the OS clipboard contains images or other binary files they should be attached to the DataTransferItemList (clipboardData.items) and also to clipboardData.files.

Please also add text/html format support

This definitely seems like an oversight since the image data is available when you drag and drop an image and are listening for the drop event. On the drop event, you have a dataTransfer property which is of type DataTransfer which is the exact same type as clipboardData. Except in this case, the data is there.

We don't support the image/png, image/gif or image/jpeg types in the clipboard or in drag and drop. Most applications supply them as files, urls or text or provide these as alternate forms, and these can be used from both the clipboard and drag and drop. Our underlying code does support the image types, but the data transfer object just doesn't expose them.

That's the point, Chrome already does for jpeg/png and you can build applications with it. Is there a specific reason why the data transfer object does not expose them?..

Chrome actually support following formats (at least from why I found): text/plain text/richtext text/html imge/png

We have every reason to support these formats, along with jpeg. :) Of course, unless there is a security reason not to do this, but well it would not be in Chrome either way.

(In reply to sys.sgx from comment #11) > That's the point, Chrome already does for jpeg/png and you can build > applications with it. Is there a specific reason why the data transfer > object does not expose them?.. The spec only had support for text types when the DataTransfer was implemented. Hence, this bug.

To implement this, DataTransfer::GetFiles would need to be changed to look for one of the image types if the file type wasn't present, then convert or store the nsIInputStream we use internally into an nsIDOMFile. Then, CacheExternalDragFormats and CacheExternalClipboardFormats would need to have the image types added to the type arrays they use. This should be enough to get it to work on the platforms where we support image types.

Looks like some nice steps towards it.

I'm giving at shot at this following Comment 15. With this patch the test attached to this bug passes but still I can't paste images on Facebook or GMail. I'm investigating whether we need other things on our side.

OK I polished the patch. Ehsan, can you take a look? I created a new DOMFileImpl* object because I couldn't find an easy way to create a DOMFile (or an nsIFile) from an nsIInputStream, that seemed the natural solution. Also, with this patch we always have two images, the jpeg version and the png, I read through the specs but couldn't find an expected behavior for this. Thanks! Try: https://tbpl.mozilla.org/?tree=Try&rev=5c5bc99b6205

Comment on attachment 8489804 [details] [diff] [review] Support pasting images in clipboardData Baku should look at the DOMFile changes, and Neil is probably a better reviewer for everything else!

Comment on attachment 8489804 [details] [diff] [review] Support pasting images in clipboardData Review of attachment 8489804 [details] [diff] [review]: ----------------------------------------------------------------- The issue here is that, DataTransfer.files returns a FileList object and a FileList object contains Files. With your patch, you are creating Blobs because you use the constructor: DOMFileImplBase(const nsAString& aContentType, uint64_t aLength) : mIsFile(false) This set mIsFile is to false and when this happens the DOMFile obj uses just nsIDOMBlob interface and not nsIDOMFile. Try to use a different constructor of DOMFileImplBase if you are able to find a valid 'name' for the File obj. Maybe you can send the name into the variant? Write a mochitest please and check that the return value is actually a File object.

Comment on attachment 8489804 [details] [diff] [review] Support pasting images in clipboardData > for (uint32_t i = 0; i < count; i++) { >- nsCOMPtr<nsIVariant> variant; >- aRv = MozGetDataAt(NS_ConvertUTF8toUTF16(kFileMime), i, getter_AddRefs(variant)); >- if (aRv.Failed()) { >- return nullptr; >- } >+ const nsTArray<TransferItem>& item = mItems[i]; >+ for (uint32_t j = 0; j < item.Length(); j++) { >+ nsCOMPtr<nsIVariant> variant; >+ aRv = MozGetDataAt(item[j].mFormat, i, getter_AddRefs(variant)); This should only be checking for the file and image types. We probably also want to favour the file type if it exists over the image types.

So, the variant is generated inside nsClipboard that has a different implementation for every DE (gtk, qt, windows etc). I don't think we want to change anything there, right? I was thinking of using "image.<type>" as a name for the file, generating it in DataTransfer directly, would that work in your opinion, baku?

> I was thinking of using "image.<type>" as a name for the file, generating it > in DataTransfer directly, would that work in your opinion, baku? I'm fine with that. bz, what do you think?

That seems fine. Worth checking what other UAs do in this case, though, and whether it's worth the spec defining it.

I did some testing across UAs. IE11: Only supports images for existing files. WebKit: Only supports images for existing files. Blink: Supports pasting and dragging images but .files contains a Blob. They're considering moving to a File with a placeholder name as well [1] [1] https://code.google.com/p/chromium/issues/detail?id=361145#c2

OK added mochitest, in this patch we return a File object with name "image.{jpg,gif,png}" depending on the type. Also addressed Neil's comments. We first check if the Variant can be cast as a File, if not we try to cast it as an image nsIInputStream (only if the mime type is JPG/GIF/PNG).

Comment on attachment 8507538 [details] [diff] [review] Support pasting images in clipboardData v2 Review of attachment 8507538 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but can I see it again before landing it? ::: content/base/public/File.h @@ +605,5 @@ > + FileImplStream(nsIInputStream* aStream, const nsAString& aName, > + uint64_t aLength, const nsAString& aContentType) > + : FileImplBase(aName, aContentType, aLength) > + , mStream(aStream) > + , mContentType(aContentType) remove this. ContentType is already stored by FileImplBase. @@ +608,5 @@ > + , mStream(aStream) > + , mContentType(aContentType) > + {} > + > + virtual nsresult GetInternalStream(nsIInputStream** aStream) MOZ_OVERRIDE; this can be inline. @@ +613,5 @@ > +private: > + ~FileImplStream() {} > + > + nsCOMPtr<nsIInputStream> mStream; > + nsString mContentType; This is not needed. ::: dom/events/DataTransfer.cpp @@ +295,3 @@ > > + aRv = MozGetDataAt(item[j].mFormat, i, getter_AddRefs(variant)); > + if (aRv.Failed()) { NS_WARN_IF @@ +300,2 @@ > > + if (!variant) { NS_WARN_IF @@ +339,5 @@ > } > > +already_AddRefed<File> > +DataTransfer::CreateFileFromImageStream(nsISupports* aParent, > + nsIInputStream* stream, aStream @@ +340,5 @@ > > +already_AddRefed<File> > +DataTransfer::CreateFileFromImageStream(nsISupports* aParent, > + nsIInputStream* stream, > + const nsAString& aMimeType) Would be nice if this was: already_AddRefed<File> DataTransfer::CreateFileFromImageStream(nsISupports* aParent, nsIInputStream* aStream, const nsAString& aMimeType, ErrorResult& aRv); @@ +349,5 @@ > + } else if (aMimeType.EqualsASCII(kGIFImageMime)) { > + fileName = NS_LITERAL_STRING("image.gif"); > + } else if (aMimeType.EqualsASCII(kPNGImageMime)) { > + fileName = NS_LITERAL_STRING("image.png"); > + } else { MOZ_ASSERT_UNREACHABLE(something) @@ +352,5 @@ > + fileName = NS_LITERAL_STRING("image.png"); > + } > + > + uint64_t available; > + stream->Available(&available); nsresult rv = ? if (NS_WARN_IF(NS_FAILED(rv)) ... You should use aRv here. ::: dom/events/DataTransfer.h @@ +245,5 @@ > + // creates a File object from a stream which contains an image of type > + // aMimeType > + static already_AddRefed<File> > + CreateFileFromImageStream(nsISupports* aParent, > + nsIInputStream* stream, aStream ::: dom/events/test/test_paste_image.html @@ +48,5 @@ > + var files = e.clipboardData.files; > + > + ok(files.length > 0, "There should be at least one file in the clipboard"); > + for (var i = 0; i < files.length; i++) { > + ok(files[i] instanceof File, ".files should contain only File objects"); check the name, the size and the contentType. Would be cool if you also use this File with a FileReader and check the content.

Thanks for the quick review baku! If fixed all your comments, and also while writing the tests I discovered two crashes ooops. One of them was caused by the fact that I wasn't calling NS_IF_ADDREF here: 51 + virtual nsresult GetInternalStream(nsIInputStream** aStream) MOZ_OVERRIDE { 52 + NS_IF_ADDREF(*aStream = mStream); 53 + return NS_OK; 54 + } And the other wan was caused by the fact that CreateSlice was not implemented. I added tests for these things. For the FileReader part, I'm not really sure what could I do with it? The problem is that the file inside the clipboard is different from the file copied, I'm guessing there's some encoding going on (there is for sure for the image/jpg version), I went around this using a canvas, we copy the image and the source in two different canvases and we check that the generated file is identical. This tests pass for a red (#F00) png/image. Given that the image is encoded I think it's better not to check for the exact size (it could vary by implementation), so I'm just checking that it's >0, all in all we have: size > 0 check, image pixel-size check, canvas pixel-by-pixel check. Please let me know if you feel we need more tests, and thanks! Linux try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e529670dc357 https://tbpl.mozilla.org/?tree=Try&rev=e529670dc357

Comment on attachment 8508408 [details] [diff] [review] Support pasting images in clipboardData v3. r=baku Review of attachment 8508408 [details] [diff] [review]: ----------------------------------------------------------------- Good test. FileReader is not needed because you use canvas to check the content of the file.

Comment on attachment 8508408 [details] [diff] [review] Support pasting images in clipboardData v3. r=baku Thanks! Neil, do you want to take another look at this?

Comment on attachment 8508408 [details] [diff] [review] Support pasting images in clipboardData v3. r=baku >+already_AddRefed<File> >+DataTransfer::CreateFileFromImageStream(nsISupports* aParent, >+ nsIInputStream* aStream, >+ const nsAString& aMimeType, >+ ErrorResult& aRv) >+{ >+ nsAutoString fileName; >+ if (aMimeType.EqualsASCII(kJPEGImageMime)) { >+ fileName = NS_LITERAL_STRING("image.jpeg"); >+ } else if (aMimeType.EqualsASCII(kGIFImageMime)) { >+ fileName = NS_LITERAL_STRING("image.gif"); >+ } else if (aMimeType.EqualsASCII(kPNGImageMime)) { >+ fileName = NS_LITERAL_STRING("image.png"); >+ } else { >+ MOZ_ASSERT_UNREACHABLE("Unsupported mime type"); >+ } How are these filenames used? Would they get used in a UI or used for a real filename? The names would need to be localized if so. >+ var textarea = SpecialPowers.wrap(document.getElementById('textarea')); >+ textarea.focus(); >+ textarea.editor.paste(1); Use the clipboard.kGlobalClipboard constant here as well.

Thanks Neil! The name may very well used in UI, I added three strings in dom.properties. Also, while writing tests for another bug, I noticed that the nsIInputStream that we get from nsClipboard is not thread safe (I was trying to display the same image twice, added tests in this patch as well). I resolved just copying over the stream and using a MemoryFile, similarly to what we already do with <canvas> in HTMLCanvasElement::MozGetAsFileImpl. This way there's no need to add a new implementation object for File. Sorry for the multiple review requests!

Neil, do you want me to find another reviewer for this?

Comment on attachment 8513136 [details] [diff] [review] Support pasting images in clipboardData v4. r=baku r=enndeakin >+ uint64_t available; >+ aRv = aStream->Available(&available); >+ if (NS_WARN_IF(aRv.Failed())) { >+ return nullptr; >+ } >+ >+ void* imgData = nullptr; >+ aRv = NS_ReadInputStreamToBuffer(aStream, &imgData, available); >+ if (NS_WARN_IF(aRv.Failed())) { >+ return nullptr; >+ } >+ >+ return File::CreateMemoryFile(aParent, imgData, available, fileName, >+ aMimeType, PR_Now()); I'm not familiar enough with this file stuff to know if this is ok. You might want to ask for feedback from someone. Since NS_ReadInputStreamToBuffer reads the entire stream synchronously, a comment here should be added as to why this is ok. What happens if you retrieve 'dataTransfer.files twice? I suspect that since the stream has already been read from, that there won't be anything to read the second time. >+ this.returned = function() { >+ counter++; >+ info("returned=" + counter + "images.length=" + images.length); Add a space before 'images.length=' to make the output test more readable.

Unfortunately I don't have time for this now :-(

Could this get reassigned for review? I'd love to see this feature land soon.

Could this get reassigned for review? I'd love to see this feature land soon.

It would sure be good to get this in, but the patch as-is doesn't merge simply - some very relevant stuff in DataTransfer.cpp has changed and it takes a person more fluent in C and Mozilla internals than me to tell how this should work now.

I think I figured out how to adapt the patch after all, but there are some open questions from previous reviews that I don't know how to address..

I added back in the test file (which went misisng from the patch at some point), and got it so that it passes on my local machine. For some reason, the testImageSize test is failing with pngs (the colours are slightly off), so I only have it enabled on gifs right now. (It also fails on jpegs for the same reason, the reasons might be related? I don't know anything about this kind of stuff :S) This hasn't done anything to deal with any of the review comments, it is just now updated to actually build on my local machine.

Slightly more up-to-date tests, I added a quick test to make sure that multiple accesses of the file property on the clipboardData property will return the same object (and thus, the inputstream won't be read multiple times). I added an option about potentially replacing the inputstream in the items array after reading it in with a BlobImpl (as the inputstream would be used up), but I haven't tested it at all. Neil, do you have any suggestions as to who I should pester for feedback on the other concerns you brought up in your review?

baku may be a good person to ask for feedback.

Comment on attachment 8651085 [details] [diff] [review] Support for images in paste events Hey baku, can you look at the input stream stuff stuff Neil was talking about in comment 34?

> What happens if you retrieve 'dataTransfer.files twice? I suspect that since > the stream has already been read from, that there won't be anything to read > the second time. It should be "cached". In DataTransfer class we have mFiles for this. Correct? This is populate at the first time GetFiles() is called, and it's used for the next calls.

(In reply to Andrea Marchesini (:baku) from comment #44) > > What happens if you retrieve 'dataTransfer.files twice? I suspect that since > > the stream has already been read from, that there won't be anything to read > > the second time. > > It should be "cached". In DataTransfer class we have mFiles for this. > Correct? > This is populate at the first time GetFiles() is called, and it's used for > the next calls. Yeah, I think it is cached, but there might be other code which will attempt to access the original items array at some other point. It might make sense to do what I was considering in the patch and change the original entry into a BlobImpl entry, so that future accesses would succeed even if they weren't cached. Not sure yet though.

Comment on attachment 8651085 [details] [diff] [review] Support for images in paste events Did you verify this with both e10s enabled and disabled? Also, this will implement retrieving images when they are drag-dropped as well. Just a note for anyone documenting this.

Comment on attachment 8651085 [details] [diff] [review] Support for images in paste events Removing the feedback flag because this patch is becoming obsolete to the mega-patch in bug 906420

This is being fixed in bug 906420 - closing as dup

Re-opening as this bug isn't related to bug 906420.

(In reply to Neil Deakin from comment #50) > Re-opening as this bug isn't related to bug 906420. It was originally closed because the patch which I attached to bug 906420 would also fix this problem. Unfortunately, the patch has been delayed for a very very long time. - I'm hoping to land it soon though (just need to rebase again)

Note: if/when a QA person wants to test this eventual fix I'd love to help..

Just ran into this bug while building a paste handler for the Imgur upload UI. I would love to see this fixed, as our current solution relies on having the focus on a contenteditable field. Please let me know if I can assist in any way.

Michael, any idea when bug 906420 might land?

(In reply to Boris Zbarsky [:bz] from comment #54) > Michael, any idea when bug 906420 might land? I'm hoping to land it within a week or so. I'm going to be finishing school, and thus having free time to actually work on mozilla stuff soon, which means that I can actually get around to un-bitrotting the code, and will be actually avaliable to deal with the potential fallout.

We need this to be able to have our design application interact with other native apps. We can get at text, but images never appear in the DataTransfer object. Chrome is currently downsampling images on Retina Macs when pasted, so make sure that the clipboard contents are 1:1 when this is fixed.

baku is taking over bug 906420, which also includes the fix for this bug.

This has been fixed in bug 906420 which has stuck the landing.