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

RESOLVED FIXED in Thunderbird 50.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

({regression})

Trunk
Thunderbird 50.0
regression
Bug Flags:
in-testsuite ?
in-moztrap ?

Thunderbird Tracking Flags

(thunderbird47 wontfix, thunderbird48 fixed, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr4549+ fixed)

Details

(Whiteboard: [regression:TB43])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
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
(Assignee)

Updated

a year ago
No longer blocks: 924530
(Assignee)

Comment 1

a year ago
Magnus, would you like to back out bug 924530?
Blocks: 924530
Flags: needinfo?(mkmelin+mozilla)

Comment 2

a year ago
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.
Flags: needinfo?(mkmelin+mozilla)
Flags: in-testsuite?
Flags: in-moztrap?
Keywords: regression
Whiteboard: [regression:TB43]
(Assignee)

Updated

a year ago
Duplicate of this bug: 1268363
(Assignee)

Comment 4

a year ago
(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.
Summary: Drag&drop of address from address book or contacts side bar opens the attachment box - regression from bug 924530 → 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

Comment 5

a year ago
Yes, that's what I meant.
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Comment 7

a year ago
Created attachment 8762368 [details] [diff] [review]
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?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
(Assignee)

Comment 8

11 months ago
Created attachment 8763400 [details] [diff] [review]
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.
Attachment #8762368 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
(Assignee)

Comment 9

11 months ago
Created attachment 8763402 [details] [diff] [review]
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.
Attachment #8763402 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 10

11 months ago
Created attachment 8763491 [details] [diff] [review]
Simple fix to stop the attachment bucket from opening (v1a).

Fixed comment and checkin comment.
Attachment #8763402 - Attachment is obsolete: true
Attachment #8763402 - Flags: review?(mkmelin+mozilla)
Attachment #8763491 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 11

11 months ago
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.
Attachment #8763491 - Flags: review?(acelists)

Comment 12

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

Comment 13

11 months ago
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!
Attachment #8763491 - Flags: review?(mkmelin+mozilla) → review+

Updated

11 months ago
Attachment #8763491 - Flags: review?(acelists)
(Assignee)

Comment 14

11 months ago
https://hg.mozilla.org/comm-central/rev/5e02381b297f
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → affected
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
OS: Unspecified → All
Hardware: Unspecified → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Comment 15

11 months ago
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.
Attachment #8763491 - Flags: approval-comm-esr45?
Attachment #8763491 - Flags: approval-comm-beta+
Attachment #8763491 - Flags: approval-comm-aurora+
(Assignee)

Comment 16

11 months ago
(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.
(Assignee)

Comment 17

11 months ago
Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/7dcfc3353d0b
status-thunderbird49: affected → fixed
(Assignee)

Comment 18

11 months ago
Beta (TB 48):
https://hg.mozilla.org/releases/comm-beta/rev/4f9859a0da24
status-thunderbird48: affected → fixed

Comment 19

8 months ago
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
Attachment #8763491 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

8 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 49+
You need to log in before you can comment on or make changes to this bug.