Move editor over to new drag and drop api

RESOLVED FIXED in mozilla13

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: Neil Deakin (not available until Aug 9), Assigned: Neil Deakin (not available until Aug 9))

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

5.69 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
49.86 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
6.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.25 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
23.62 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
40.48 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
5.26 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
Editor and thus textboxes still use the old model which can cause problems. It should be converted into the new api.
Blocks: 614974
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Created attachment 578791 [details] [diff] [review]
Part 1 - remove spurious event checks

Nothing ever sets this.
Attachment #576953 - Attachment is obsolete: true
Attachment #578791 - Flags: review?(ehsan)
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.
Created attachment 578794 [details] [diff] [review]
Part 3 - remove nsIDOMNSEvent::tmpRealOriginalTarget

We can remove this as it is now unused.
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.
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.
Created attachment 578797 [details] [diff] [review]
Part 6 - editor drag and drop tests
Note that these patches rely on bug 707382.
Depends on: 707382
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
Attachment #578795 - Attachment is obsolete: true
Created attachment 579089 [details] [diff] [review]
Part 5.2 - merge the text and html editor implementations of insertFromDrop

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+
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.
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+
Created attachment 585472 [details] [diff] [review]
Part 6  - combine html drag type constants
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+
Blocks: 588270
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)
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Flags: in-testsuite+

Updated

6 years ago
Depends on: 728581

Updated

6 years ago
Depends on: 728652

Updated

6 years ago
Depends on: 728707
Depends on: 730307

Updated

6 years ago
Depends on: 732585

Updated

5 years ago
Depends on: 730694
No longer depends on: 730694
Depends on: 730694

Updated

5 years ago
Depends on: 775366
Depends on: 781611

Updated

5 years ago
Depends on: 788404

Updated

4 years ago
Depends on: 900414

Updated

3 years ago
Depends on: 1050566
You need to log in before you can comment on or make changes to this bug.