noticed that with 2002.04.03.09-static mozilla bits on linux rh7.2, there's no Copy Image context menu item. per Marlon's spec, the context menus for an inline image, image as link, and image as url should contain this item. however, i don't know if there's even a backend for copying image. if not, i'd imagine there's not enough time to implement the backend, hence this could prolly be futured.

...but i'm nominating for nsbeta1 if there *is* a backend. :)

Right. I don't think the backend has been implemented.

Nav triage team: nsbeta1-

See bug 21747, "Implement backend for cmd_copyImageContents"

There are already 6 context menu items for an image, and I don't see how this one would be useful. Adding this would also make it harder to find "Copy Image Location" because its name is similar.

*** Bug 138652 has been marked as a duplicate of this bug. ***

If this is bugging anybody else, I just found out I can drag-n-drop an image onto another document (on Mac OS X, at least). Not as convenient as copy-paste, but better than having to save it somewhere. Cheers.

This bug seems to be still open although a compremise for composer is great, but the real thread I thought was just a simple image copy, right mouse click.

Instead of adding Copy Image to the context menu, I think we should make the following work: 1. Drag an image or image link directly into an image editor. 2. Start dragging an image or image link, drop immediately, Ctrl+C, paste into image editor. 3. Drag an image (but not an image link) to the desktop.

The Context Menu is a pretty much standard for objects, copy and paste. It will be sadly missed. Dragging images around ang getting rogue copies of temp items is very poor UI. I Vote to keep it under Context menu's COPY. :-)

I vote for "Copy Image to Clipboard" or simply "Copy Image" from the context menu as well. Please do not confuse Joe Users with drag-n-drops and other foo. You could add those too if you want, but always have a direct "copy image" on the context menu as well.

I don't think we can have both Copy Image Location and Copy Image in the context menu without making both commands hard to find. I'm ok with removing Copy Image Location and replacing it with Copy Image.

The "Copy Image Location" should just be a *selectable* text on the Properties menu when you right click to an image. It should not be part of the main context menu. IE has it right there, UI-wise.

I agree with symonty, it would be bad practice to depend on drag and drop as the only method to carry images around. drag and drop isn't immediately evident to some types of users, especially on windows. as for eliminating [Copy Image Location] - i don't see why this feature justifies the removal of another which is completely different. the spec shows [Copy Image], the more frequently used item, is above [Copy Image Location] giving it priority. If we think [Copy Image Location] is a waste of space, lets discuss it in another bug. Keep in mind it is maintaining 4.x parity. It runs parallel with other menus items such as [Copy Link Location], [Copy Email Address] etc.. Wouldn't the removal of one justify the removal of these others, since they offer the same level of utility? The contextual menus were predicated on providing means to manipulate (copy) underlying data not immediately visible, nor obtainable without two or three additional steps.

If Copy Image adds two flavours to the clipboard -- the image itself, and the URI -- then the one menu item can perform both actions in a context sensitive manner.

that is definitely an option, but what about the case when an application could receive both flavours? do we give image priority over URI, wherever both are accepted? i think it's acceptable, and am willing to bet that the image is going to be the desired result in the majority of cases where applications could receive both data types. therefore, for the minority, ambiguous case (when URI is desired), users are forced to live with the tradeoff of having quick and concise context menus, instead of total unfettered control. and the former is our ultimate concern.

Yeah, for the cases where they accept both, the user just has to use Windows' "Paste Special" command (and whatever the equivalent is on other operating systems). Or they can just View Image and copy the URI from the Location bar...

nominating. any time for this for buffy?

I'm ok with replacing Copy Image Location with Copy Image. What should we do for Linux, where Mozilla doesn't yet have the backend for copying an image (bug 21747)?

nsbeta1+/adt3 per the nav triage team.

The "Copy Image" feature is one I use very frequently in Internet Explorer and one I sorely miss in Mozilla. If it comes down to a choice between "Copy Image Location" and "Copy Image", I would much prefer to have "Copy Image". This will give me one less reason to open up IE. :)

I totally agree with that, I open IE or Opera just to have the copy image feature. It's a shame to not have such a handy feature.

*** Bug 184426 has been marked as a duplicate of this bug. ***

I strongly request this feature be put in. The ONLY reason I ever open up IE anymore is to be able to copy images directly to the clipboard! I really do not feel it will junk up the pop-up UI at all.

I would have to strongly agree with the above comments on this to put this feature in. This is still one of the only reasons I still use IE (and one of the reasons keeping my IE-using friends away from Moz/Phoenix/etc...) As an engineering student with a lot of need to do web-based research, this feature is a godsend to capturing images and placing them directly into wordprocessing apps. After all, lets not forget the main purpose of the web is still for information gathering. This is a much better solution than to "Save Image as..." and then opening it up again in Word,etc. Also, as for the comments that state that this would add more clutter to the context menu. I have a hard time justifying the use of the "Send Image..." selection. Any decent mail app will be able to copy and paste an image quite easily. No reason to have a selection that does the SAME thing but losing important (and basic UI) functionality. Thanks!

This feature is especially useful for grapic artists who use Photoshop or other package to modify, combine and other wise use images from the web. I have to keep boot IE every time I want to grab images from a website, and paste them into my graphics app.

*** Bug 186415 has been marked as a duplicate of this bug. ***

This patch allows chimera to implemenet Copy Image in a context menu. Hope it will be useful here. Note that it copies a chunk of code from Content (nsContentAreaDragDrop.cpp) to layout (nsCopySupport). The copy/drag and drop code really needs major refactoring to avoid duplicate code.

Over to Shuehan.

*** Bug 187961 has been marked as a duplicate of this bug. ***

Bug 193053 has patches to add Image copy, and drag and drop of images.

*** Bug 193392 has been marked as a duplicate of this bug. ***

See also bug 210043, same bug for Firebird.

Is this scheduled to be fixed for Moz 1.4 final? This is a hugely important bug in my book, as I need to be able to copy images to Photoshop.

As I said in another bug (which was a duplicate of this), this bug is the only reason why I still use Internet Explorer. I need to be able to copy images to work on my websites. They have known about this bug for some time and seem to refuse to even consider fixing it. I doubt it would take them long to add this feature to a new version of Mozilla.

To the comments that copy image to clipboard should replace copy link location: the link location is quite useful when you're on a bulletin board and need to paste in the location so that others can see your link. It's also useful if you want to inline the image into a bulletin board post. I have a workaround for this problem that I use now though I'm not a heavy image user. ContextMenu extensions provides enough tools so that you can piece together a script to do the work for you. I have a context menu option to copy an image to the clipboard and another to drop the image into MS Paint. It's klunky but it works.

I said that "Copy Image" should replace "Copy Image Location", not "Copy Link Location". You would still be able to get an image's URL using "View Image" or "Properties". Also, pasting an image copied by "Copy Image" into something that only accepts text might paste the URL.

Yes, Copy Image should replace Copy Image Location, and should copy the URI in the text format, and the image in the bitmap format. (Clipboards can store more than one format.) Note, by the way, for those of you who must have this feature to use Mozilla, that you _can_ just _drag_ the image you want to your paint program.

To copy an image into Photoshop 7, you have to drag the image to Photoshop's window in the taskbar, then wait for Photoshop to pop up, then find the image you want to paste it into (you can't paste it as a new image) and drop it. This is a terrible workaround. I would like to see "Copy Image" work exactly as it does in IE6. I don't even think it's necessary to copy the image location as text as well, as that may have potential to screw up other apps that are looking for an image only. "Copy Image" should do exactly as stated -- copy the image to the clipboard. You can always "View Image" and copy the URL that appears in the URL bar if you want the image location, or right-click -> Properties -> copy and paste the location that appears. People are used to using both of these behaviors from IE. Please implement this feature, and implement it so it just copies the image. It doesn't need to be any fancier than that. (I don't want to have to keep reporting bugs if it ends up that Photoshop or some other program won't accept an image on the clipboard if there is text there as well.)

Multiple clipboard formats are quite well supported, that's not a problem.

Comment on attachment 110378 [details] [diff] [review] Patch used in chimera to hook up Copy Image This patch is on the trunk now.

Attachment 114287 [details] [diff] in bug 193053 is a patch to add Copy Image to the image context menu. As well as that, I believe some work needs to be done in Windows widget code to put image data on the native clipboard.

erica: speak for yourself about being used to behaviors from IE. I personally am not used to any behaviors from IE and don't want it to become harder for me to get an image URL just because IE makes it hard for a user to get an image URL.

Maybe it's because I'm not a Mozilla programmer, but I really don't understand the problem. I can't imagine it can be that difficult to implement *both* a "Copy image" and a "Copy image location" function, in two separate menu items. I don't see why these two should exclude or conflict with each other. It's really not about IE compatibility or something, it's just a feature that some of us are desperately missing.

"I need to be able to copy images to work on my websites." I'm ©urious: What kind of fair use do you make of such images? "you _can_ just _drag_ the image you want to your paint program." Not to GIMP for Windows. I can drag files from Windows Explorer to GIMP, but when I try to drag an image from Mozilla to GIMP, all I get is the "no" cursor. Want a test case? "I can't imagine it can be that difficult to implement *both* a 'Copy image' and a 'Copy image location' function" The UI designers try to eliminate unnecessary items from context menus.

Excuse-me, I sent the message below for Bug 21747, I wanted to send it for Bug 135300. Sorry for that... So I paste my message here : Yes I think that "Copy Image" is a very useful feature that lacks in Mozilla 1.4 . I think it would be great if there were "Copy Image" and "Copy Image Location" (in order to have a short context menu, why not doing a submenu called "Menu" and when you move the mouse over it, it appears two items at the right of Menu with the two items mentioned above ?), even if I don't use Copy Image Location. In Opera (7.11), there are the two options in the context menu. A context menu with a lot of items would not somehow be very annoying.

> (Clipboards can store more than one format.) Is this a safe assumption on all platforms? It would be bad to break platform parity over a trivial thing like combining two menu items.

I just want to add that in my last message concerning this bug (#46), I wanted to say that the menu could be called "Copy" (and not "Menu"). In it, there could be "Copy image location", "Copy Image" and, if the image is a link "Copy link location" (it would thus be a menu like File > New in Mozilla 1.4).

updated version of attachment #114287 [details] [diff] [review] for adding context menu item, also move Copy Image to just above Copy Image Location. I think this is all that is needed, for windows as well as mac.

i've been annoyed at the lack of a "right click -> copy image" since i first starting using this browser. i vote strongly in favor of adding it as the original bug states and not using any the half-backward workarounds suggested (for example, by jesse) that do not fix the problem.

I don't like it when they remove "Copy Image Location". Why not add a new menu item? What if people need to copy the location of that image? Install yet another extension? mozilla is more and more becomming 'puzzleware'. Great for new users. Glue your own extensions to make a perfectly working/functional MSIE clone...

I would like to strongly support this bug. The user interface *is* the program. If users can't see it, they don't know about it. I'm a programmer, and I thought it was odd, but I assumed it was just something that could have been difficult on one platform and so had been conveniently ignored. Windows users in particular are used to being able to right-click on things to copy them, in just about every program that offers some form of clipboard facility. If they don't see it there, they'll just assume that Mozilla/Firebird doesn't support it. They won't go out and get it as an extension - none of my artist friends would even think of that, let alone be interested in the hassle. They'll use IE, a substandard browser, for the sake of a context menu item.

(In reply to comment #52) > Windows users in particular are used to being able to right-click on things to > copy them, in just about every program that offers some form of clipboard > facility. If they don't see it there, they'll just assume that Mozilla/Firebird > doesn't support it. They won't go out and get it as an extension - none of my > artist friends would even think of that, let alone be interested in the hassle. > They'll use IE, a substandard browser, for the sake of a context menu item. Wow you just described me perfectly! So our group writes the code for all of our web based apps and I end up doing all the user documentaion. For most of that user documentation. I basically take screen shots with alt-prtscn and paste and crop into word or powerpoint. If i only want a single pic from a page right clicking and pasting is such a no brainer that I can't live without it. Just thought you might like to know why i still cant use Mozilla for day today use.

It´s necessry to simplify the right-click menu and it will be more intuitive. I suggest: > Copy > Copy Location Copy: simple copy (cut-paste) for any object: Image, Text, ... Copy Location: active for objects with Location. As the option "Show Image" I don´t understand its mission: a simple view in a new window!! It´s very obvious, the image it´s just been showed.

*** Bug 242214 has been marked as a duplicate of this bug. ***

In regards to Comment #54, I agree that "Copy" would be more intuitive. On a longer context menu, having the shorter item "copy" instead of "copy image" would allow the brain to distinguish the item more easily. Any possibility of getting it shortened to just "copy"?

there are correct fixes for this ui, but they don't include as many menu items as we have. (they still require supporting the feature) (which isn't implemented... [at least for some platforms]) <ben> there's no way two copy menu items are needed for images. both data flavors should be added to the transferable in a single "Copy" item. <ben> plain text applications can get the text version, rich text apps can get +the image version, or something else through "Edit->Paste Special..." (see MS Word and other well behaved applications) it's been around forever; it's the right thing to do <bz> it'd simplify the "link location / image location" **** I always get with +linked images... -- This is a summary of sorts of an irc conversation. Ben and I agree completely here. ben was nice enough to do the writing. neil: would you please sign off on changing the spec?

Only btw: This extension works for me with Seamonkey (Windows). http://extensionroom.mozdev.org/more-info/copyimage

timeless, looks like Ian beat ben to it by almost two years (comment 15 :-) Summary for the volunteer: * Fix nsCopySupport.cpp to copy the location and image together * Change the menuitem to copy image (Separate patches accepted, in order!)

*** Bug 242739 has been marked as a duplicate of this bug. ***

Is there any work being done on porting FireFox's funcitonality from bug 210043 to Seamonkey?

No, because it doesn't implement comment 57.

Isn't this bug fixed now? I've got a Copy Image item on my context menu in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041117 MultiZilla/1.7.0.0h and have had for some time now.

In order. comment #59 > * Fix nsCopySupport.cpp to copy > the location and image together

Comment on attachment 169824 [details] [diff] [review] non-UI patch v1 First off, using the -p context to diff and more context, say "diff -up8", makes diffs easier to read. >Index: mozilla/content/base/src/nsCopySupport.cpp >+ //Add the unicode data flavor This code largely duplicates the code in DocumentViewerImpl::CopyImageLocation (which seems to be the current implementation of the "copy image location" method). I'd prefer the two methods share code (both call the same static method in nsCopySupport or nsContentUtils or something). More seriously, the code here puts the image only in aClipboardID (which you're passing as nsIClipboard::kGlobalClipboard, whereas the existing "copy image location" code puts the URL in both nsIClipboard::kGlobalClipboard and nsIClipboard::kSelectionClipboard. I don't think we really want to break that....

Thanks the review. Addressed those 2 issues... Currently, copyImageContents doen't support unix, *so* copyImageContents is using only the global clipboard, right? Or is there any problem to set raw image flavor onto the linux selection clipboard?

Comment on attachment 169902 [details] [diff] [review] non-UI patch v2 >Index: mozilla/content/base/public/nsCopySupport.h >+ static nsresult ImageCopy(nsIImageLoadingContent* aImageElement, >+ PRBool aContent); I'd call that last arg "aCopyImageData". >Index: mozilla/content/base/src/nsCopySupport.cpp >+ nsCOMPtr<nsITransferable> trans(do_CreateInstance(kCTransferableCID)); >+ NS_ENSURE_TRUE(trans, NS_ERROR_FAILURE); Why suppress the actual rv from do_CreateInstance here? >+ nsCOMPtr<nsISupportsString> >+ location(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID)); >+ NS_ENSURE_TRUE(location, NS_ERROR_FAILURE); Same here. >+ if(aContentsFlag){ Spaces after "if" and before '{', please. >+ // get the image data form the element s/form/from/ >+ nsCOMPtr<nsISupportsInterfacePointer> >+ imgPtr(do_CreateInstance(NS_SUPPORTS_INTERFACE_POINTER_CONTRACTID)); >+ NS_ENSURE_TRUE(imgPtr, NS_ERROR_FAILURE); Again, why suppress return value? >+ // get clipboard >+ nsCOMPtr<nsIClipboard> clipboard(do_GetService(kCClipboardCID)); >+ NS_ENSURE_TRUE(clipboard, NS_ERROR_FAILURE); And here. I don't really follow comment 66; we're putting the transferable with the image contents on both global and selection clipboard; I suspect the image contents data flavor will just get ignored on the latter.... The non-support on Unix is not in this function but in the widget code that handles getting the image data out of the transferable if someone asks for it. So the patch looks correct in general except for the above nits.

The difference is based on comment #67 .

Comment on attachment 169960 [details] [diff] [review] non-UI patch v3 Actually, do_CreateInstance guarantees an error rv on null return. So no need for those NS_ENSURE_TRUE checks.... r=bzbarsky with that. jst, could you sr?

Removed null checks as comment #69 .

This patch corresponds to > * Change the menuitem to copy image at the comment #59 In the long run, maybe it's time to delete <command id="cmd_copyImageLocation"/> elements in various files, http://lxr.mozilla.org/mozilla/search?string=id%3D%22cmd_copyImageLocation%22 because nobody !(seamonkey, fx, tb, camino?) use this command. On the other hand, goDoCommand("cmd_copyImageLocation") still just works, and they doesn't hurt anybody, other than the "Avoid Possible Bloat(tm)" strategy. IMHO, they should be removed especially from browser and mail components, but I'd like to wait-and-see here.

Comment on attachment 169964 [details] [diff] [review] non-UI patch v4 r=bzbarsky

Re comment #71: > In the long run, maybe it's time to delete > <command id="cmd_copyImageLocation"/> elements in various files, [...] > because nobody !(seamonkey, fx, tb, camino?) use this command. > > On the other hand, goDoCommand("cmd_copyImageLocation") > still just works, and they doesn't hurt anybody, Yes, and it's still useful and even "revived" by certain addons. > other than the "Avoid Possible Bloat(tm)" strategy. I don't think that the impact is high enough to justify a removal.

cmd_CopyImageContents? :-/

(In reply to comment #74) > :-/ You mean we should use "cmd_copyImage" instead?

I pondered for a while, and I'd like to suggest some interface changes. nsIContentViewerEdit: 1. Add "void copyImage();" to this interface. 2. copyImageLocation() ,copyImageContents() and copyImage() have very the same functionality. Copy both location and image data to the clipboard. 3. Remove copyImageLocation() and copyImageContents() in the future. Maybe Mozilla 1.8 or 2.0 or 3.0 4. Get ready to remove them. webshell: 1. Support the command "cmd_copyImage" as well as "cmd_copyImageLocation" and "cmd_copyImageContents" 2. These three commands works identically. Copy both location and image data to the clipboard. 3. cmd_copyImageLocation and cmd_copyImageContents are deprecated. To be removed as well. Boris, would you review this idea?

This is the basics of how to remove cmd_copyImageLocation.

Comment on attachment 170021 [details] [diff] [review] non-UI patch v5 Sorry for bug spam. nsIClipboardCommands is @status FROZEN

Comment on attachment 169972 [details] [diff] [review] FE patch v1 (Navigator and Mail/News) r=me once the copied image pastes into Notepad, Paint, NVu etc.

Comment on attachment 169964 [details] [diff] [review] non-UI patch v4 sr=jst

Comment on attachment 169964 [details] [diff] [review] non-UI patch v4 I just checked in this patch for 1.8b

(In reply to comment #81) > (From update of attachment 169964 [details] [diff] [review] [edit]) > I just checked in this patch for 1.8b bz, thank you. Then I should check in attachment 169964 [details] [diff] [review] for 1.8b, as well. That does not mean that this bug is going to be marked "resolved" for 1.8b, because Neil and other guys' (ben, bz, and timeless in comment #57) request is cmd_copyImage[Contents] should copy also the HTML fragment. I think there shold be a sticky way to create the transferable per element/selection. I'm planning to move the class nsTransferableFactory from nsContentAreaDragDrop.cpp to nsCopySupport.h/.cpp

(In reply to comment #82) > Then I should check in attachment 169964 [details] [diff] [review] [edit] Oops, I mean comment 169972 the front end patch.

That patch doesn't have sr yet, does it?

Comment on attachment 169972 [details] [diff] [review] FE patch v1 (Navigator and Mail/News) You're working on copy image from navigator, paste into editor I hope?

Comment on attachment 169972 [details] [diff] [review] FE patch v1 (Navigator and Mail/News) Checked in for 1.8b

(In reply to comment #85) > You're working on copy image from navigator, >paste into editor I hope? This patch is to make it possible to paste to composer, though this is not so ambitious as I said before. > I'm planning to move the class > nsTransferableFactory > from nsContentAreaDragDrop.cpp > to nsCopySupport.h/.cpp bz, thank you for checking in, again.

Comment on attachment 171394 [details] [diff] [review] non-UI patch v6 >Index: mozilla/content/base/public/nsCopySupport.h >+ private: >+ // copy string data onto the transferable >+ static nsresult AppendString(nsITransferable *aTransferable, >+ const nsAString& aString, >+ const char* aFlavor); >+ >+ // copy HTML node data >+ static nsresult AppendDOMNode(nsITransferable *aTransferable, >+ nsIDOMNode *aDOMNode); Why not just make these non-class statics in nsCopySupport.cpp? >Index: mozilla/layout/base/nsDocumentViewer.cpp > NS_IMETHODIMP DocumentViewerImpl::CopyImageLocation() Why the changes here? I'd prefer that we catch non-image nodes here, and QI to nsIDOMNode in nsCopySupport methods if needed...

(In reply to comment #88) >>Index: mozilla/layout/base/nsDocumentViewer.cpp >>NS_IMETHODIMP DocumentViewerImpl::CopyImageLocation() >Why the changes here? I'd prefer that we catch non-image nodes here, and QI to >nsIDOMNode in nsCopySupport methods if needed... Because GetPopupImageNode works by getting a DOM Node and calling QI to nsImageLoadingContent; now that CopyImageLocation needs a DOM Node there's little point calling GetPopupImageNode, because you'll only have to QI back and forth. Note that this leaves IsInImage as the last caller of GetPopupImageNode.

(In reply to comment #88) > Why the changes here? I'd prefer that we catch non-image > nodes here, and QI to nsIDOMNode in nsCopySupport > methods if needed... As already stated in comment #89, it simply reduces the total number of QIs.

along with comment #88

Sorry, I know this is off-topic, but why do I keep getting posts on this bug by E-mail, even though I removed my address (bugzilla@sunlight.de) from the CC list? Can someone give me a hint?

(In reply to comment #92) > Sorry, I know this is off-topic, but why do I keep getting posts on this bug by > E-mail, even though I removed my address (bugzilla@sunlight.de) from the CC > list? Can someone give me a hint? you have voted for this bug so either remove your vote or change your email settings in preferences

Comment on attachment 171485 [details] [diff] [review] non-UI patch v7 It'll be a few days before I can get to this.

Patches here are breaking Copy Image in Camino. On Mac we don't necessarily want 'Copy Image' to put both the URL and the image data onto the clipboard, because some apps will take the text data (the URL) first, when the user really wants to paste the image data. I think the embedder/caller needs to be able to specify what 'copy image' really means.

Hmm.. That causes some issues with comments from Marlon and Ben (via timeless) earlier in this bug. I think they were assuming that apps would do the Right Thing with multiple clipboard flavors, and it sounds like they don't.... So before we patch more stuff, could we clearly spec out the api we want?

I think the underlying layers (nsCopySupport) needs to be told what to put in the clipboard: nsresult ImageCopy(nsIImageLoadingContent* aImageElement, PRBool aCopyImageData, PRBool aCopyImageLocation); Maybe we can then use nsICommandParams to feed down flags from the UI layer about what the UI wants copied.

Sure. That seems reasonable.

or maybe an PRInt32 aCopyFlags arg? with COPY_IMAGE and COPY_TEXT constants, that can be ORd together.

Is it a good idea to even copy text data along with the image data? One thing about trying to paste an image inside a program that doesn't support images, is that it will not allow it -- thus indicating to the user that the program does not support pasting the data they requested. Pasting text instead of the requested image might not be desirable.

Kevin, please read earlier discussion in this bug. Apparently at least some Gecko-based applications (Firefox, eg) believe that this is desirable, yes.

(In reply to comment #99) >COPY_IMAGE and COPY_TEXT Not forgetting COPY_HTML!

winword.exe also prefers text to bitmap, although I would have thought it would prefer HTML to text given the choice.

(In reply to comment #95) > some apps will take the text data (the URL) > first, when the user really wants to paste the > image data. Then what happens on Drag and Drop? > I think the embedder/caller needs to be able > to specify what 'copy image' really means. Hmm, I guess this is not a embedding issue, because all the Mac products would be affected. Though I agree to making the api more flexible as suggested in comment #97, comment #99 and comment #102. IMHO, the difficulty lies in FE rather than api. The front end is going to be painfully complicated, as it's no more "XP"FE ... Neil: Is it ok to put #ifdef XP_MAC on a xul file a la firefox? Or I should use platformCommunicatorOverlay.xul instead? I sometimes think we should give up enhancing copyImageContents and just show "Copy" menuitem even if nothing is selected (In this case, copy the popupNode instead of the selection).

By the way, is there any way we can affect which flavor on the clipboard is the "primary" one? Or is it completely up to the app being pasted into to decide what it wants?

Applications usually look in the clipboard for flavors in a fixed order, normally richest (rich text) to simplest (plain text). The problem with images and text is that they are such different flavors; they are not variations of the same flavor. Inserting an image is a very different user action than inserting text, and most users will make a conscious choice to get one or the other. Hence the need to keep control over which we put in the clipboard.

what simon said. the application is in control of the order in which it searches. it's out of our hands entirely.

(In reply to comment #107) > what simon said. the application is in control of the order in which it > searches. it's out of our hands entirely. Can we have an option to make this feature only copy image data (and no text)?

Kevin, please please please read the bug like I asked you to. Please. It'll really help, or so I hope.

* flags (comment #99) * leaving out Mac > +#if defined(XP_MAC) || defined(XP_MACOSX)

(In reply to comment #106) > Hence the need to keep control over which we put > in the clipboard. To "keep control", we need one more "copy" menuitem to implement the HTML thingy. ------------------- View Image Block Image from... Copy Image Location #URL only Copy Image Contents #Image data only ------------------- Copy #Sink: URL, image and HTML ------------------- ?

Comment on attachment 172421 [details] [diff] [review] non-UI patch v8 - shortcut.Append(PRUnichar('\n')); + shortcut.AppendLiteral("\n"); why this change? Also, do we really want this to be different on different platforms?

Comment on attachment 172421 [details] [diff] [review] non-UI patch v8 > NS_IMETHODIMP DocumentViewerImpl::CopyImageContents() > { > NS_ENSURE_TRUE(mPresShell, NS_ERROR_NOT_INITIALIZED); > nsCOMPtr<nsIImageLoadingContent> node; > GetPopupImageNode(getter_AddRefs(node)); > // make noise if we're not in an image > NS_ENSURE_TRUE(node, NS_ERROR_FAILURE); > >- return nsCopySupport::ImageCopy(node, PR_TRUE); >+#if defined(XP_MAC) || defined(XP_MACOSX) >+ return nsCopySupport::ImageCopy(node, COPY_SUPPORT_IMAGE); >+#else >+ return nsCopySupport::ImageCopy(node, COPY_SUPPORT_TEXT | >+ COPY_SUPPORT_HTML | >+ COPY_SUPPORT_IMAGE); >+#endif > } I don't think this is the right thing to do. I think you need to feed the flags down from the UI layer via nsICommandParams. A Mac embedder *might* want all the flavors; let the embedder decide, not gecko.

Comment on attachment 172421 [details] [diff] [review] non-UI patch v8 Yes, what Simon said. We need to expose an api that will let the embeddor choose the sort of copy that happens. This code should have no ifdefs. What it should have is a document viewer api that takes flags, probably, and this change should be propagated all the way to whatever the embedding interface is that is calling into this code.

Canm we get get some traction on this? It's blocking Camino 0.9.

If we can't get traction on the more flexible back end for 1.8b, I think we should back the earlier patches out.... In any case, one or the other should definitely block 1.8b2.

We're not getting any traction on this. Can we back this out? Camino is still broken.

*** Bug 285317 has been marked as a duplicate of this bug. ***

bz, is this what you mean?

Comment on attachment 176909 [details] [diff] [review] Move the flags to nsIContentViewerEdit >Index: docshell/base/nsIContentViewerEdit.idl >=================================================================== >RCS file: /cvsroot/mozilla/docshell/base/nsIContentViewerEdit.idl,v >retrieving revision 1.5 >diff -p -u -d -r1.5 nsIContentViewerEdit.idl >--- docshell/base/nsIContentViewerEdit.idl 17 Apr 2004 21:49:36 -0000 1.5 >+++ docshell/base/nsIContentViewerEdit.idl 9 Mar 2005 17:54:45 -0000 >@@ -40,7 +40,7 @@ > > #include "nsISupports.idl" > >-[scriptable, uuid(42d5215c-9bc7-11d3-bccc-0060b0fc76bd)] >+[scriptable, uuid(1691a02f-53b2-4cb8-8769-48e7efc908b8)] > interface nsIContentViewerEdit : nsISupports > { > void search(); >@@ -55,8 +55,11 @@ interface nsIContentViewerEdit : nsISupp > void copyLinkLocation(); > readonly attribute boolean inLink; > >- void copyImageLocation(); >- void copyImageContents(); >+ const long COPY_TEXT = 0x0001; >+ const long COPY_HTML = 0x0002; >+ const long COPY_DATA = 0x0004; >+ const long COPY_ALL = -1; I'd prefer these flags be called COPY_IMAGE_FOO, unless there's a plan to use them elsewhere. >Index: dom/src/base/nsGlobalWindowCommands.cpp >=================================================================== >RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindowCommands.cpp,v >retrieving revision 1.15 >diff -p -u -d -r1.15 nsGlobalWindowCommands.cpp >--- dom/src/base/nsGlobalWindowCommands.cpp 18 Feb 2005 07:29:40 -0000 1.15 >+++ dom/src/base/nsGlobalWindowCommands.cpp 9 Mar 2005 17:54:46 -0000 >@@ -603,9 +603,9 @@ nsresult > nsClipboardImageCommands::DoClipboardCommand(const char *aCommandName, nsIContentViewerEdit* aEdit, nsICommandParams* aParams) > { > if (!nsCRT::strcmp(sCopyImageLocationString, aCommandName)) >- return aEdit->CopyImageLocation(); >+ return aEdit->CopyImage(nsIContentViewerEdit::COPY_TEXT); > >- return aEdit->CopyImageContents(); >+ return aEdit->CopyImage(nsIContentViewerEdit::COPY_ALL); This will still break Mac. CopyImage() needs to use COPY_DATA, or you need to pass the flags down in aParams.

I'll try to look tomorrow, but why is this stuff on nsIContentViewerEdit anyway? Copying is not an editing-related thing, really (though pasting is).

nsIContentViewerEdit is an API created before nsICommands became the One True Way to do stuff like this (and they aren't used everywhere either).

Ah, ok. So this is an api we should be striving to deprecate and remove, then?

I think nsIContentViewFile and nsIContentViewEdit should go away, yes.

Comment on attachment 176945 [details] [diff] [review] Provide the possibility of doCommandParams >Index: content/base/src/nsCopySupport.cpp >+static nsresult AppendString(nsITransferable >+ return aTransferable->SetTransferData(aFlavor, data, >+ (aString.Length() * 2)); I know that's what all this code had, but how about s/2/sizeof(PRUnichar)/ ? With that, r=bzbarsky. Simon, can you sr?

Fix checked in.

So, have you filed a bug about either remvoing the Copy Image Location command or changing the Copy Image command params in Firefox's frontend? Firefox has both commands in its UI. The last includes the former....

(In reply to comment #129) > So, have you filed a bug about either remvoing the Copy Image Location command > or changing the Copy Image command params in Firefox's frontend? Firefox has > both commands in its UI. The last includes the former.... > If you are referring to just copying image data in Firefox, then that particular "bug" has been filed here: Bug 286118