1226362 - Use HTML Drag and Drop API in Thunderbird, get rid of nsDragAndDrop.js

Status: RESOLVED FIXED

(Thunderbird :: General, task, P2)

Description

[Magnus Melin [:mkmelin]]

Bug 1171979 prepares for toolkit's bug 1162050.

We should use the HTML Drag and Drop API instead - http://developer.mozilla.org/En/DragDrop/Drag_and_Drop (and remove our copy of nsDragAndDrop.js)

Comment 1

[Axel Grude [:realRaven]]

(In reply to Magnus Melin from comment #0)
> Bug 1171979 prepares for toolkit's bug 1162050.
> 
> We should use the HTML Drag and Drop API instead -
> http://developer.mozilla.org/En/DragDrop/Drag_and_Drop (and remove our copy
> of nsDragAndDrop.js)

Are there ongoing any changes in the current Thunderbird beta (50.0b3) that might affect button dragging? Just found that the dragstart event on my QuickFolders toolbar doesn't fire anymore...

Comment 2

[Magnus Melin [:mkmelin]]

Well bug 1162050 landed. Not sure if that would affect button dragging or not.

Comment 3

[Axel Grude [:realRaven]]

Yes(In reply to Magnus Melin from comment #2)
> Well bug 1162050 landed. Not sure if that would affect button dragging or
> not.
Yes that was it - I found myself in the meantime. I will check my dragdrop events as well and make sure they are replaced.

https://developer.mozilla.org/en-US/docs/Web/Events

turned out to be very helpful, as it shows which of the events will be deprecated.

Comment 4

[Magnus Melin [:mkmelin]]

https://developer.mozilla.org/en-US/docs/Web/API/HTML_Drag_and_Drop_API

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1226362_remove-nsDragAndDrop-usage-chat-0.patch

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 9121269 [details] [diff] [review]
Bug-1226362_remove-nsDragAndDrop-usage-chat-0.patch

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

Code looks sane to me, but I can't say I understand all the changes. Magnus' review & testing here should be enough.

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1226362_remove-nsDragAndDrop-usage-addresss-0.patch

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1226362_remove-nsDragAndDrop-usage-messenger-0.patch

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9121269 [details] [diff] [review]
Bug-1226362_remove-nsDragAndDrop-usage-chat-0.patch

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

Looks to be working pretty ok, but I get this if I happen to drag the actual text node
JavaScript error: chrome://messenger/content/chat/imAccounts.js, line 710: TypeError: aEvent.target.closest is not a function

I think you need to do this check 

```
  onDragStart(aEvent) {
    let target = aEvent.target;
    if (target.nodeType != Node.ELEMENT_NODE) {
      target = target.parentNode;
    }
    let accountElement = target.closest(
      'richlistitem[is="chat-account-richlistitem"]'
    );
```

And probably the same for ondragover and drop

::: mail/components/im/content/imAccounts.js
@@ +716,5 @@
>      if (gAccountManager.accountList.itemCount == 1) {
>        throw new Error("Can't drag while there is only one account!");
>      }
>  
> +    aEvent.dataTransfer.effectAllowed = "all";

should this also be "move"?

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9121883 [details] [diff] [review]
Bug-1226362_remove-nsDragAndDrop-usage-addresss-0.patch

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

::: mailnews/addrbook/content/abDragDrop.js
@@ +97,2 @@
>  
> +    event.dataTransfer.effectAllowed = "all";

copyMove or copy, depending on case, like it was before?

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9122572 [details] [diff] [review]
Bug-1226362_remove-nsDragAndDrop-usage-messenger-0.patch

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

::: mail/base/content/mailCore.js
@@ +859,5 @@
>   *
>   * @param aAttachment the attachment object
>   * @return the TransferData
>   */
> +function CreateAttachmentTransferData(event, attachmentData, index) {

I think it would be preferable if this took event, attachments as arguments

Also, return value is now void. Please update the documentation.

@@ +874,2 @@
>  
> +  if (attachmentData.url && name) {

could just do an early return when not set

Comment 12

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #10)

copyMove or copy, depending on case, like it was before?

We are setting up "all" here in nsDragAndDrop.js: https://searchfox.org/comm-central/source/mail/base/content/nsDragAndDrop.js#371
So I followed that.

Comment 13

[Magnus Melin [:mkmelin]]

Bu that's in another place though. Maybe it was wrong? If it works, I'd update it to be more specific.

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: [checked in] Bug-1226362_remove-nsDragAndDrop-usage-addresss-1.patch

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: [checked in] Bug-1226362_remove-nsDragAndDrop-usage-chat-1.patch

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1226362_remove-nsDragAndDrop-usage-messenger-1.patch

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9122964 [details] [diff] [review]
Bug-1226362_remove-nsDragAndDrop-usage-messenger-1.patch

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

At least dragging a file from the file manager to compose (attach area) doesn't work.
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 6811: TypeError: data.exists is not a function

::: mail/base/content/mailCore.js
@@ +853,5 @@
>    return displayName.replace(/(.)\1{9,}/g, "$1…$1");
>  }
>  
>  /**
>   * Create a TransferData object for a message attachment, either from the

This is now incorrect.

@@ +856,5 @@
>  /**
>   * Create a TransferData object for a message attachment, either from the
>   * message reader or the composer.
>   *
> + * @param event       the associated event

@param {Event} event - The associated event.

@@ +857,5 @@
>   * Create a TransferData object for a message attachment, either from the
>   * message reader or the composer.
>   *
> + * @param event       the associated event
> + * @param attachments the array of attachment objects

@param {nsIMsgAttachment} attachments - The attachments to setup

@@ +862,2 @@
>   */
> +function CreateAttachmentTransferData(event, attachments) {

maybe we should rename the function too, setupDataTransfer perhaps?

@@ +874,2 @@
>  
> +    var name = attachment.name || attachment.displayName;

let's make it let instead of var now that we're touching this, for the variables below too

@@ +874,4 @@
>  
> +    var name = attachment.name || attachment.displayName;
> +
> +    if (!attachment.url) {

|| !name, so you can skip the if(name) below

::: mail/base/content/msgHdrView.js
@@ +3390,1 @@
>        for (let item of selection) {

just do 
for (let item of target.parentNode.selectedItems) {

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6508,5 @@
>     * Adjust the drop target when dragging from the attachment bucket onto itself
>     * by picking the nearest possible insertion point (generally, between two
>     * list items).
>     *
> +   * @param event the drag-and-drop event being performed

@param {Event} event - ................

@@ +6708,5 @@
>      let attachments = [];
> +    let dt = event.dataTransfer;
> +    let dataList = [];
> +    let count = 0;
> +    while (count < dt.mozItemCount) {

should be a for loop, no?

@@ +6718,5 @@
> +            let dragObj = {
> +              data,
> +              type: flavour,
> +            };
> +            dataList.push(dragObj);

please inline dragObj

dataList.push({data, type: flavour});

I'd assume the if (data) check is redundant when you already check the flavour is there

Comment 18

[Khushil Mistry [:khushil324]]

Attachment: Bug-1226362_remove-nsDragAndDrop-usage-messenger-2.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fa3c55f01adfff028a81f4bbf358af0940b2379a&selectedJob=286461971

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9123038 [details] [diff] [review]
Bug-1226362_remove-nsDragAndDrop-usage-messenger-2.patch

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

I'll attach an updated patch.

::: mail/base/content/mailCore.js
@@ +859,3 @@
>   *
> + * @param {Event} event - The associated event.
> + * @param {nsIMsgAttachment} attachments - The attachments to setup

{nsIMsgAttachment[]}

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6508,5 @@
>     * Adjust the drop target when dragging from the attachment bucket onto itself
>     * by picking the nearest possible insertion point (generally, between two
>     * list items).
>     *
> +   * @param {Event} event -  the drag-and-drop event being performed

nit: double space

@@ +6711,5 @@
> +      let types = Array.from(dt.mozTypesAt(count));
> +      for (let flavour of flavours) {
> +        if (types.includes(flavour)) {
> +          let data = dt.mozGetDataAt(flavour, count);
> +          dataList.push({ data, type: flavour });

Hmm, we don't need to make that type to confuse stuff

@@ +6720,2 @@
>      for (let dataListObj of dataList) {
> +      let data = dataListObj.data;

let's change this to 

for (let { data, favour} of dataList) {

@@ +6736,2 @@
>            try {
> +            size = data.fileSize;

Need to do an  (data instanceof Ci.nsIFile) call to make this a file you can access fileSize of
Dragged files from the filenmanager didn't get a size now.

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: [checked in] Bug-1226362_remove-nsDragAndDrop-usage-messenger-3.patch

Comment 21

[Khushil Mistry [:khushil324]]

Checkin needed for all three patches. All three patches are independent of each other.
Also, we are using nsIDragService at multiple places and I think we can remove those: https://searchfox.org/comm-central/search?q=Cc%5B%22%40mozilla.org%2Fwidget%2Fdragservice%3B1%22%5D&case=true&regexp=false&path=
What do you think about those?

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7ed85c4ae6bf
Use HTML Drag and Drop API in Messenger. r=mkmelin

Comment 23

[Khushil Mistry [:khushil324]]

Any problems with the remaining patches?

Comment 24

[Magnus Melin [:mkmelin]]

No, just saving some for landing after merges.

Comment 25

[Magnus Melin [:mkmelin]]

(In reply to Khushil Mistry [:khushil324] from comment #21)

Also, we are using nsIDragService at multiple places and I think we can remove those: https://searchfox.org/comm-central/search?q=Cc%5B%22%40mozilla.org%2Fwidget%2Fdragservice%3B1%22%5D&case=true&regexp=false&path=
What do you think about those?

Didn't look at the details, but if we can get rid of some of it, let's do it.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c5297b226713
Use HTML Drag and Drop API in Chat. r=mkmelin
https://hg.mozilla.org/comm-central/rev/1706fa6a364a
Use HTML Drag and Drop API in Address Book. r=mkmelin

Comment 27

[Magnus Melin [:mkmelin]]

Closing this not to keep it open across versions. New bug for further things, like comment 25.