46867 - nsIClipboardCommands (or a generic command dispatch API) needs to be implemented.

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[Judson Valeski]

You get it by doing a GetInterface on the web browser object. 
nsIContentViewerEdit is not needed. 
nsIClipboard is a lower-level interface that we might want to expose as well for
     X selection. 
SelectNone() semantics need to be specified - where is the insertion point
     (collapsed selection) 
CanPasteSelection() should be renamed to CanPaste() 
PasteSelection() should be renamed to Paste()

Comment 1

[Judson Valeski]

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

Comment 2

[Judson Valeski]

-> potts

Comment 3

[Adam Lock]

The nsIClipboardCommands & nsIContentViewerEdit interfaces are practically 
identical. I think they should be consolidated into a single interface.

BTW the current implementation of nsIClipboardCommands on the nsWebshell 
doesn't do anything at all.

Comment 4

[lchiang]

per email with Jud, changing nsbeta3+ to nsbeta3- on all "embed" keyword bugs
since embedding changes will not be made in the mn6 branch. If you feel this bug
fix needs to go into mn6 branch, please list the reasons/user impact/risk and
nominate for rtm. Thanks.

Comment 5

[Judson Valeski]

over to casa de pink

Comment 6

[Peter Trudelle]

M0.9

Comment 7

[Peter Trudelle]

Pink: please add swag to status whiteboard

Comment 8

[Judson Valeski]

We need text fields (form as well as composer content areas) in content areas to
be able to support cut/copy/paste when embedded. HTML content areas (static)
need to support "copy."

It would be helpful to have an understanding of how much of this relies on XUL
(and in what areas) and how much doesn't.

Comment 9

[Mike Pinkerton (not reading bugmail)]

none of the clipboard stuff relies on XUL. I'm not sure what you need from me
here. What needs to be written apart from what is already written?

Comment 10

[Judson Valeski]

on the trivial end, method renaming suggested above.

pavlov, you had a lot of input on this topic in the api review meeting that
generated this bug. More specifically, how do we differentiate between selection
level copy on linux, vs. ctrl-c copies. Does the current iface accommodate?

Comment 11

[Stuart Parmenter]

i said that people should use nsIClipboard and avoid nsIClipboardCommands all
together.

Comment 12

[Mike Pinkerton (not reading bugmail)]

i guess the goal of nsIClipboardCommands is that to handle a paste from the app,
the embedding app shouldn't have to know how to get the HTML text field and how
to put the data into it.

All i plan on doing is to make these calls use the commandDispatcher to dispatch
the appropriate command to the currently focussed widget, then we won't have to
write a bunch of special-case code.

Comment 13

[Mike Pinkerton (not reading bugmail)]

notes to self:

implement (from globalOverlay.js), |goDoCommand()| and |goUpdateCommand()| in
C++ to send the relevant command string to the focussed object.

Comment 14

[Judson Valeski]

so you're saying that it's up to us to handle an internal cut/copy->paste (works
for me), but what if I (an embedding app) want to copy a URL from the content
area (us) and paste it into my URL bar?

Should this bug depend on this cmdDispatcher bug?
http://bugzilla.mozilla.org/show_bug.cgi?id=54203

Consider what functionality an embedding app is going to want/need. They're
going to want to cut/copy/paste, just like we do.

Comment 15

[Mike Pinkerton (not reading bugmail)]

jud, the embedding app needs to know when something in our embedded area is
focussed in order to call our API, or to handle it itself. Once that is covered,
what you mentioned should be a no-brainer.

Comment 16

[Mike Pinkerton (not reading bugmail)]

after talking to hyatt, we decided that instead of having specific command
dispatch APIs for different functionality, we should just come up with a single
generic one. It will certainly be needed for things other than just clipboard,
and then we can obsolete nsIClipboardCommands.

cc'ing hyatt and adam lock.

Comment 17

[Adam Lock]

We already have a generic command handler interface in nsICommandHandler. 

See:

http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsICommandH
andler.idl

Ignore the nsICommandHandlerInit interface - that's just some implementation 
specific glue for nsCommandHandler.cpp.

The nsICommandHandler interface is pretty simple with just exec and query 
methods that any object in C++ or Javascript can implement.

You could delete nsIClipboardCommands and do the same thing through 
nsICommandHandler e.g.

Before:

nsCOMPtr<nsIClipboardCommands> clip = do_QueryInterface(someObject);
PRBool canCut;
clip->CanCutSelection(&canCut);
if (canCut)
{
    clip->CutSelection();
}

After:

nsCOMPtr<nsICommandHandler> cmd = do_QueryInterface(someObject);
nsXPIDLCString canCut;
nsCString yes; yes.AssignWithConversion("yes");
cmd->Query("canCutCutSelection", nsnull, getter_Copies(canCut));
if (yes.EqualsWithConversion(canCut))
{
    cmd->Exec("cutSelection", nsnull, nsnull);
}

Obviously the verbs, arguments and result strings are all part of the contract 
between caller and callee so they should be defined somewhere and not hardcoded 
as above.

Comment 18

[Mike Pinkerton (not reading bugmail)]

if what adam says is the case, then I will cvs remove nsIClipboardCommands and
mark this bug invalid. arguments?

Comment 19

[Peter Trudelle]

Don't mark it invalid. This is tracking the embedding need for clipboard
support. It sounds like this may not have been adequately defined and documented
for embedders, perhaps that is all that remains.

Comment 20

[Simon Fraser [no longer active]]

Wait up. I recall, in one API review meeting I was at, that people said that we 
should provide functionality-specific command handling interfaces for embedders. 
I.e. that we *do* provide things like nsIClipboardCommands, instead of generic 
"do named command" type interfaces. Judson, Doug -- comments?

Comment 21

[David Hyatt]

Why would we need to complicate things with additional interfaces though?

Comment 22

[Mike Pinkerton (not reading bugmail)]

who was asking for these additional interfaces? Can someone answer hyatt's 
question? I need to know if there is work to be done, or not.

Comment 23

[Judson Valeski]

the api changes fell out of a mozilla porkjockeys meeting a few months ago. We
glossed over this one w/ out going into too much detail because it seemed hard.
I'm going to forward this bug to porkjockeys so we can open this up for
discussion again.

Comment 24

[Mike Pinkerton (not reading bugmail)]

i have the code written (figured i might as well, it was easy). How do i go 
about testing any of it?

Comment 25

[Mike Pinkerton (not reading bugmail)]

Attachment: Patch to implement nsIClipboardCommands, needs review

Comment 26

[Mike Pinkerton (not reading bugmail)]

can someone r= the attached patch to nsWebShell? I've noticed that none of the
test harnesses use it, so I can't really test it.

Comment 27

[Adam Lock]

Webshell is an evil thing and I'm not sure I like the idea of it growing again. 
Should this stuff be moved into docshell or would that bad too?

Also, if we're covering a generic interface (nsIController) with a custom 
interface (nsIClipboardCommands), then perhaps webshell (or docshell) should 
also implement nsIController so people can inject the string commands directly.

Comment 28

[Mike Pinkerton (not reading bugmail)]

whatever. i just filled in the stubbed out interface. if you guys want the code
elsewhere, let me know.

Comment 29

[Mike Pinkerton (not reading bugmail)]

ok, conrad added some stuff to the mac embedding harness and the new clipboard
commands do actually work! Note, in the diff above, I had to prefix the commands
with "cmd_" to get it to work.

Comment 30

[Mike Pinkerton (not reading bugmail)]

r=sfraser/a=hyatt. fixed.

Comment 31

[Judson Valeski]

so am I able to get the nsIClipboardCommands interface from the webbrowser object?

Comment 32

[Judson Valeski]

re-opening as the methods haven't been renamed (as outlined in the first comment
of the bug). the concept of "selection" isn't valid in the iface anymore (it's
implicit).

Comment 33

[Mike Pinkerton (not reading bugmail)]

i've made the changes. the semantics of SelectNone() are that the insertion
point is at the beginning of the previous selection. If you disagree, speak up.
It's easy to change.

Comment 34

[Mike Pinkerton (not reading bugmail)]

checked in

Comment 35

[Paul Wyskoczka]

Updating QA Contact

Comment 36

[Michael Dunn]

Test with WinEmbed.exe