dropping an image into an editor inserts a link

RESOLVED FIXED

Status

()

Core
Drag and Drop
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Rich Walsh, Assigned: mkaply)

Tracking

(Blocks: 1 bug, {fixed1.8})

Trunk
x86
OS/2
fixed1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
When users drop an image file or link into an HTML editor window, they expect
that the image will be inserted.  Instead, they get a link.  The reason is that
the HTML flavor created by nsDragService is always formatted with an anchor tag.

This patch modifies the HTML creation code to look for extensions GIF, PNG, JPG,
and JPEG.  If found, it uses an IMG tag and sets the 'Alt' text to the URL's
title (if any).

FWIW... it would be really nice if this patch, along with Bug #243270, could
make it into the 1.8 branch.  Together, they would give the OS/2 versions of
Thunderbird 1.5 & Seamonkey 1.0 features that other platforms have had for years.
(Reporter)

Comment 1

12 years ago
Created attachment 196431 [details] [diff] [review]
revises native d&d image handling
Attachment #196431 - Flags: review?(mozilla)
(Assignee)

Comment 2

12 years ago
Just out of curiosity, is this how Windows handles this issue?

Comment 3

12 years ago
I tried this in NVU and it seems to work mostly. In some cases it inserts
strange image source URIs, like
 <img src="../../../m:/Graphics/Tests/anim.gif"
while the correct entry would be something like
 <img src="../../Graphics/Tests/anim.gif"
or just
 <img src="m:/Graphics/Tests/anim.gif"


But, should there not be a way to ask Mozilla for the supported image
extensions? In my MNG builds I would have to add at least two more lines like
                !stricmp(extension.get(), "mng") ||
                !stricmp(extension.get(), "jng") ||
while I am pretty sure that Mozilla keeps the list of supported image type
somewhere already. Otherwise, another one to add for official builds would be BMP.

Ah, there is imgLoader::GetMimeTypeFromContent in
modules\libpr0n\src\imgLoader.cpp that returns NS_ERROR_NOT_AVAILABLE if a
non-supported image type is found and there is some factory stuff (that I never
understood) in modules\libpr0n\build\nsImageModule.cpp that might help as well.
(Reporter)

Comment 4

12 years ago
(In reply to comment #2)
> Just out of curiosity, is this how Windows handles this issue?

Probably not.  IIUC, COM-originated drags provide flavor-data so they don't have
to think about that aspect.  They may still have to compose html - I don't know
but I'll investigate.
(Reporter)

Comment 5

12 years ago
(In reply to comment #3)
> I tried this in NVU and it seems to work mostly. In some cases it inserts
> strange image source URIs, like
>  <img src="../../../m:/Graphics/Tests/anim.gif"

Using a Thunderbird Mail compose window & Seamonkey's Composer, I haven't seen
these anomalies.  You may want to add some printf()s to NativeDragover() &
NativeDataToTransferrable() to watch the filename get changed into a URL & then
get formatted as HTML.

> But, should there not be a way to ask Mozilla for the supported image
> extensions? [snip] Ah, there is imgLoader::GetMimeTypeFromContent in
> modules\libpr0n\src\imgLoader.cpp that returns NS_ERROR_NOT_AVAILABLE if a
> non-supported image type is found

I knew this was less than desirable but I didn't know where to look.  I'll see
if the suggested method fits the bill.
(Reporter)

Comment 6

12 years ago
(In reply to comment #3)
> But, should there not be a way to ask Mozilla for the supported image
> extensions?  Ah, there is imgLoader::GetMimeTypeFromContent in
> modules\libpr0n\src\imgLoader.cpp that returns NS_ERROR_NOT_AVAILABLE

This isn't suitable.  It's a sniffer that takes the first few bytes of the
file's content.  Using during d&d this for a file that may not be stored locally
is impractical at best.  If there were a listing of supported file extensions,
it would still be problematic because it would include .bmp & .ico.  Since only
Windows-format bitmaps & icons are supported, you'd have to use a sniffer to
distinguish between them & the unsupported OS/2 varieties.

> In my MNG builds I would have to add at least two more lines like
> !stricmp(extension.get(), "mng") || !stricmp(extension.get(), "jng")

Is there a commandline -define that identifies the presence of these extra
decoders?  If so, I can add an #ifdef.  Otherwise, I can add an "#if 0" that you
can change to #if 1"  :-)
(Reporter)

Comment 7

12 years ago
> > Just out of curiosity, is this how Windows handles this issue?
>
> I don't know but I'll investigate.

It appears that COM supplies preformatted HTML so the Win code doesn't have to
deal with this at all.

Comment 8

12 years ago
OK, I didn't think about how unpractival sniffing is at that point. Another
thing I found was the nsModuleComponentInfo structure in
xpfe/components/build/nsModule.cpp, but that has similar problems because of the
non-uniqueness of the file extensions, and one probably doesn't have access to
that easily. (Btw, OS/2 BMPs are supported as well, but only the old
uncompressed version.)

And don't worry about the MNG lines. It's the job of the MNG patch to add
something like that. I will alert glennrp and opi once this got in.

Finally, perhaps another improvement that makes more sense: you might want to
use extension.LowerCaseEqualsLiteral("gif") instead of !stricmp(extension.get(),
"gif"). The XPCOM code in NVU is too old for that but it seems to be more
Mozilla-like and seems to work for nsCAutoString.
(Assignee)

Comment 9

12 years ago
Comment on attachment 196431 [details] [diff] [review]
revises native d&d image handling

r=mkaply
Attachment #196431 - Flags: review?(mozilla) → review+

Comment 10

12 years ago
Created attachment 201437 [details] [diff] [review]
Patch with "nicer" string handling

Same patch as Rich's, just with
   extension.LowerCaseEqualsLiteral("gif")
etc. instead of
   !stricmp(extension.get(), "gif")
etc.
(Reporter)

Comment 11

12 years ago
(In reply to comment #10)
> Created an attachment (id=201437) [edit]
> Patch with "nicer" string handling

Sorry, I forgot about this one.  Thanks for fixing it.

Updated

12 years ago
Blocks: 314605
(Assignee)

Comment 12

12 years ago
Comment on attachment 201437 [details] [diff] [review]
Patch with "nicer" string handling

OS/2 only fix
Attachment #201437 - Flags: review+
Attachment #201437 - Flags: approval1.8rc2?

Comment 13

12 years ago
Comment on attachment 201437 [details] [diff] [review]
Patch with "nicer" string handling

OS/2 Only - approved to land
Attachment #201437 - Flags: approval1.8rc2? → approval1.8rc2+
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.