Implement cmd_copyLink, cmd_copyImageLocation, cmd_copyImageContents

RESOLVED FIXED in mozilla0.9

Status

()

Core
XUL
P1
major
RESOLVED FIXED
18 years ago
10 years ago

People

(Reporter: Mike Pinkerton (not reading bugmail), Assigned: Dan Rosen)

Tracking

({embed})

Trunk
mozilla0.9
embed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta1+])

Attachments

(13 attachments)

11.82 KB, patch
Details | Diff | Splinter Review
4.02 KB, patch
Details | Diff | Splinter Review
24.28 KB, patch
Details | Diff | Splinter Review
3.55 KB, patch
Details | Diff | Splinter Review
23.39 KB, patch
Details | Diff | Splinter Review
642 bytes, patch
Details | Diff | Splinter Review
642 bytes, patch
Details | Diff | Splinter Review
17.48 KB, patch
Details | Diff | Splinter Review
20.61 KB, patch
Details | Diff | Splinter Review
45.27 KB, patch
Details | Diff | Splinter Review
60.98 KB, patch
Details | Diff | Splinter Review
72.75 KB, patch
Details | Diff | Splinter Review
72.11 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Keywords: embed
(Reporter)

Comment 1

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

Comment 2

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

Comment 3

18 years ago
assigning to anthonyd -- anthony see 630001
Assignee: beppe → anthonyd

Comment 4

18 years ago
(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.
Severity: normal → major
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9

Comment 5

18 years ago
nominating 0.8
Keywords: mozilla0.8

Updated

18 years ago
Priority: -- → P2

Comment 6

18 years ago
moved to moz0.9 -- adding nsbeta1 data
Keywords: mozilla0.8 → nsbeta1
Whiteboard: [nsbeta1+]

Updated

18 years ago
Blocks: 64833

Comment 7

18 years ago
Beppe asked me to take this, and to target Moz 0.8.
Assignee: anthonyd → akkana
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9 → mozilla0.8

Comment 8

18 years ago
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?
Status: NEW → ASSIGNED

Comment 9

18 years ago
Created attachment 23501 [details] [diff] [review]
Preliminary patch

Comment 10

18 years ago
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).
(Reporter)

Comment 11

18 years ago
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.
(Reporter)

Comment 12

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

Comment 13

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

Comment 14

18 years ago
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?)

Comment 15

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

Comment 16

18 years ago
Hmm...can you post the code you have now?

Comment 17

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

Comment 18

18 years ago
Created attachment 23554 [details] [diff] [review]
Patch: make the UI use the new command

Comment 19

18 years ago
Created attachment 23884 [details] [diff] [review]
Patch: implement copy link AND copy image (includes UI changes)

Comment 20

18 years ago
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
Whiteboard: [nsbeta1+] → [nsbeta1+] Awaiting r and sr

Comment 21

18 years ago
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 ...)
Whiteboard: [nsbeta1+] Awaiting r and sr → [nsbeta1+] Awaiting review and browser UI help

Comment 22

18 years ago
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.
Assignee: akkana → pinkerton
Status: ASSIGNED → NEW
Component: Editor → XP Toolkit/Widgets: Menus
QA Contact: sujay → jrgm
Whiteboard: [nsbeta1+] Awaiting review and browser UI help → [nsbeta1+] Patch

Comment 23

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

Comment 24

18 years ago
Created attachment 23981 [details] [diff] [review]
small docviewer change
(Reporter)

Comment 25

18 years ago
--> saari
Assignee: pinkerton → saari

Comment 26

18 years ago
Created attachment 24848 [details] [diff] [review]
updated patch, seems to work fine on Mac at least

Comment 27

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

Comment 28

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

Comment 29

18 years ago
Created attachment 25034 [details] [diff] [review]
update to make windows patch happy

Comment 30

18 years ago
Created attachment 25035 [details] [diff] [review]
update to make windows patch happy

Comment 31

18 years ago
works on linux also, must have been a transitory problem

Comment 33

18 years ago
->moz0.9
Target Milestone: mozilla0.8 → mozilla0.9

Updated

18 years ago
Blocks: 70229

Comment 34

18 years ago
Created attachment 29207 [details] [diff] [review]
updated patch for layout/content split

Comment 35

17 years ago
->dr
Assignee: saari → dr
(Assignee)

Comment 36

17 years ago
patch is slightly bitrotted. hacking...
Status: NEW → ASSIGNED
Keywords: patch
Summary: Implement command "cmd_copy_link" → Implement command "cmd_copyLink"
Whiteboard: [nsbeta1+] Patch → [nsbeta1+]
(Assignee)

Comment 37

17 years ago
Created attachment 30354 [details] [diff] [review]
cleaned up, updated patch. also fixes irritating "viwer" misspelling.
(Assignee)

Comment 38

17 years ago
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?
(Assignee)

Comment 39

17 years ago
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).
Summary: Implement command "cmd_copyLink" → Implement cmd_copyLink, cmd_copyImageLocation, cmd_copyImageContents
(Reporter)

Comment 40

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

Updated

17 years ago
Blocks: 21747
(Assignee)

Comment 41

17 years ago
Created attachment 30866 [details] [diff] [review]
more patch-love in progress. this one works, sorta. needs cleanup.
(Assignee)

Comment 42

17 years ago
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?
Keywords: review
Priority: P2 → P1
(Assignee)

Comment 43

17 years ago
Created attachment 30927 [details] [diff] [review]
the latest...
(Assignee)

Comment 44

17 years ago
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!
(Assignee)

Comment 45

17 years ago
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.
(Assignee)

Comment 46

17 years ago
Created attachment 31140 [details] [diff] [review]
Final patch, hopefully :)
(Assignee)

Updated

17 years ago
Blocks: 76318
(Assignee)

Comment 47

17 years ago
Created attachment 31262 [details] [diff] [review]
cleaned up COM-fu, printf-fu, and popupNode is now entirely managed by focus controller

Comment 48

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

Comment 49

17 years ago
r=hyatt
(Assignee)

Comment 50

17 years ago
Fixed 17 Apr. (moz0.9).
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

17 years ago
No longer blocks: 64833

Updated

10 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.