609632 - Image files dragged and dropped into compose window don't get inserted as image

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Jonathan Kamens]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12
Build Identifier: 

If you drag and drop a JPG, GIF or PNG file from either Windows or Linux (Nautilus) into the Thunderbird compose window, it gets inserted into the message you're composing as an image.

If, on the other hand, you drag-and-drop a BMP file, a "file://..." link gets inserted into the body of the message.

A BMP file should be inserted as an image like the others.


Reproducible: Always

Comment 1

[rsx11m]

Confirmed for Windows on TB3.3a1pre recent trunk build. I'm seeing the familiar "Security Error: Content at about:blank may not load or link to <path>" in the error console, thus I assume that's a case which remained denied by bug 520189, though the fix for bug 572637 wasn't considering specific image types (Ehsan?).

On a side note, BMP isn't the most efficient format for either inline images or attaching them, given the lack of compression, but that's another topic...

Comment 2

[(no longer active)]

Is this a regression?  If yes, what did the HTML generated for the BMP images before the regression look like?

Comment 3

[Alice0775 White]

FYI, 
If I perform  Insert > Image... > Choose File..  *bmp > OK, I can insert BMP into Composing editor.

Comment 4

[Joe Sabash [:JoeS1]]

http://hg.mozilla.org/mozilla-central/rev/794555fec7aa

Should bmp be added here, or am I stretching my understanding of the code.

@@ -1378,64 +1377,6 @@ NS_IMETHODIMP nsHTMLEditor::InsertFromTr
    1.12          rv = InsertTextAt(stuffToPaste, aDestinationNode, aDestOffset, aDoDeleteSelection);
    1.13        }
    1.14      }
    1.15 -    else if (0 == nsCRT::strcmp(bestFlavor, kFileMime))
    1.16 -    {
    1.17 -      nsCOMPtr<nsIFile> fileObj(do_QueryInterface(genericDataObj));
    1.18 -      if (fileObj && len > 0)
    1.19 -      {
    1.20 -        
    1.21 -        nsCOMPtr<nsIURI> uri;
    1.22 -        rv = NS_NewFileURI(getter_AddRefs(uri), fileObj);
    1.23 -        NS_ENSURE_SUCCESS(rv, rv);
    1.24 -        
    1.25 -        nsCOMPtr<nsIURL> fileURL(do_QueryInterface(uri));
    1.26 -        if (fileURL)
    1.27 -        {
    1.28 -          PRBool insertAsImage = PR_FALSE;
    1.29 -          nsCAutoString fileextension;
    1.30 -          rv = fileURL->GetFileExtension(fileextension);
    1.31 -          if (NS_SUCCEEDED(rv) && !fileextension.IsEmpty())
    1.32 -          {
    1.33 -            if ( (nsCRT::strcasecmp(fileextension.get(), "jpg") == 0 )
    1.34 -              || (nsCRT::strcasecmp(fileextension.get(), "jpeg") == 0 )
    1.35 -              || (nsCRT::strcasecmp(fileextension.get(), "gif") == 0 )
    1.36 -              || (nsCRT::strcasecmp(fileextension.get(), "png") == 0 ) )
    1.37 -            {
    1.38 -              insertAsImage = PR_TRUE;

Comment 5

[rsx11m]

Well, the patch cited by Joe is from a very recent security patch not present in currently nightlies yet (try again tomorrow), thus can't be the cause.

But, it's disturbing in a way that it appears to actually remove the ability to drop PNG/JPG/GIF into a message body as well. I hope this isn't going to happen as it would be a *serious* removal of a substantial use case for mail/news.

Having said that, the if() statement removed by this patch would indeed have been required to have a "bmp" clause to set "insertAsImage", thus selecting the correct code (<IMG> rather than <A>) in the following clause.

Comment 6

[rsx11m]

Ok, this changes the scope of this bug dramatically. I've downloaded tinderbox build 1288924886 (20101104194126) for SM 2.1b2pre and find that drag-and-drop of /any/ image file (including PNG or JPEG) does no longer work for Mail/News and the Composer, this is an unacceptable regression.

I do not have access to bug 512717 to see the reasoning of that removal, but it's obvious from code review and timing that it is the cause.

Comment 7

[rsx11m]

This didn't land on mozilla-1.9.2 yet, but I'd suggest blocking for that branch (and 1.9.1 if applicable) as well if that apparent security fix is supposed to go into the release branches as well for the next dot release.

Comment 8

[(no longer active)]

Attachment: Part 1: Insert image files dropped onto editable areas as data: URI images

We can use the machinery added in bug 490879 to inject image files dropped into editable areas as data: URI images.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Will that not affect editor stuff that edits locally and then uploads (rewriting the file:// URIs in the process)?

Comment 10

[(no longer active)]

Attachment: Part 2: Support inserting BMP images by dropping or pasting into editable areas

This patch adds the BMP mimetype to the list of accepted image formats.

Comment 11

[rsx11m]

(In reply to comment #8)
> We can use the machinery added in bug 490879 to inject image files dropped into
> editable areas as data: URI images.

Creating a snapshot of the file's contents to insert it as a "data:" URI would
also resolve a major part of bug 404922 and make drag-and-drop vs. copy-paste consistent. I'd like that solution (and thanks for reacting so quickly!).

Comment 12

[(no longer active)]

(In reply to comment #9)
> Will that not affect editor stuff that edits locally and then uploads
> (rewriting the file:// URIs in the process)?

I'm not quite sure what editor stuff you're talking about here...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Last I checked, the Seamonkey suite's HTML editor component, as well as other Gecko-based editor products (NVu, etc), allowed editing a site locally and then uploading it via FTP to a server.

Comment 14

[rsx11m]

Good point. My guess is that the contents of such image files will be resolved
as data: URIs as well when the page is edited, thus hard-wired in the HTML code and no longer updated when the source images are updated. Neil should know.

Comment 15

[[e-mail]]

I believe it uses webbrowserpersist, which ignores it because data: is defined as a non-persistable resource.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It sounds to me like those editors will work OK. There won't be an external file, but uploaded pages will still work.

It seems appropriate to me that editor apps should be responsible for "externalizing" image URLs if they want images to be external, since only the app really knows how to do that properly.

Also, is it possible for apps to override the drag-drop behavior directly? I hope so.

Comment 17

[Boris Zbarsky [:bzbarsky]]

They'll work unless one also edits the image... at which point the image edits won't show up in the page and will break, no?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's pretty normal for drag-and-drop to copy instead of link. That seems like an OK default to me.

Comment 19

[(no longer active)]

(In reply to comment #16)
> It sounds to me like those editors will work OK. There won't be an external
> file, but uploaded pages will still work.

That should be the case, unless they explicitly strip data: URI images, or something like that.

> It seems appropriate to me that editor apps should be responsible for
> "externalizing" image URLs if they want images to be external, since only the
> app really knows how to do that properly.

I agree.

> Also, is it possible for apps to override the drag-drop behavior directly? I
> hope so.

It is.  They can listen to the "drop" event, and access the file object(s) dropped through event.dataTransfer.files, and call event.preventDefault() if they do not want the default editor provided drop behavior to take effect.

Comment 20

[Benjamin Smedberg]

Does this change affect contentEditable or drag/drop behavior visible to untrusted content? I'm a little worried about the ability of content to initiate a drag session for content in an iframe which they don't control, and then drop it into content they do control in order to steal interesting data.

Comment 21

[Jesse Ruderman]

Like in bug 605991?  We'll have to fix that anyway, and I don't think supporting image drops really makes it worse.

Comment 22

[Benjamin Smedberg]

Comment on attachment 488483 [details] [diff] [review]
Part 1: Insert image files dropped onto editable areas as data: URI images

I missed that these weren't reviewed yet. Please don't nominate for approval until reviews are complete.

Comment 23

[(no longer active)]

(In reply to comment #21)
> Like in bug 605991?  We'll have to fix that anyway, and I don't think
> supporting image drops really makes it worse.

I agree.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why do we need to hardcode image types here? Shouldn't we be able to look up the file extension in extraMimeEntries via  nsExternalHelperAppService::FillMIMEInfoForExtensionFromExtras, and if it's an "image/" type we should then go ahead and create an <img> element with a data: URL containing the file data. (Then it would be a simple extension to create <audio> and <video> elements for audio and video files ... although maybe we don't want to embed video as data: URLs...)

Comment 25

[(no longer active)]

Attachment: Part 2: Support inserting any image type by dropping or pasting into editable areas

(In reply to comment #24)
> Why do we need to hardcode image types here? Shouldn't we be able to look up
> the file extension in extraMimeEntries via 
> nsExternalHelperAppService::FillMIMEInfoForExtensionFromExtras, and if it's an
> "image/" type we should then go ahead and create an <img> element with a data:
> URL containing the file data.

Done.

> (Then it would be a simple extension to create
> <audio> and <video> elements for audio and video files ... although maybe we
> don't want to embed video as data: URLs...)

Hmm, isn't the same thing true for audio as well?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It makes more sense to me to fold the patches together instead of adding some code and then removing it.

(In reply to comment #25)
> Hmm, isn't the same thing true for audio as well?

I guess, although many audio files are quite small.

Comment 27

[(no longer active)]

Landed the two patches folded together:

http://hg.mozilla.org/mozilla-central/rev/f576b6e16d46

Comment 28

[(no longer active)]

(In reply to comment #26)
> (In reply to comment #25)
> > Hmm, isn't the same thing true for audio as well?
> 
> I guess, although many audio files are quite small.

True, but do we want different behaviors based on the side of the dropped file?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No.

Actually you could argue that we should allow video and audio to be dropped as data: URLs, because there isn't anything else reasonable that we can do.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Except maybe upload the video to Youtube etc and embed a link to it --- but that's something we don't want to hardcode in Gecko :-)

Comment 31

[(no longer active)]

(In reply to comment #29)
> No.
> 
> Actually you could argue that we should allow video and audio to be dropped as
> data: URLs, because there isn't anything else reasonable that we can do.

Can we handle large (several hundred megabytes) URI values reliably (in terms of performance, especially?)  I haven't really tested this...

(In reply to comment #30)
> Except maybe upload the video to Youtube etc and embed a link to it --- but
> that's something we don't want to hardcode in Gecko :-)

Well, technically websites are able to do that using HTML5 drag/drop APIs!

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> (In reply to comment #29)
> > No.
> > 
> > Actually you could argue that we should allow video and audio to be dropped as
> > data: URLs, because there isn't anything else reasonable that we can do.
> 
> Can we handle large (several hundred megabytes) URI values reliably (in terms
> of performance, especially?)  I haven't really tested this...

No. We would have to limit it somehow.

Comment 33

[(no longer active)]

I pushed two followup fixes:

http://hg.mozilla.org/mozilla-central/rev/9ca8a80bd900
http://hg.mozilla.org/mozilla-central/rev/d81f20da03b8

Comment 34

[(no longer active)]

OK, I had recompiled with my new part 2 patch while the patch was popped, so unsurprisingly the functionality was broken (in addition to builds).

Comment 35

[(no longer active)]

Attachment: Followup

Comment 36

[(no longer active)]

Follow-up landed:

http://hg.mozilla.org/mozilla-central/rev/fdb62b0ef699

Comment 37

[rsx11m]

Works nice in the most recent tinderbox build (tested TB 3.3a1pre with PNG), the only observed issue is that it's causing now bug 578104 in mail/news since the original filename got lost in the process (that's a regression from prior behavior where the leaf of the path was used as attachment name).

I'll amend bug 578104 respectively, but this may need some follow-up bug in editor to somehow provide the filename leaf for applications which need it.

Comment 38

[rsx11m]

Verified fixed in Thunderbird (Shredder/Miramar 3.3a1) and SeaMonkey current nightly (mail/news and composer) on Windows XP, Windows 7, and Linux x86_64.