Move editor over to new drag and drop api

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

Editor and thus textboxes still use the old model which can cause problems. It should be converted into the new api.
Blocks: 614974
Posted patch Patch: Initial Attempt (obsolete) — Splinter Review
This is an initial patch which changes editor to use the drag/drop api instead of the drag service.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Nothing ever sets this.
Attachment #576953 - Attachment is obsolete: true
Attachment #578791 - Flags: review?(ehsan)
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.
Depends on: 707382
- Fixes up text handling and indenting and removes double call to ScrollSelectionIntoView
Attachment #578795 - Attachment is obsolete: true
Updated patch
Attachment #578796 - Attachment is obsolete: true
Attachment #578794 - Flags: review?(bugs)
Attachment #578793 - Flags: review?(jonas)
Attachment #578793 - Flags: review?(ehsan)
Attachment #578797 - Flags: review?(ehsan)
Attachment #579088 - Flags: review?(ehsan)
Attachment #579089 - Flags: review?(ehsan)
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!
Attachment #578794 - Flags: review?(bugs) → review+
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!
Attachment #578791 - Flags: review?(ehsan) → review+
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.
Attachment #578793 - Flags: review?(ehsan) → review+
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.
Attachment #579088 - Flags: review?(ehsan)
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.
Attachment #579089 - Flags: review?(ehsan) → review+
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.  :-)
Attachment #578797 - Flags: review?(ehsan) → review+
> -    // 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.
Attachment #579088 - Attachment is obsolete: true
Attachment #585396 - Flags: review?(ehsan)
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...
Attachment #585396 - Flags: review?(ehsan) → review+
Attachment #585472 - Flags: review?(ehsan)
Comment on attachment 585472 [details] [diff] [review]
Part 6  - combine html drag type constants

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

Looks great!
Attachment #585472 - Flags: review?(ehsan) → review+
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.
Attachment #578793 - Flags: review?(jonas)
Flags: in-testsuite+
Depends on: 728581
Depends on: 728652
Depends on: 728707
Depends on: 730307
Depends on: 732585
Depends on: 730694
No longer depends on: 730694
Depends on: 730694
Depends on: 775366
Depends on: 781611
Depends on: 788404
Depends on: 900414
Depends on: 1050566
You need to log in before you can comment on or make changes to this bug.