For embedding context menus, we need a "copy link" command that is separate from the normal "copy" command. Someone in gecko/ender needs to implement this. Really don't know who should own this.

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

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

assigning to anthonyd -- anthony see 630001

(after talking with Mike Pinkerton) what needs to be done here is, whichever Window Controller is implementing the other clipboard commands, needs to support cmd_copy_link, and when an image is selected cmd_copy should copy the image bits.

nominating 0.8

moved to moz0.9 -- adding nsbeta1 data

Beppe asked me to take this, and to target Moz 0.8.

I have a patch that implements this command. It's not hooked up to any UI, because UI and implementation for copy link location has existed for a long time and that context menu doesn't seem to use the controllers. (This also means that I haven't tested my new functionality.) To be honest, I don't really understand the reasoning behind this bug or how it's supposed to be used, so Mike or Jud, can you please review my patch and tell me whether I'm fixing the right bug, and whether this is supposed to be hooked up somewhere so it'll be testable?

Oh, while I'm in there, should I also add Copy Image Bits and Copy Image Location to the command list? As long as I'm modifying all these files, it would be easy to add these as well (with the Copy Image Bits implementation returning NS_ERROR_NOT_IMPLEMENTED for the moment until the backend for that is ready).

embedding apps need a way to force a "copy link location" to be executed (generally by context menu of their own writing). This implements it.

it would be nice if we could make navigator use this code, fwiw. we already use the controllers for cmd_copy and cmd_paste, etc.

Yes, that would be nice, though it won't actually gain us anything unless we do the image copy command as well (since they share code). Browser controllers are a bit out of my area, though: I tried modifying contentAreaContextOverlay.xul, nsContextMenu.js, and utilityOverlay.xul in communicator/resources/content, defining a new broadcaster cmd_copyLink (I changed the capitalization from what's in the patch, for consistency with the other commands in nsGlobalWindow) and changing oncommand="contextMenu.copyLink()" to observes="cmd_copyLink" as cut/copy/paste were doing, but it's not calling into nsGlobalWindow::DoCommand or IsCommandEnabled at all (it does call them for regular copy). Can some browser person help explain what file I'm missing, or where the control flow goes from contentAreaContextOverlay commands?

Did you try putting the broadcaster into utilityOverlay.xul and then obtaining it (<broadcaster id="cmd_copyLink"/>) from each app file that includes contentAreaContextOverlay.xul and requires it (navigatorOverlay.xul, mailWindowOverlay.xul, etc?)

Thanks, Blake! That was what I was missing. One thing left, though: when I added it to utilityOverlay.xul, I had to say disabled="false", otherwise the command was always greyed out. It never called nsGlobalWindow::IsCommandEnabled for that menu item. The other broadcasters in utilityOverlay.xul all have disabled="true". It's sort of academic, because the menu item isn't even shown if it's disabled (and showing/hiding it is handled in nsContextMenu.js), but I suspect I'm doing something wrong if IsCommandEnabled is never called.

Hmm...can you post the code you have now?

Patch on the way. Other files that have <broadcaster id="cmd_copy in them are: bm-find.xul, bookmarks.xul, history.xul -- do these need copy link as well?

I went ahead and added the two copy image commands, because to me that makes the most sense, and removed the code that had become redundant. I also changed PresShell::DoCopyLinkLocation to handle the situation where the focused node is inside a link (rather than being a link itself), though whatever JS code is handling the visibility of the menu items isn't showing the Copy Link Location menu item in this case, so for now it's academic. Both Copy Link

This seems to have stopped working, for reasons I can't figure out -- getting the focused node from the event state manager is failing, which breaks IsInLink and IsInImage and therefore everything else. I have no idea how the focused node is set. (This really should pass to someone familiar with the browser UI at this point ...)

On Beth's advice, handing over to xp toolkit folks, since the only remaining issue is a possible focus problem, and was never an editor issue anyway.

Might as well attach my quick fix to make DocumentViewerImpl::GetInLink check the parent chain, too. BTW, is there a better way to tell when you're at the root of the document than looking for html, body or null?

--> saari

this is targeted at 0.8 and has a patch attached. Are you all still trying to get this in for 0.8?

Index: mozilla/docshell/base/nsIContentViewerEdit.idl ========================================================= ========== RCS file: /m/pub/mozilla/docshell/base/nsIContentViewerEdit.idl,v retrieving revision 1.2 diff -w -u -2 -r1.2 nsIContentViewerEdit.idl --- nsIContentViewerEdit.idl 2000/07/18 23:11:26 1.2 +++ nsIContentViewerEdit.idl 2001/02/12 02:17:15 @@ -35,4 +35,11 @@ readonly attribute boolean copyable; + void copyLinkLocation(); + readonly attribute boolean inLink; + + void copyImageLocation(); + void copyImageContents(); + readonly attribute boolean inImage; + void cutSelection(); readonly attribute boolean cutable;

works on linux also, must have been a transitory problem

->moz0.9

->dr

patch is slightly bitrotted. hacking...

response from akkana to email: ------------------------------ > I'm about ready to check in 64313, which you worked on. I wanted to be > sure of a couple things before I check in, though: > > * This always worked on Navigator before... I assume there's code > somewhere in xpfe we can throw out now? Yes. My last patch threw out as much as I could of it. But I've forgotten now where the old code was. :-) You might be able to tell from looking at my patches, but things might have moved; the easiest way might be just to see what those menu items call up in the current source tree. > * I don't see any UI for "copy image contents" -- do I need to make > some? I'm unclear on whether the backend is actually implemented yet. I don't think it is, so it probably shouldn't be exposed yet (but if it is implemented, yes, it would be great to expose it). Pinkerton or Pavlov would probably know about the backend. Looking at the current patch, PresShell::DoCopyImageContents() returns NS_ERROR_NOT_IMPLEMENTED, so if the clipboard backend is implemented now, the pres shell method would also have to be written. > * In general, does the patch (from 4/10) look ok? Saari gave this to > me just to clean up and check in, so I just want to make sure > everything is copacetic... In DocumentViewerImpl::GetInLink(), in the if (!node) and if (!tag || tag == nsHTMLAtoms::html || tag == nsHTMLAtoms::body) cases, it returns NS_OK without ever setting *aIsInLink. Looks like it should be PR_FALSE in those cases? Maybe the *aIsInLink = PR_FALSE inside the earlier !node clause should be moved outside the clause to catch all three cases? (Might be my error from the original patch, and I'm just noticing it now.) The rest of the patch looks fine. Does it need any strings in a locale dtd file, or are the strings already there since the two implemented functions were already in the menus?

Working on more patch-love, per Akkana, to: - throw out old code, - fix frontend to use new code (in communicator and mailnews), - fix up DocumentViewerImpl::GetInLink Akkana: The strings you mention are already in a locale dtd file, afaict, since we already have those menuitems. The one thing we'll need to do after this goes in, in the frontend, is fix up the ids for the copy image location menuitem to accomodate copy image contents. I'll file a bug on that (I already have bug 21747 on implementing copy image contents on unix).

widget is ready to copy images. anthonyd has the bug (last i recall) to actually hook it up in the presShell.

This patch (4/13) is mostly there... Still some work to do: - make sure mailnews & editor don't regress (haven't tried yet) - make sure my PresShell::DoCopyHref isn't on crack for unix - fix spacing in nsDOMWindowController::SupportsCommand - clean the bejeezus out of utilityOverlay.xul Also need to get it working on Mac and Win32 obviously... Any preliminary takers for a review or super-review?

This patch is pretty much functionally complete and clean. Haven't tested on windows or mac, so there might be some minor cleanup yet to do. There's only one problem: I'm pretty certain I've exposed a bug in EventStateManager, where it doesn't know it has focused content when you click on an html <img>. It seems absurd that this wouldn't have been uncovered before, so I'm kind of doubting myself here, but tracing through the GetInImage code, I find EventStateManager::GetFocusedContent is failing on one particular case... Seems pretty definite. Looking for help testing and reviews now, please. Thanks!

Errr... So there was no EventStateManager bug. The reason it wouldn't give me a focused image is because images don't get focus. Am I learning yet? None of these patches, therefore, ever should have worked. Weirdness. So I've been whacking just about everything in the patch. Should have something real soon now.

This stuff is really dense, maybe use vertical whitespace to separate. Also, NS_ENSURE_SUCCESS(rv, rv) may be a bit paranoid. But, it's certainly not wrong to do... + // get the document + nsCOMPtr<nsIDocument> document; + rv = GetDocument(*getter_AddRefs(document)); + NS_ENSURE_SUCCESS(rv, rv); + NS_ENSURE_TRUE(document, NS_ERROR_FAILURE); + // get the script global object + nsCOMPtr<nsIScriptGlobalObject> global; + rv = document->GetScriptGlobalObject(getter_AddRefs(global)); + NS_ENSURE_SUCCESS(rv, rv); + NS_ENSURE_TRUE(global, NS_ERROR_FAILURE); Maybe use |while (node)| here: + // to look for xlinks also, but this is good enough for now. + while (PR_TRUE) { + + // if we have no node, fail + NS_ENSURE_TRUE(node, NS_ERROR_FAILURE); else-after-return. Waah! + NS_ENSURE_SUCCESS(rv, rv); + return CopyStringToClipboard(srcText); + } + else { sr=waterson

r=hyatt

Fixed 17 Apr. (moz0.9).