192567 - allow for listener to modify contents before paste/drop

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Kathleen :Brade]

The editor should allow for a listener to modify the contents before paste/drop.

Comment 1

[Judson Valeski]

cc'ing adam lock. he's done some of this kind of "filtering" in the save-as
context in the past; it may be applicable here.

also plussing for topembed.

Comment 2

[Adam Lock]

Paste / drop is done in nsClipboard.js and nsDragAndDrop.js, so it should be
possible to change either to perform any additional modification.

Comment 3

[Kathleen :Brade]

Attachment: nsIDataTransferListener.idl

Comment 4

[kinmoz]

Comment on attachment 114249 [details]
nsIDataTransferListener.idl

Should we have a return arg that allows listeners to  short circuit the
paste/drop?

Comment 5

[Kathleen :Brade]

Attachment: progress on shuffling code to make a place for hooks

Comment 6

[Kathleen :Brade]

Attachment: different tact on hook placement (work in progress)

Comment 7

[Joe Francis]

i talked with kathy out this.  thoughts so far:

we need to change the filter api so that it takes:
1) a {targetnode, offset} instead of nsrange, to specify where in doc we are
pasting.
2) a {fragmentstartnode, fragstartoffset, fragendnode, fragendoffset} to tell
where in the "fragmentAsNode" tree is the stream (as opposed to the enclosing
context).

The InsertHTMLWithBlahBlah call will need to examine results of filter call to
resync data (like rangeStartHint, etc)

We also need to refactor paste code ingeneral so that InsertFromtransferable has
some equivalent that can be told to insert at an arbitrary point instead of only
at seelction.  This will allow reordering of InsertFromDrop code so that the
deleteselection call can be defered until after the insert, which gives us a
chance to cancel an operation before we've done a deletion.

We also need to have filtering at doc load time.  We may want a param for filter
to tell it what the situation is (doc laod, drop, paste, etc).

Comment 8

[Kathleen :Brade]

Attachment: idl file (work in progress)

Comment 9

[Kathleen :Brade]

Attachment: revised idl file

Comment 10

[Joe Francis]

Attachment: code to set up filter params

Here is some code to set up the filter params, before calling the filter from
nsHTMLEditor::InsertHTMLWithCharsetAndContext().  Note that I will have to
rewrite some of nsHTMLEditor::InsertHTMLWithCharsetAndContext() to deal with
changes the filter makes to the fragment start/end.  That can come later: most
of the time the filter wont want to change where the fragment start and end
are.

Comment 11

[Kathleen :Brade]

Attachment: patch which incorporates snippet from Joe and current idl

Comment 12

[Kathleen :Brade]

Attachment: updated patch to release listeners, postpone deletion, and add comments

Although this isn't a final patch yet (since we need to not regress the
deletion/collapsing stuff) it is pretty close so I'd appreciate some feedback
(especially from Kin and Joe)

Comment 13

[Joe Francis]

Attachment: diffs to mozilla/editor

added in new work to propogate possibly adjusted boundary points between
context and stream, from the filter back into the paste code.

Comment 14

[Joe Francis]

fyi: i did not build that patch, so it may have problems.

Comment 15

[Kathleen :Brade]

Attachment: patch revision 3

Comment 16

[Joe Francis]

looks good to me.  The stuff you #if 0'd can be removed.

Comment 17

[Kathleen :Brade]

Attachment: cleanup from previous revision (after consulting with dbaron)

Comment 18

[kinmoz]

Comment on attachment 117921 [details] [diff] [review]
cleanup from previous revision (after consulting with dbaron)

==== nsIContentFilter.idl needs param comments. Comments should also tell
embedders 

what their responsibilities are in terms of changing the out parameters.


=== We could possibly be lying to the embedder about targetNode/targetOffset
since that 

can change after we do a delete selection.


==== Use nsCOMArray for mContentFilters instead of nsVoidArray.


==== DoContentFilterCallback() should bail if aDoContinue is ever set to false.


==== Do we need filter ordering? That is, the editor may set up certain filters
it 

wants/needs before embedders can register theirs. Do we need a mechanism that
allows 

people to say where their filter belongs in the chain?


==== The new args need an 'a' prefix:


 NS_IMETHODIMP nsHTMLEditor::InsertFromTransferable(nsITransferable
*transferable, 
						    const nsAString &
aContextStr,
-						    const nsAString & aInfoStr)
+						    const nsAString & aInfoStr,
+						    nsIDOMNode
*destinationNode,
+						    PRInt32 destOffset,
+						    PRBool doDeleteSelection,
+						    PRBool doCollapseSelection)


==== What is this about?


+	 // need to provide a hook from this point including passing along
+	 // doDeleteSelection and doCollapseSelection


==== Did you accidentally delete the first line of this comment?


-
-      // If we deleted the selection because we dropped from another doc,
       //  then we don't have to relocate the caret (insert at the deletion
point)
       if (!(deleteSelection && srcdomdoc != destdomdoc))


==== This entire thing should go away right? InsertFromTransferable() takes a 

doCollapseSelection which then gets passed to InsertHTMLWithCharsetAndContext()
but it 

never gets used along the way.


-      // If we deleted the selection because we dropped from another doc,
       //  then we don't have to relocate the caret (insert at the deletion
point)
       if (!(deleteSelection && srcdomdoc != destdomdoc))
       {
	 // Move the selection to the point under the mouse cursor
-	 selection->Collapse(newSelectionParent, newSelectionOffset);
+	 // selection->Collapse(newSelectionParent, newSelectionOffset);
+	 doCollapseSelection = PR_TRUE;
       }      


==== Remove the printfs:


+
+      if (mouseEvent)
+	 printf("we have a mouse event; user wants copy = %d\n",
userWantsCopy);
+      else
+	 printf("we do NOT have a mouse event so no user copy\n");



==== Make sure the args you added in nsHTMLEditor.h all start with an 'a' so
that they 

are consistent with the existing params.

Comment 19

[Kathleen :Brade]

Attachment: previous patch with cleanup based on Kin/Joe's comments

Comment 20

[Simon Fraser [no longer active]]

+  void NotifyOfInsertion(in AString mimeType,
...
+                         inout long targetOffset,
+                         out boolean continueWithInsertion);

Leading lowercase method names in idl please. Also, can't this just return a
boolean?

Descriptions are missing for some parameters (fragmentToInsertNode), and the
names seem a little confusing -- fragmentToInsertAsNode ?

+  res = DoContentFilterCallback(aFlavor, 
+                                (nsIDOMNode **)address_of(fragmentAsNode), 
+                                (nsIDOMNode **)address_of(streamStartParent), 
+                                &streamStartOffset,
+                                (nsIDOMNode **)address_of(streamEndParent),
+                                &streamEndOffset, 
+                                (nsIDOMNode **)address_of(targetNode), 
+                                &targetOffset, &doContinue);
+

Oof, this scares me. The code around this call is going to have to be very
careful not to hold any aliases to objects that can be changed by the callee.

 #ifdef XP_MAC
         mouseEvent->GetAltKey(&userWantsCopy);
+

and XP_MACOSX too.

Comment 21

[Joe Francis]

Comment on attachment 118793 [details] [diff] [review]
previous patch with cleanup based on Kin/Joe's comments

I still don't understand why we have the aCollapseSelection param since it is
unused.

Also, use nsString() instead of nsAutoString() for empty strings passed as
params - it's faster.

Comment 22

[kinmoz]

Comment on attachment 118793 [details] [diff] [review]
previous patch with cleanup based on Kin/Joe's comments

==== Rework some of the idl comments like we talked about on IRC.

==== I agree with sfraser, the arg names weren't obvious when I first saw them.
I'd suggest something like:

+  void NotifyOfInsertion(in AString mimeType,
+			  in nsIURL contentSourceURL,
+			  inout nsIDOMNode docFragment,
+			  inout nsIDOMNode contentStartNode,
+			  inout long contentStartOffset,
+			  inout nsIDOMNode contentEndNode,
+			  inout long contentEndOffset,
+			  inout nsIDOMNode insertionPointNode,
+			  inout long insertionPointOffset,
+			  out boolean continueWithInsertion);


... but it's  up to  you.

And I think there should be a comment that mentions why we are providing a
docFragment as well as the content range ... it should mention that that the
content start and end nodes aren't necessarily immediate children of the
docFrag, and any nodes above the start and end nodes are for "context" from the
source document.


==== I'd just remove the aCollapseSelection from the various methods since they
are not used (just cruft) and all the code that calculates it.


Address jfrancis and sfraser's comments and I'm fine with this.

Comment 23

[Kathleen :Brade]

Attachment: updated patch

Comment 24

[kinmoz]

Comment on attachment 118939 [details] [diff] [review]
updated patch

sr=[e-mail] with the minor comment changes we discussed on irc, and the
removal of the mContentFilters init left over from when it was a pointer:

 , mIgnoreSpuriousDragEvent(PR_FALSE)
+, mContentFilters(nsnull)
 , mTypeInState(nsnull)

Comment 25

[Joe Francis]

Comment on attachment 118939 [details] [diff] [review]
updated patch

forwarding my r=

Comment 26

[Kathleen :Brade]

checked into trunk

Comment 27

[Charles Rosendahl]

Verified.  Will file separate bugs if need be as the API is tested with listeners.