Last Comment Bug 499008 - Move editor over to new drag and drop api
: Move editor over to new drag and drop api
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Neil Deakin
:
Mentors:
Depends on: 707382 728581 728652 728707 730307 730694 732585 775366 781611 788404 900414 1050566
Blocks: 588270 614974
  Show dependency treegraph
 
Reported: 2009-06-17 14:47 PDT by Neil Deakin
Modified: 2014-08-19 05:33 PDT (History)
12 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: Initial Attempt (62.82 KB, patch)
2011-11-25 09:10 PST, Neil Deakin
no flags Details | Diff | Review
Part 1 - remove spurious event checks (5.69 KB, patch)
2011-12-02 17:46 PST, Neil Deakin
ehsan: review+
Details | Diff | Review
Part 2, move editor dragstart handling to ContentAreaDragDrop (49.86 KB, patch)
2011-12-02 17:53 PST, Neil Deakin
ehsan: review+
Details | Diff | Review
Part 3 - remove nsIDOMNSEvent::tmpRealOriginalTarget (6.47 KB, patch)
2011-12-02 17:56 PST, Neil Deakin
bugs: review+
Details | Diff | Review
Part 4 - convert drop handling to use datatransfer (36.61 KB, patch)
2011-12-02 17:59 PST, Neil Deakin
no flags Details | Diff | Review
Part 5 - merge the text and html editor implementations of insertFromDrop (23.72 KB, patch)
2011-12-02 18:00 PST, Neil Deakin
no flags Details | Diff | Review
Part 6 - editor drag and drop tests (8.25 KB, patch)
2011-12-02 18:01 PST, Neil Deakin
ehsan: review+
Details | Diff | Review
Part 4.2 - convert drop handling to use datatransfer (36.68 KB, patch)
2011-12-05 09:22 PST, Neil Deakin
no flags Details | Diff | Review
Part 5.2 - merge the text and html editor implementations of insertFromDrop (23.62 KB, patch)
2011-12-05 09:22 PST, Neil Deakin
ehsan: review+
Details | Diff | Review
Part 4.3 - convert drop handling to use datatransfer (40.48 KB, patch)
2012-01-03 06:37 PST, Neil Deakin
ehsan: review+
Details | Diff | Review
Part 6 - combine html drag type constants (5.26 KB, patch)
2012-01-03 11:03 PST, Neil Deakin
ehsan: review+
Details | Diff | Review

Description Neil Deakin 2009-06-17 14:47:18 PDT
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 2011-11-25 09:10:35 PST
Created attachment 576953 [details] [diff] [review]
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 2011-12-02 17:46:11 PST
Created attachment 578791 [details] [diff] [review]
Part 1 - remove spurious event checks

Nothing ever sets this.
Comment 3 Neil Deakin 2011-12-02 17:53:13 PST
Created attachment 578793 [details] [diff] [review]
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 2011-12-02 17:56:14 PST
Created attachment 578794 [details] [diff] [review]
Part 3 - remove nsIDOMNSEvent::tmpRealOriginalTarget

We can remove this as it is now unused.
Comment 5 Neil Deakin 2011-12-02 17:59:13 PST
Created attachment 578795 [details] [diff] [review]
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 2011-12-02 18:00:34 PST
Created attachment 578796 [details] [diff] [review]
Part 5 - merge the text and html editor implementations of insertFromDrop

They both do more or less the same thing.
Comment 7 Neil Deakin 2011-12-02 18:01:05 PST
Created attachment 578797 [details] [diff] [review]
Part 6 - editor drag and drop tests
Comment 8 Neil Deakin 2011-12-02 18:01:54 PST
Note that these patches rely on bug 707382.
Comment 9 Neil Deakin 2011-12-05 09:22:25 PST
Created attachment 579088 [details] [diff] [review]
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 2011-12-05 09:22:58 PST
Created attachment 579089 [details] [diff] [review]
Part 5.2 - merge the text and html editor implementations of insertFromDrop

Updated patch
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-12-07 02:41:44 PST
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-12-20 21:08:39 PST
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-12-20 22:59:15 PST
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 2011-12-21 05:48:34 PST
You also need to apply the patch in bug 707382 (the bug is marked dependant on it)
Comment 15 :Ehsan Akhgari (out sick) 2011-12-29 12:11:52 PST
Comment on attachment 578791 [details] [diff] [review]
Part 1 - remove spurious event checks

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

Sweet!
Comment 16 :Ehsan Akhgari (out sick) 2011-12-29 12:15:19 PST
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 :Ehsan Akhgari (out sick) 2011-12-29 12:48:20 PST
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 :Ehsan Akhgari (out sick) 2011-12-29 12:58:49 PST
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 :Ehsan Akhgari (out sick) 2011-12-29 13:04:20 PST
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 2012-01-03 06:37:39 PST
Created attachment 585396 [details] [diff] [review]
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 :Ehsan Akhgari (out sick) 2012-01-03 10:58:44 PST
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 2012-01-03 11:03:49 PST
Created attachment 585472 [details] [diff] [review]
Part 6  - combine html drag type constants
Comment 23 :Ehsan Akhgari (out sick) 2012-01-03 11:12:48 PST
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) PTO Until July 5th 2012-02-16 10:28:08 PST
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.

Note You need to log in before you can comment on or make changes to this bug.