499008 - Move editor over to new drag and drop api

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Neil Deakin]

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

Comment 1

[Neil Deakin]

Attachment: Patch: Initial Attempt

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

Comment 2

[Neil Deakin]

Attachment: Part 1 - remove spurious event checks

Nothing ever sets this.

Comment 3

[Neil Deakin]

Attachment: Part 2, move editor dragstart handling to ContentAreaDragDrop

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.

Comment 4

[Neil Deakin]

Attachment: Part 3 - remove nsIDOMNSEvent::tmpRealOriginalTarget

We can remove this as it is now unused.

Comment 5

[Neil Deakin]

Attachment: Part 4 - convert drop handling to use datatransfer

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

Comment 6

[Neil Deakin]

Attachment: Part 5 - merge the text and html editor implementations of insertFromDrop

They both do more or less the same thing.

Comment 7

[Neil Deakin]

Attachment: Part 6 - editor drag and drop tests

Comment 8

[Neil Deakin]

Note that these patches rely on bug 707382.

Comment 9

[Neil Deakin]

Attachment: Part 4.2 - convert drop handling to use datatransfer

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

Comment 10

[Neil Deakin]

Attachment: Part 5.2 - merge the text and html editor implementations of insertFromDrop

Updated patch

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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!

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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'

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 14

[Neil Deakin]

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

Comment 15

[(no longer active)]

Comment on attachment 578791 [details] [diff] [review]
Part 1 - remove spurious event checks

Review of attachment 578791 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet!

Comment 16

[(no longer active)]

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 17

[(no longer active)]

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 18

[(no longer active)]

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 19

[(no longer active)]

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.  :-)

Comment 20

[Neil Deakin]

Attachment: Part 4.3 - convert drop handling to use datatransfer

> -    // 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 21

[(no longer active)]

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 22

[Neil Deakin]

Attachment: Part 6 - combine html drag type constants

Comment 23

[(no longer active)]

Comment on attachment 585472 [details] [diff] [review]
Part 6  - combine html drag type constants

Review of attachment 585472 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 25

[Ed Morley [:emorley]]

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