1268238 - Drag&drop of address from address book or contacts side bar opens the attachment box (but doesn't add address as vCard attachment) - regression from bug 924530

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

Drag&drop of address from address book or contacts side bar opens the attachment box.

Caused by bug 924530 since addresses are now dragged with flavour application/x-moz-file instead of text/x-moz-address.

This was discovered in bug 1267804 comment #8.

Address box opened here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4364

Comment 1

[Jorg K (CEST = GMT+2)]

Magnus, would you like to back out bug 924530?

Comment 2

[Magnus Melin [:mkmelin]]

The problem isn't that the attachment box opens. It should, so you can drag a vcard to add as attachment.
The problem is that trying to drop there drops in the To field instead.

So is the text/x-moz-address flavor missing? Maybe you just need to add it (first)?

I'd hold of backing it out, until we have time to investigate it a bit more.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #2)
> The problem isn't that the attachment box opens. It should, so you can drag
> a vcard to add as attachment.
That doesn't work. When dragging an address to the attachment box, it gets added to the composition field (To, CC, etc.) instead.

So you're saying that the desired behaviour is:
Drag to composition field: Add address, drag to attachment box: Attach vCard.

Comment 5

[Magnus Melin [:mkmelin]]

Yes, that's what I meant.

Comment 6

[Jorg K (CEST = GMT+2)]

Somehow the reference is wrong:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4504
is where the attachment box opens.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Investigation patch

I thought this would be an easy exercise for a Sunday morning, but it's not.

Although file flavours are added on onDragStart and I can see the file flavour on onDragOver, it's no longer there on onDrop.

This is the (ugly) debug output when dragging an address from the contacts sidebar to the attachment bucket which has opened:

==== abDragDrop.js: added application/x-moz-file-promise*
application/x-moz-file ==== onDragOver flavour  <== during drag over the attachment bucket
<snip>
application/x-moz-file ==== onDragOver flavour
1 ===== dnd JS
true can handle multiple
1 ===== dnd JS calling onDrop
1  === drop on attach
text/x-moz-address= flavour ===

Any idea where the other flavours got lost?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Investigation patch (v2).

No one replied, so I did some more investigation.

First we need to understand which flavours are supported in the drop onto the addressing widget:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4485
      flavourSet.appendFlavour("text/x-moz-message");
      flavourSet.appendFlavour("application/x-moz-file", "nsIFile");
      flavourSet.appendFlavour("text/x-moz-address");
      flavourSet.appendFlavour("text/x-moz-url");

Then we need to understand which flavours are set up for the drag of an address:
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abDragDrop.js#55
      aXferData.data.addDataForFlavour("moz/abcard", selectedRows);
      aXferData.data.addDataForFlavour("text/x-moz-address", selectedAddresses);
      aXferData.data.addDataForFlavour("text/unicode", selectedAddresses);
        aXferData.data.addDataForFlavour("text/vcard", decodeURIComponent(vCard));
        aXferData.data.addDataForFlavour("application/x-moz-file-promise-dest-filename", card.displayName + ".vcf");
        aXferData.data.addDataForFlavour("application/x-moz-file-promise-url", "data:text/vcard," + vCard);
        aXferData.data.addDataForFlavour("application/x-moz-file-promise", abFlavorDataProvider);

Now, with the patch we see:
==== dragOver: checking flavour from set text/x-moz-message
==== dragOver: checking flavour from set application/x-moz-file
==== dragOver: calling onDragOver with application/x-moz-file
Since the dragged address is not a "text/x-moz-message" but can be a "application/x-moz-file" (promise), the drag gets called with "application/x-moz-file" and the attachment bucked opens.

Now, on drop we see:
==== drop: supported type text/x-moz-message
==== drop: type of dragged object moz/abcard
==== drop: type of dragged object text/x-moz-address
==== drop: type of dragged object text/plain
==== drop: type of dragged object text/vcard
==== drop: type of dragged object application/x-moz-file-promise-dest-filename
==== drop: type of dragged object application/x-moz-file-promise-url
==== drop: type of dragged object application/x-moz-file-promise
==== drop: supported type application/x-moz-file
==== drop: type of dragged object moz/abcard
==== drop: type of dragged object text/x-moz-address
==== drop: type of dragged object text/plain
==== drop: type of dragged object text/vcard
==== drop: type of dragged object application/x-moz-file-promise-dest-filename
==== drop: type of dragged object application/x-moz-file-promise-url
==== drop: type of dragged object application/x-moz-file-promise
==== drop: supported type text/x-moz-address
==== drop: type of dragged object moz/abcard
==== drop: type of dragged object text/x-moz-address
==== drop: type of dragged object text/plain
==== drop: type of dragged object text/vcard
==== drop: type of dragged object application/x-moz-file-promise-dest-filename
==== drop: type of dragged object application/x-moz-file-promise-url
==== drop: type of dragged object application/x-moz-file-promise
Once again, we're checking for "text/x-moz-message" first. Next, we're checking for "application/x-moz-file" and *DO NOT* find a match since the file flavours all have "-promise" at the end.

Next we check for "text/x-moz-address" and find a match.

We also learn this: The drop is executed with one flavour only. What we would need to do is drop with flavour "text/x-moz-address" onto the addressing fields and drop with flavour "application/x-moz-file" onto the attachment bucket. To do this, they would have to be separated so they could both return a list of supported flavours.

So the best way to restore the original behaviour of not opening the attachment bucket when dragging an address is simply to change the order of the supported flavours. Patch coming up.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Simple fix to stop the attachment bucket from opening (v1).

Here is the simple fix. As explained in comment #8, the current setup doesn't allow different supported flavour for addressing and attachment bucket since they share one drag&drop handler. So therefore we cannot drop an address as vCard file into the bucket.

This patch restores the previous functionality of not opening the attachment bucket when an address is dragged. This fixes the regression from bug 924530.

Use together with WIP investigation patch to see what's going on.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Simple fix to stop the attachment bucket from opening (v1a).

Fixed comment and checkin comment.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8763491 [details] [diff] [review]
Simple fix to stop the attachment bucket from opening (v1a).

Somehow this pretty trivial change that fixes a regression has been waiting for review for almost two weeks now. I'd like to land it soon and uplift it to TB 48 beta.

Aceman, perhaps you can do the review for me. Explanation for the change in comment #8.

Comment 12

[Magnus Melin [:mkmelin]]

Not sure about the "one flavor only", the drop function can check what the target element is, (aEvent.target.localName)

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8763491 [details] [diff] [review]
Simple fix to stop the attachment bucket from opening (v1a).

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

This should be ok as a regression fix though, sorry about the delay!

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/5e02381b297f

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8763491 [details] [diff] [review]
Simple fix to stop the attachment bucket from opening (v1a).

[Approval Request Comment]
Regression caused by (bug #): bug 924530
User impact if declined: ugly puzzling behaviour of attachment bucket opening for no good reason.
Testing completed (on c-c, etc.): Manual testing.
Risk to taking this patch (and alternatives if risky):
Low risk, just reordered the list of flavours.

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #12)
> Not sure about the "one flavor only", the drop function can check what the
> target element is, (aEvent.target.localName)
Once you're in the drop function, it's to late. The drop function is called from nsDragAndDrop.js with one flavour only. Please look at the investigation patch. As I said in comment #8: If you want different drop behaviour, you need to have two distinct targets, addressing and attachments. Currently we have one target only.

Comment 17

[Jorg K (CEST = GMT+2)]

Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/7dcfc3353d0b

Comment 18

[Jorg K (CEST = GMT+2)]

Beta (TB 48):
https://hg.mozilla.org/releases/comm-beta/rev/4f9859a0da24

Comment 19

[Kent James (:rkent)]

Comment on attachment 8763491 [details] [diff] [review]
Simple fix to stop the attachment bucket from opening (v1a).

http://hg.mozilla.org/releases/comm-esr45/rev/8b518a8a6515