Editor and thus textboxes still use the old model which can cause problems. It should be converted into the new api.

This is an initial patch which changes editor to use the drag/drop api instead of the drag service.

Nothing ever sets this.

This adds the data from editable areas (textbox, input, textarea, contenteditable, designmode, <editor>) before the dragstart event in the same place as selections are done.

We can remove this as it is now unused.

This involves some rearranging to split up as clipboard uses the transferable api. Once clipboard uses datatransfer this can be cleaned up.

They both do more or less the same thing.

Note that these patches rely on bug 707382.

- Fixes up text handling and indenting and removes double call to ScrollSelectionIntoView

Updated patch

Comment on attachment 578794 [details] [diff] [review] Part 3 - remove nsIDOMNSEvent::tmpRealOriginalTarget I didn't look at other patches, but if this is possible, great!

I have tried doing a complete rebuild (rm -rf * in my obj-dir, hg pull -u, apply all the patches in increasing order of number, configure, then use pymake) and I get a build failure with this output: c:/mozilla-central/obj-dir/editor/libeditor/text/../../../../editor/libeditor/text/nsPlaintextDataTransfer.cpp(177) : error C2039: 'MozGetDataAt' : is not a member of 'nsIDOMDataTransfer' c:\mozilla-central\obj-dir\dist\include\nsIDOMDataTransfer.h(29) : see declaration of 'nsIDOMDataTransfer' c:/mozilla-central/obj-dir/editor/libeditor/text/../../../../editor/libeditor/text/nsPlaintextDataTransfer.cpp(203) : error C2039: 'GetMozItemCount' : is not a member of 'nsIDOMDataTransfer' c:\mozilla-central\obj-dir\dist\include\nsIDOMDataTransfer.h(29) : see declaration of 'nsIDOMDataTransfer' c:/mozilla-central/obj-dir/editor/libeditor/text/../../../../editor/libeditor/text/nsPlaintextDataTransfer.cpp(236) : error C2039: 'GetMozSourceNode' : is not a member of 'nsIDOMDataTransfer' c:\mozilla-central\obj-dir\dist\include\nsIDOMDataTransfer.h(29) : see declaration of 'nsIDOMDataTransfer' c:/mozilla-central/obj-dir/editor/libeditor/text/../../../../editor/libeditor/text/nsPlaintextDataTransfer.cpp(305) : error C2039: 'GetDropEffectInt' : is not a member of 'nsIDOMDataTransfer' c:\mozilla-central\obj-dir\dist\include\nsIDOMDataTransfer.h(29) : see declaration of 'nsIDOMDataTransfer'

It seems that the type of a few of the dataTransfer object types is incorrectly set to nsIDOMDataTransfer instead of nsIDOMNSDataTransfer. In these situations, couldn't the standard nsIDOMDataTransfer methods be used instead?

You also need to apply the patch in bug 707382 (the bug is marked dependant on it)

Comment on attachment 578791 [details] [diff] [review] Part 1 - remove spurious event checks Review of attachment 578791 [details] [diff] [review]: ----------------------------------------------------------------- Sweet!

Comment on attachment 578793 [details] [diff] [review] Part 2, move editor dragstart handling to ContentAreaDragDrop Review of attachment 578793 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the editor parts.

Comment on attachment 579088 [details] [diff] [review] Part 4.2 - convert drop handling to use datatransfer Review of attachment 579088 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to look at an updated version of this patch when you address the comments below. Thanks! ::: editor/libeditor/html/nsHTMLDataTransfer.cpp @@ +1298,5 @@ > // we're done! > return NS_OK; > } > > +bool nsHTMLEditor::ShouldSanitizeInsertedData(nsIDOMDocument* aSourceDoc) I think the name of this method implies the reverse of its semantics! @@ +1325,5 @@ > + nsIPrincipal* destPrincipal = destdoc->NodePrincipal(); > + NS_ASSERTION(srcPrincipal && destPrincipal, "How come we don't have a principal?"); > + rv = srcPrincipal->Subsumes(destPrincipal, &isSafe); > + if (NS_FAILED(rv)) > + return false; This should be unnecessary, since if rv is a failure code, isSafe should not be changed, and we just fall through the |return isSafe;| code. But this is not a strong objection, so if you feel this way the code is cleaner, let's keep it that way. @@ +1331,5 @@ > + > + return isSafe; > +} > + > +nsresult nsHTMLEditor::InsertObject(const char* aType, nsISupports* aObject, bool aIsSafe, I'm assuming that this method is mostly a refactored version of the code living in nsHTMLEditor::InsertFromTransferable today... @@ -1312,5 @@ > if ( NS_SUCCEEDED(transferable->GetAnyTransferData(getter_Copies(bestFlavor), getter_AddRefs(genericDataObj), &len)) ) > { > nsAutoTxnsConserveSelection dontSpazMySelection(this); > nsAutoString flavor; > - flavor.AssignWithConversion(bestFlavor); I'm not sure why this line has been removed... @@ +1484,5 @@ > + aDestinationNode, aDestOffset, > + aDoDeleteSelection, > + isSafe); > + } > + else { Nit: put these two on the same line please. @@ +1496,5 @@ > > // After ScrollSelectionIntoView(), the pending notifications might be > // flushed and PresShell/PresContext/Frames may be dead. See bug 418470. > if (NS_SUCCEEDED(rv)) > ScrollSelectionIntoView(false); I think this comment is false too. Please remove it. @@ +1635,3 @@ > nsCOMPtr<nsIDOMDocument> srcdomdoc; > + if (sourceNode) > + sourceNode->GetOwnerDocument(getter_AddRefs(srcdomdoc)); Again, I think this should fail if we fail to retrieve the source document. @@ +1720,5 @@ > break; > } > + > + nsCOMPtr<nsIDOMDocument> destdomdoc; > + GetDocument(getter_AddRefs(destdomdoc)); I think you need to add error checking here as well. @@ -1695,5 @@ > } > > - // handle transferable hooks > - if (!nsEditorHookUtils::DoInsertionHook(domdoc, aDropEvent, trans)) > - return NS_OK; Is this removal intentional? ::: editor/libeditor/text/nsPlaintextDataTransfer.cpp @@ +74,5 @@ > #include "nsContentUtils.h" > > +// private clipboard data flavors for html copy/paste > +#define kHTMLContext "text/_moz_htmlcontext" > +#define kHTMLInfo "text/_moz_htmlinfo" We're defining these constants at a bunch of places in our code base. I've always hated that. Now that you're here, could you please move them to a header (like nsITransferable.idl) and just include that header where necessary? (You can do this in a separate patch.) Thanks! @@ +173,5 @@ > + nsCOMPtr<nsIDOMDragEvent> dragEvent(do_QueryInterface(aDropEvent)); > + NS_ENSURE_TRUE(dragEvent, NS_OK); > + > + nsCOMPtr<nsIDOMDataTransfer> dataTransfer; > + dragEvent->GetDataTransfer(getter_AddRefs(dataTransfer)); I think you should add error checking for the above call. @@ +245,3 @@ > nsCOMPtr<nsIDOMDocument> srcdomdoc; > + if (sourceNode) > + sourceNode->GetOwnerDocument(getter_AddRefs(srcdomdoc)); The old code fails here if the source document cannot be retrieved. I think the new code needs to fail similarly here. @@ +295,5 @@ > for (i = 0; i < numItems; ++i) > { > + nsCOMPtr<nsIVariant> data; > + dataTransfer->MozGetDataAt(NS_LITERAL_STRING("text/plain"), i, > + getter_AddRefs(data)); Nit: indentation. @@ +307,5 @@ > > + // Beware! This may flush notifications via synchronous > + // ScrollSelectionIntoView. > + if (NS_SUCCEEDED(rv)) > + ScrollSelectionIntoView(false); Is this comment true? Async ScrollSelectionIntoView calls should not flush.

Comment on attachment 579089 [details] [diff] [review] Part 5.2 - merge the text and html editor implementations of insertFromDrop Review of attachment 579089 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/idl/nsIEditor.idl @@ -365,5 @@ > - /** > - * insertFromDrop looks for a dragsession and inserts the > - * relevant data in response to a drop. > - */ > - void insertFromDrop(in nsIDOMEvent aEvent); Please rev the IID in case for some reason we end up backing out only this patch or something! ::: editor/idl/nsIHTMLEditor.idl @@ -204,5 @@ > - /** > - * insertFromDrop looks for a dragsession and inserts the > - * relevant data in response to a drop. > - */ > - void insertFromDrop(in nsIDOMEvent aEvent); Ditto! ::: editor/libeditor/base/nsEditor.h @@ +772,5 @@ > + bool aDoDeleteSelection) { return nsnull; } > + > + virtual nsresult InsertFromDrop(nsIDOMEvent* aDropEvent) { return nsnull; } > + > + virtual already_AddRefed<nsIDOMNode> FindUserSelectAllNode(nsIDOMNode* aNode) { return nsnull; } Please make these methods pure virtual. ::: editor/libeditor/text/nsPlaintextDataTransfer.cpp @@ +236,5 @@ > + dataTransfer->GetMozSourceNode(getter_AddRefs(sourceNode)); > + > + nsCOMPtr<nsIDOMDocument> srcdomdoc; > + if (sourceNode) > + sourceNode->GetOwnerDocument(getter_AddRefs(srcdomdoc)); See my comment on the error checking case from the previous patch.

Comment on attachment 578797 [details] [diff] [review] Part 6 - editor drag and drop tests Review of attachment 578797 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/base/tests/test_dragdrop.html @@ +10,5 @@ > + <script type="application/javascript" > + src="chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js"></script> > +</head> > + > +<body onload="setTimeout(doTest, 0)"> Please call doTest as the callback of waitForFocus. @@ +111,5 @@ > + is(input.value, "Some Plain Text", "Drag text/html and text/plain onto input"); > + > + // -------- Test dragging regular text of text/plain to <textarea> > + > +// XXXndeakin Can't test textareas due to some event handling issue Please file a followup bug on this and include the bug number in this comment. I'm assuming that the actual user initiated drag-drop works for textareas. :-)

> - // handle transferable hooks > - if (!nsEditorHookUtils::DoInsertionHook(domdoc, aDropEvent, trans)) > - return NS_OK; > Is this removal intentional? Yes. Support for the hooks was removed when switching to the datatransfer drag and drop api long ago. Only the editor which still used the old api called them. Normal drag and drop event listeners can just be used instead. > + if (sourceNode) > + sourceNode->GetOwnerDocument(getter_AddRefs(srcdomdoc)); > The old code fails here if the source document cannot be retrieved. I think the new code needs to fail similarly here. Interestingly, GetOnwerDocument can fail whereas the old code which called nsIDragSession::GetSourceDocument cannot. > + virtual already_AddRefed<nsIDOMNode> FindUserSelectAllNode(nsIDOMNode* aNode) { return nsnull; } > Please make these methods pure virtual. nsPlaintextDataTransfer doesn't implement FindUserSelectAllNode so I left this one as is. > + <script type="application/javascript" > + src="chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js"></script> > +</head> > + > +<body onload="setTimeout(doTest, 0)"> > Please call doTest as the callback of waitForFocus. The test doesn't rely on focus, but I'll change this anyway.

Comment on attachment 585396 [details] [diff] [review] Part 4.3 - convert drop handling to use datatransfer Review of attachment 585396 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/text/nsPlaintextDataTransfer.cpp @@ +74,5 @@ > #include "nsContentUtils.h" > > +// private clipboard data flavors for html copy/paste > +#define kHTMLContext "text/_moz_htmlcontext" > +#define kHTMLInfo "text/_moz_htmlinfo" Still here...

Comment on attachment 585472 [details] [diff] [review] Part 6 - combine html drag type constants Review of attachment 585472 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!

Comment on attachment 578793 [details] [diff] [review] Part 2, move editor dragstart handling to ContentAreaDragDrop Ehsan's review is enough for me. I don't know this code particularly well, but if there's something in particular you want me to look at feel free to let me know what.

https://hg.mozilla.org/mozilla-central/rev/bb4c0eb9c220 https://hg.mozilla.org/mozilla-central/rev/260066ec0e47 https://hg.mozilla.org/mozilla-central/rev/788b8d367cb5 https://hg.mozilla.org/mozilla-central/rev/4978f973c79a https://hg.mozilla.org/mozilla-central/rev/71292b71e5ad https://hg.mozilla.org/mozilla-central/rev/890868587345 https://hg.mozilla.org/mozilla-central/rev/51d333edddfd