135300 - context menus: add Copy Image per ui spec

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[sairuh (rarely reading bugmail)]

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.

Comment 1

[sairuh (rarely reading bugmail)]

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

Comment 2

[Blake Ross]

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

Comment 3

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 4

[Clement Cherlin]

See bug 21747, "Implement backend for cmd_copyImageContents"

Comment 5

[Jesse Ruderman]

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.

Comment 6

[Jesse Ruderman]

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

Comment 7

[Ken Harris]

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.

Comment 8

[symonty]

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.

Comment 9

[Jesse Ruderman]

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.

Comment 10

[symonty]

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.

:-)

Comment 11

[Eugenia Loli]

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.

Comment 12

[Jesse Ruderman]

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.

Comment 13

[Eugenia Loli]

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.

Comment 14

[marlon bishop]

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.   

Comment 15

[Hixie (not reading bugmail)]

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.

Comment 16

[marlon bishop]

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.

Comment 17

[Hixie (not reading bugmail)]

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...

Comment 18

[sairuh (rarely reading bugmail)]

nominating. any time for this for buffy?

Comment 19

[Jesse Ruderman]

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)?

Comment 20

[Paul Wyskoczka]

nsbeta1+/adt3 per the nav triage team.

Comment 21

[Albert Yarusso]

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.  :)

Comment 22

[Rick Bull]

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.

Comment 23

[Matthias Versen [:Matti]]

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

Comment 24

[Chris]

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.

Comment 25

[prabhath]

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!

Comment 26

[Brander Roullett]

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.

Comment 27

[R.K.Aa.]

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

Comment 28

[Simon Fraser [no longer active]]

Attachment: Patch used in chimera to hook up Copy Image

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.

Comment 29

[Samir Gehani]

Over to Shuehan.

Comment 30

[R.K.Aa.]

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

Comment 31

[Simon Fraser [no longer active]]

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

Comment 32

[Simon Fraser [no longer active]]

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

Comment 33

[Jesse Ruderman]

See also bug 210043, same bug for Firebird.

Comment 34

[erica]

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.

Comment 35

[seph]

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.

Comment 36

[Michael Moy]

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.

Comment 37

[Jesse Ruderman]

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.

Comment 38

[Hixie (not reading bugmail)]

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.

Comment 39

[erica]

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.)

Comment 40

[Hixie (not reading bugmail)]

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

Comment 41

[Simon Fraser [no longer active]]

Comment on attachment 110378 [details] [diff] [review]
Patch used in chimera to hook  up Copy Image

This patch is on the trunk now.

Comment 42

[Simon Fraser [no longer active]]

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.

Comment 43

[scratch]

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.

Comment 44

[Joerg Starkmuth]

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.

Comment 45

[Damian Yerrick]

"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.

Comment 46

[Michael Perrin]

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.

Comment 47

[Jonadab the Unsightly One (Nathan Eady)]

> (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.

Comment 48

[Michael Perrin]

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).

Comment 49

[shliang]

Attachment: updated version of context menu patch from bug 193053

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.

Comment 50

[flii (cj)]

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.

Comment 51

[HJ]

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...

Comment 52

[Laurence "GreenReaper" Parry]

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.

Comment 53

[Dmitri Bichko]

(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.

Comment 54

[J.L Gascon]

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.

Comment 55

[R.K.Aa.]

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

Comment 56

[Kevin Ar18]

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"?

Comment 57

[timeless]

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?

Comment 58

[OstGote!]

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

Comment 59

[[e-mail]]

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!)

Comment 60

[Boris Zbarsky [:bzbarsky]]

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

Comment 61

[Kelly Price]

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

Comment 62

[[e-mail]]

No, because it doesn't implement comment 57.

Comment 63

[Raj Bhaskar]

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.

Comment 64

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v1

In order.
comment #59

> * Fix nsCopySupport.cpp to copy 
> the location and image together

Comment 65

[Boris Zbarsky [:bzbarsky]]

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....

Comment 66

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v2

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 67

[Boris Zbarsky [:bzbarsky]]

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.

Comment 68

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v3

The difference is based on comment #67 .

Comment 69

[Boris Zbarsky [:bzbarsky]]

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?

Comment 70

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v4

Removed null checks as comment #69 .

Comment 71

[O. Atsushi (Torisugari)]

Attachment: FE patch v1 (Navigator and Mail/News)

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 72

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 73

[Karsten Düsterloh]

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.

Comment 74

[[e-mail]]

cmd_CopyImageContents? :-/

Comment 75

[O. Atsushi (Torisugari)]

(In reply to comment #74)
> :-/

You mean we should use "cmd_copyImage" instead?

Comment 76

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v5

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?

Comment 77

[[e-mail]]

Attachment: cmd_copyImageLocation is unnecessary

This is the basics of how to remove cmd_copyImageLocation.

Comment 78

[O. Atsushi (Torisugari)]

Comment on attachment 170021 [details] [diff] [review]
non-UI patch v5

Sorry for bug spam.
nsIClipboardCommands is @status FROZEN

Comment 79

[[e-mail]]

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 80

[Johnny Stenback (:jst)]

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

sr=jst

Comment 81

[Boris Zbarsky [:bzbarsky]]

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

I just checked in this patch for 1.8b

Comment 82

[O. Atsushi (Torisugari)]

(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

Comment 83

[O. Atsushi (Torisugari)]

(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.

Comment 84

[Boris Zbarsky [:bzbarsky]]

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

Comment 85

[[e-mail]]

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 86

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169972 [details] [diff] [review]
FE patch v1 (Navigator and Mail/News)

Checked in for 1.8b

Comment 87

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v6

(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 88

[Boris Zbarsky [:bzbarsky]]

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...

Comment 89

[[e-mail]]

(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.

Comment 90

[O. Atsushi (Torisugari)]

(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.

Comment 91

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v7

along with comment #88

Comment 92

[Joerg Starkmuth]

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 ([e-mail]) from the CC
list? Can someone give me a hint?

Comment 93

[Nicu Buculei]

(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 ([e-mail]) 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 94

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 171485 [details] [diff] [review]
non-UI patch v7

It'll be a few days before I can get to this.

Comment 95

[Simon Fraser [no longer active]]

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.

Comment 96

[Boris Zbarsky [:bzbarsky]]

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?

Comment 97

[Simon Fraser [no longer active]]

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.

Comment 98

[Boris Zbarsky [:bzbarsky]]

Sure.  That seems reasonable.

Comment 99

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 100

[Kevin Ar18]

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.

Comment 101

[Boris Zbarsky [:bzbarsky]]

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

Comment 102

[[e-mail]]

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

Comment 103

[[e-mail]]

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

Comment 104

[O. Atsushi (Torisugari)]

(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). 

Comment 105

[Boris Zbarsky [:bzbarsky]]

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?

Comment 106

[Simon Fraser [no longer active]]

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.

Comment 107

[Mike Pinkerton (not reading bugmail)]

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

Comment 108

[Kevin Ar18]

(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)?

Comment 109

[Boris Zbarsky [:bzbarsky]]

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

Comment 110

[O. Atsushi (Torisugari)]

Attachment: non-UI patch v8

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

Comment 111

[O. Atsushi (Torisugari)]

(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 112

[Christian :Biesinger (don't email me, ping me on IRC)]

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 113

[Simon Fraser [no longer active]]

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 114

[Boris Zbarsky [:bzbarsky]]

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.

Comment 115

[Simon Fraser [no longer active]]

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

Comment 116

[Boris Zbarsky [:bzbarsky]]

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.

Comment 117

[Josh Aas]

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

Comment 118

[Boris Zbarsky [:bzbarsky]]

Attachment: Backout of those patches in this bug that have reviews (and hence presumably landed)

Comment 119

[OstGote!]

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

Comment 120

[[e-mail]]

Attachment: Move the flags to nsIContentViewerEdit

bz, is this what you mean?

Comment 121

[Simon Fraser [no longer active]]

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.

Comment 122

[[e-mail]]

Attachment: Provide the possibility of doCommandParams

Comment 123

[Boris Zbarsky [:bzbarsky]]

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).

Comment 124

[Simon Fraser [no longer active]]

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

Comment 125

[Boris Zbarsky [:bzbarsky]]

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

Comment 126

[Simon Fraser [no longer active]]

I think nsIContentViewFile and nsIContentViewEdit should go away, yes. 

Comment 127

[Boris Zbarsky [:bzbarsky]]

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?

Comment 128

[[e-mail]]

Fix checked in.

Comment 129

[Asaf Romano (gone)]

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....

Comment 130

[Kevin Ar18]

(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