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

VERIFIED FIXED in mozilla2.0b8

Status

()

Core
Editor
--
major
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: Jonathan Kamens, Assigned: Ehsan)

Tracking

({regression})

Trunk
mozilla2.0b8
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

(Whiteboard: [tb33needed])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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

7 years ago
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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Is this a regression?  If yes, what did the HTML generated for the BMP images before the regression look like?

Comment 3

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

Comment 4

7 years ago
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

7 years ago
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

7 years ago
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.
Severity: normal → major
Component: Message Compose Window → Editor
Product: Thunderbird → Core
QA Contact: message-compose → editor
Summary: bmp dragged and dropped into compose window doesn't get inserted as image → Image files dragged and dropped into compose window don't get inserted as image
Version: unspecified → Trunk

Comment 7

7 years ago
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.
blocking2.0: --- → ?
Keywords: regression
Blocks: 512717
Whiteboard: [tbtrunkneeds]
Created attachment 488483 [details] [diff] [review]
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #488483 - Flags: review?(roc)
Attachment #488483 - Flags: approval2.0?
Will that not affect editor stuff that edits locally and then uploads (rewriting the file:// URIs in the process)?
Created attachment 488487 [details] [diff] [review]
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.
Attachment #488487 - Flags: review?(roc)
Attachment #488487 - Flags: approval2.0?

Comment 11

7 years ago
(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!).
Blocks: 404922
(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...
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

7 years ago
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.
I believe it uses webbrowserpersist, which ignores it because data: is defined as a non-persistable resource.
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.
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?
It's pretty normal for drag-and-drop to copy instead of link. That seems like an OK default to me.
(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.
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

7 years ago
Like in bug 605991?  We'll have to fix that anyway, and I don't think supporting image drops really makes it worse.
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.
Attachment #488483 - Flags: approval2.0?
Attachment #488487 - Flags: approval2.0?
(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.
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...)
blocking2.0: ? → beta8+
Whiteboard: [tbtrunkneeds] → [tb33needs]
Created attachment 490613 [details] [diff] [review]
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?
Attachment #488487 - Attachment is obsolete: true
Attachment #490613 - Flags: review?(roc)
Attachment #488487 - Flags: review?(roc)
Whiteboard: [tb33needs] → [tb33needs][has patch][needs review roc]
Attachment #488483 - Flags: review?(roc) → review+
Attachment #490613 - Flags: review?(roc) → review+
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.
Landed the two patches folded together:

http://hg.mozilla.org/mozilla-central/rev/f576b6e16d46
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [tb33needs][has patch][needs review roc] → [tb33needs]
Target Milestone: --- → mozilla2.0b8
(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?
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.
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 :-)
(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!
(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.
I pushed two followup fixes:

http://hg.mozilla.org/mozilla-central/rev/9ca8a80bd900
http://hg.mozilla.org/mozilla-central/rev/d81f20da03b8
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 490741 [details] [diff] [review]
Followup
Attachment #490741 - Flags: review?(roc)
Attachment #490741 - Flags: review?(roc) → review+
Whiteboard: [tb33needs] → [tb33needs][needs landing]
Follow-up landed:

http://hg.mozilla.org/mozilla-central/rev/fdb62b0ef699
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [tb33needs][needs landing] → [tb33needs]
Flags: in-litmus?

Comment 37

7 years ago
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.
Blocks: 578104

Comment 38

7 years ago
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.
Status: RESOLVED → VERIFIED
Whiteboard: [tb33needs] → [tb33needed]
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.