The default bug view has changed. See this FAQ.

Address Book Contact Drag & Drop to another address book not working

RESOLVED FIXED in Thunderbird 49.0

Status

Thunderbird
Address Book
P1
major
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Prashant, Assigned: Jorg K (GMT+1))

Tracking

({regression})

49 Branch
Thunderbird 49.0
x86_64
Windows 7
regression

Thunderbird Tracking Flags

(thunderbird47 unaffected, thunderbird48 fixed, thunderbird49 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

11 months ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160502030207

Steps to reproduce:

Address Book Contact Drag & Drop not Working from One Address Book to Another Book


Actual results:

Address Book Contact Drag & Drop not Working from One Address Book to Another Book


Expected results:

Address Book Contact Drag & Drop not Working from One Address Book to Another Book
(Reporter)

Updated

11 months ago
Severity: normal → major
OS: Unspecified → Windows 7
Priority: -- → P1
Hardware: Unspecified → x86_64
(Assignee)

Comment 1

11 months ago
Confirmed.

This works in TB 45 and TB 47.0a2 (2016-03-16) , so it's a recent regression. Alice, can you find it for me?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(alice0775)
Keywords: regression, regressionwindow-wanted
Summary: Address Book Contact Drag & Drop not Working → Address Book Contact Drag & Drop to another address book not working
(Assignee)

Comment 2

11 months ago
Maybe bug 860857? https://hg.mozilla.org/mozilla-central/rev/4cbc83f4c4b1?
(That's already caused bug 1267259.)

Comment 3

11 months ago
(In reply to Jorg K (GMT+2) from comment #2)
> Maybe bug 860857? https://hg.mozilla.org/mozilla-central/rev/4cbc83f4c4b1?
> (That's already caused bug 1267259.)

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0891f0fa044cba28024849803e170ed7700e01e0&tochange=fc15477ce628599519cb0055f52cc195d640dc94
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=aaf73ae87441&tochange=a5f12c8dc64f

Suspect Bug 860857
Blocks: 860857
Flags: needinfo?(alice0775)
Keywords: regressionwindow-wanted
(Assignee)

Comment 4

11 months ago
Thanks, Alice!

Neil, something in bug 860857 caused another regression (previous one in bug 1267259).

The addresses we drag carry a few flavours, as you can see here:
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abDragDrop.js#55
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abDragDrop.js#76

Any hints? I haven't debugged it yet. Basically, when we drag the address over an address book, we get the "no parking sign" where a drop target should offer itself.
Flags: needinfo?(enndeakin)
(Assignee)

Updated

11 months ago
status-thunderbird47: --- → unaffected
status-thunderbird48: --- → affected
status-thunderbird49: --- → affected

Comment 5

11 months ago
That addressbook code is using the old drag and drop apis which shouldn't have been affected by the changes in 860857, so I don't know offhand.

Assuming that abDirTreeObserver is what you are dropping on, can you confirm that abDirTreeObserver.canDrop is being called and what data it is receiving? Maybe check what nsTreeBodyFrame::GetDropEffect is being set to?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 6

11 months ago
(In reply to Neil Deakin from comment #5)
> That addressbook code is using the old drag and drop apis which shouldn't
> have been affected by the changes in 860857, so I don't know offhand.
What's old? In bug 1171979 we already moved from the legacy events: draggesture to dragstart, dragdrop to drop (only that bug 1162050 hasn't landed). So dragstart/drop are old?
 
> Assuming that abDirTreeObserver is what you are dropping on, can you confirm
> that abDirTreeObserver.canDrop is being called and what data it is
> receiving? Maybe check what nsTreeBodyFrame::GetDropEffect is being set to?
I'll look into it.

Comment 7

11 months ago
(In reply to Jorg K (GMT+2) from comment #6)
> What's old? In bug 1171979 we already moved from the legacy events:
> draggesture to dragstart, dragdrop to drop (only that bug 1162050 hasn't
> landed). So dragstart/drop are old?

No, I mean it is using nsITransferable directly rather than event.dataTransfer as well as the obsolete nsDragAndDrop.js script.
(Assignee)

Comment 8

11 months ago
OK, returns false here:
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abDragDrop.js#203

So trans.getAnyTransferData(flavor, dataObj, len); a few lines before throws.

As you said, using nsITransferable:
https://dxr.mozilla.org/comm-central/source/mozilla/widget/nsITransferable.idl#144

So the behaviour of this changed?
Flags: needinfo?(enndeakin)

Comment 9

11 months ago
OK, so it looks like nsDragAndDrop.js is actually using DataTransfer, so this could cause an issue. The dragstart event is using DataTransfer which now encodes non-recognized string types into a single type and ignores non-string data. This means that non-string data isn't encoded into an nsITransferable, but is kept in the DataTransfer. I'm guessing that 'moz/abcard' is assigned some non-string data.

For dropping however, the address book code is not using DataTransfer but is instead using nsITransferable directly. Now, 'moz/abcard' does not exist.

I'd suggest changing canDrop to use DataTransfer instead. canDrop takes a third (optional) argument which specifies the dataTransfer. You might verify:

canDrop: function(index, orientation, dataTransfer)
 {
    if (dataTransfer.types.contains("moz/abcard") {
      ... 
    }
 }

As a bonus, it should allow this code to be significantly simplified.
Flags: needinfo?(enndeakin)
(Assignee)

Comment 10

11 months ago
(In reply to Neil Deakin from comment #9)
> OK, so it looks like nsDragAndDrop.js is actually using DataTransfer, so
> this could cause an issue. The dragstart event is using DataTransfer which
> now encodes non-recognized string types into a single type and ignores
> non-string data. This means that non-string data isn't encoded into an
> nsITransferable, but is kept in the DataTransfer. I'm guessing that
> 'moz/abcard' is assigned some non-string data.
I guess so:
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abDragDrop.js#55
One line and two lines below we assign text to text/x-moz-address and text/unicode,
but moz/abcard gets assigned "selected rows".

> For dropping however, the address book code is not using DataTransfer but is
> instead using nsITransferable directly. Now, 'moz/abcard' does not exist.
So that's a consequence of the change you described.

> I'd suggest changing canDrop to use DataTransfer instead. canDrop takes a
> third (optional) argument which specifies the dataTransfer. You might verify:
> canDrop: function(index, orientation, dataTransfer)
>  {
>     if (dataTransfer.types.contains("moz/abcard") {
>       ... 
>     }
>  }

Hmm, I've done that:
canDrop: function(index, orientation, dataTransfer)
and further down to inspect the object:
    if (dataTransfer.types.contains("moz/abcard")) {
      for (var key in dataTransfer) dump ("key=|"+ key + "|, value=|" + dataTransfer[key] + "|\n");
    }

Result:
abDragDrop.js, line 192: TypeError: dataTransfer is undefined

If it worked, how would I get to the data? Sorry, I'm not a great JS programmer.

Comment 11

11 months ago
OK, so I was assuming that was implementation of nsITreeView::CanDrop. Perhaps it isn't. I'm not familiar with Thunderbird code. Maybe

https://dxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abTrees.js#174

needs to pass on the dataTransfer?
(Assignee)

Comment 12

11 months ago
Created attachment 8748378 [details] [diff] [review]
WIP - mainly for debugging

Neil, thank you so much for doing the debugging for me. Yes, the suggested change gets me the data into "canDrop".

Dumping out the dataTransfer object I saw a "getData" method, so I tried:
    if (dataTransfer.types.contains("moz/abcard")) {
      var data = dataTransfer.getData("moz/abcard");
      for (var key in data) dump ("key=|"+ key + "|, value=|" + data[key] + "|\n");
    }

If I drag items 2 and 3 from an address book, I get this:
key=|0|, value=|2|
key=|1|, value=|,|
key=|2|, value=|3|
So that represents the "selected rows".

I'll take it from here. Thanks again.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 13

11 months ago
Created attachment 8748402 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v1)

This does the job. The code is much simplified. Sorry about all the white-space changes due to changed indentation.
Attachment #8748378 - Attachment is obsolete: true
Attachment #8748402 - Flags: review?(philip.chee)
Attachment #8748402 - Flags: review?(acelists)
(Assignee)

Comment 14

11 months ago
Created attachment 8748410 [details] [diff] [review]
Same patch, but indentation left in place. Easier to review.
(Assignee)

Comment 15

11 months ago
Comment on attachment 8748402 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v1)

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

::: mailnews/addrbook/content/abDragDrop.js
@@ +187,2 @@
>  
> +    if (!dataTransfer.types.contains("moz/abcard")) {

includes() is better, right?

@@ +226,5 @@
>    {
>      var dragSession = dragService.getCurrentSession();
>      if (!dragSession)
>        return;
> +    if (!dataTransfer.types.contains("moz/abcard")) {

includes(), see above.

@@ +227,5 @@
>      var dragSession = dragService.getCurrentSession();
>      if (!dragSession)
>        return;
> +    if (!dataTransfer.types.contains("moz/abcard")) {
> +      return false;

Sorry, I'll change this to: return;

Comment 16

11 months ago
Comment on attachment 8748402 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v1)

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

::: mailnews/addrbook/content/abDragDrop.js
@@ +187,2 @@
>  
> +    if (!dataTransfer.types.contains("moz/abcard")) {

Yes, includes().

@@ +188,5 @@
> +    if (!dataTransfer.types.contains("moz/abcard")) {
> +      return false;
> +    }
> +
> +    // The data contains the "selected rows" in a comma-separated way.

What is comma-separated in this case? You handle it as rows = ["data1", "," , "data2" ] and skip when the member is a comma. Is that true?
(Assignee)

Comment 17

11 months ago
Thanks for looking at it. Can you cope with the original patch, or is the second one with less white-space changes better for you?

If you look at comment #12, you can see the structure of the data:
key=|0|, value=|2|
key=|1|, value=|,|
key=|2|, value=|3|
So yes, I handle it as [ind1, ",", ind2].

It's set up here:
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abDragDrop.js#55

If you have a better idea, please let me know. Surely a JS connoisseur could kill the commas in one statement without having to skip them in the loop. Sadly my JS skills are not that great.

Comment 18

11 months ago
So you say that when selectedRows = "1,2" then passing it via
aXferData.data.addDataForFlavour("moz/abcard", selectedRows) to your function produces an array [ 1, ",", 2] ? That looks kinda strange. How does it decide how to split the string? Or is that defined somewhere else for the "moz/abcard" type?

Comment 19

11 months ago
(In reply to Jorg K (GMT+2) from comment #17)
> If you have a better idea, please let me know. Surely a JS connoisseur could
> kill the commas in one statement without having to skip them in the loop.
> Sadly my JS skills are not that great.

You could kill them like array = array.filter(x => x != ",") but I also sometimes do not like these increasingly unreadable JS constructs :)
(Assignee)

Comment 20

11 months ago
I have no idea:
As I said repeatedly: Data is set up here:
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abDragDrop.js#55
aXferData.data.addDataForFlavour("moz/abcard", selectedRows);
I have no idea what selectedRows contains, most likely an array of integers from
var selectedRows = GetSelectedRows();

When I retrieve dataTransfer.getData("moz/abcard"); I seem to get an array [ 1, ",", 2], that's what
key=|0|, value=|2|
key=|1|, value=|,|
key=|2|, value=|3|
tells me, right?
See comment #12:
  var data = dataTransfer.getData("moz/abcard");
  for (var key in data) dump ("key=|"+ key + "|, value=|" + data[key] + "|\n");

OK, I'll dump out selectedRows as well.
(Assignee)

Comment 21

11 months ago
OK, includes() gives:
TypeError: dataTransfer.types.includes is not a function
Neil suggested "contains()" in comment #9.

Here is the data: in and out are the same. So kindly review the patch now:
  aXferData.data.addDataForFlavour("moz/abcard", selectedRows);
  for (var key in selectedRows) dump ("in - key=|"+ key + "|, value=|" + selectedRows[key] + "|\n");

  var rows = dataTransfer.getData("moz/abcard");
  var numrows = rows.length;
  for (var key in rows) dump ("out - key=|"+ key + "|, value=|" + rows[key] + "|\n");

in - key=|0|, value=|2|
in - key=|1|, value=|,|
in - key=|2|, value=|5|
out - key=|0|, value=|2|
out - key=|1|, value=|,|
out - key=|2|, value=|5|

Let me know whether I should use 
var rows = dataTransfer.getData("moz/abcard").filter(x => x != ",");
or rather skip the commas in the loops.
(Assignee)

Comment 22

11 months ago
Created attachment 8748576 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v1a).

OK, I killed the erroneous "return false" and explained the structure of the data in a comment. I left the contains() since includes() somehow doesn't work.

This should be good to go.
Attachment #8748402 - Attachment is obsolete: true
Attachment #8748410 - Attachment is obsolete: true
Attachment #8748402 - Flags: review?(philip.chee)
Attachment #8748402 - Flags: review?(acelists)
Attachment #8748576 - Flags: review?(acelists)

Comment 23

11 months ago
(In reply to Jorg K (GMT+2) from comment #21)
> OK, includes() gives:
> TypeError: dataTransfer.types.includes is not a function
> Neil suggested "contains()" in comment #9.

Sorry, I assumed datatransfer.types is an array. But if it isn't use what works :)

> in - key=|0|, value=|2|
> in - key=|1|, value=|,|
> in - key=|2|, value=|5|
> out - key=|0|, value=|2|
> out - key=|1|, value=|,|
> out - key=|2|, value=|5|

Here I did previously look up what selectedRows contains, and if I look at the correct function at http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abResultsPane.js#233 then it seems to me it should be a string. How comes you can decode it in the 'in' block as an array? Or does the for..in loop actually loop over the single characters in the string? In which case it could fail on multiple digit card indexes.
Can you try dropping cards at higher-indexed rows? So the string is e.g. "1,10" ?
(Assignee)

Comment 24

11 months ago
Created attachment 8748583 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v2).

Damn, you're right, it's just a string. Yes, you can access the individual characters as elements of an array ;-)

Here's the revised patch, I noticed that is doesn't work, when dragging more than one, I lose some of the ones that are not dragged from the source. I'll have to look at it further later today.
Attachment #8748576 - Attachment is obsolete: true
Attachment #8748576 - Flags: review?(acelists)

Comment 25

11 months ago
(In reply to Jorg K (GMT+2) from comment #22)
> Created attachment 8748576 [details] [diff] [review]
> Adapt address drag/drop to new scheme after bug 860857 (v1a).
> 
> OK, I killed the erroneous "return false" and explained the structure of the
> data in a comment. I left the contains() since includes() somehow doesn't
> work.

This is bug 1250621.
(Assignee)

Comment 26

11 months ago
Created attachment 8748651 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v3).

Now it's right ;-)

(In reply to Jorg K (GMT+2) from comment #24)
> Here's the revised patch, I noticed that is doesn't work, when dragging more
> than one, I lose some of the ones that are not dragged from the source. I'll
> have to look at it further later today.
Yes, I had the loop level wrong and it deleted more cards that I had moved. Fixed that one. The patch looks pretty terrible due to the changed indentation.

Maybe one day we should be using MozReview which can cope with white-space better.
Attachment #8748583 - Attachment is obsolete: true
Attachment #8748651 - Flags: review?(acelists)

Comment 27

11 months ago
Comment on attachment 8748651 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v3).

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

I need to see why I have the feeling I have already reviewed this AB code recently.

Can you please produce the patch version without indent changes again?

::: mailnews/addrbook/content/abDragDrop.js
@@ +187,4 @@
>  
> +    if (!dataTransfer.types.contains("moz/abcard")) {
> +      return false;
> +    }

Can you move this above the comment block? We probably do not need the draggingMailList variable if we don't want to do anything.

@@ +197,2 @@
>      {
> +      if (gAbView.getCardFromRow(rows[j]).isMailList)

If you wanted to be fancy, the whole loop can reduce to draggingMailList = rows.some(row => gAbView.getCardFromRow(row).isMailList);
(Assignee)

Comment 28

11 months ago
Created attachment 8748700 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v3a) [checked in with mods, comment #32].

Check moved up. Patch without white-space change coming.

Aceman, you were in here for the "all address books" stuff.
Attachment #8748651 - Attachment is obsolete: true
Attachment #8748651 - Flags: review?(acelists)
Attachment #8748700 - Flags: review?(acelists)
(Assignee)

Comment 29

11 months ago
Created attachment 8748703 [details] [diff] [review]
Same patch v3a but without indent change (so indent is wrong).
(Assignee)

Comment 30

11 months ago
Oops, there is a double blank line after var draggingMailList = false; and I also forgot to remove the dumps ;-(

I'll fix that in the next round.

Comment 31

11 months ago
Comment on attachment 8748703 [details] [diff] [review]
Same patch v3a but without indent change (so indent is wrong).

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

Thanks, for for me. Would you mind if we actually checked in this patch without the indent changes? It is better if we do not clutter the history of lines that do not change functionally.
Attachment #8748703 - Flags: review+
(Assignee)

Comment 32

11 months ago
Checked in the original v3a patch with the fixed indent after a quick survey on IRC.
Removed dump() and double spacing first (see comment #30).

https://hg.mozilla.org/comm-central/rev/067b77c50b68
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-thunderbird49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 49.0
(Assignee)

Comment 33

11 months ago
Comment on attachment 8748700 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v3a) [checked in with mods, comment #32].

This will need to go to Aurora, since it was regressed by changes in mozilla48.
Attachment #8748700 - Attachment description: Adapt address drag/drop to new scheme after bug 860857 (v3a). → Adapt address drag/drop to new scheme after bug 860857 (v3a) [checkin in with mods, comment #32].
Attachment #8748700 - Flags: review?(acelists)
Attachment #8748700 - Flags: review+
Attachment #8748700 - Flags: approval-comm-aurora+
(Assignee)

Updated

11 months ago
Attachment #8748700 - Attachment description: Adapt address drag/drop to new scheme after bug 860857 (v3a) [checkin in with mods, comment #32]. → Adapt address drag/drop to new scheme after bug 860857 (v3a) [checked in with mods, comment #32].
(Assignee)

Comment 34

11 months ago
Aurora (TB 48)
https://hg.mozilla.org/releases/comm-aurora/rev/d27e0d778376
status-thunderbird48: affected → fixed

Comment 35

11 months ago
Comment on attachment 8748700 [details] [diff] [review]
Adapt address drag/drop to new scheme after bug 860857 (v3a) [checked in with mods, comment #32].

> +    // The data contains the "selected rows" in a comma-separated way.
> +    var rows = dataTransfer.getData("moz/abcard");
> +
> +    for (var j = 0; j < rows.length; j++)
I think you can use  |for (let row of rows)| here.

> +    // The data contains the a string of "selected rows", eg.: "1,2".
> +    var rows = dataTransfer.getData("moz/abcard").split(",").map(j => parseInt(j, 10));
You can coerce a string to an integer number like "123"|0 so perhaps j|0

> +    let cardsToCopy = [];
> +    for (let j = 0; j < numrows; j++) {
> +      cardsToCopy.push(gAbView.getCardFromRow(rows[j]));

I think you can avoid a loop here by doing this as:

let cardsToCopy = rows.map(k => gAbView.getCardFromRow(k))
Attachment #8748700 - Flags: review+
(Assignee)

Comment 36

11 months ago
Landed already ;-( [RattyAway twaps JorgK with a wet trout].
All the things you're suggesting refer to old code I didn't touch.

Be happy, you got SM fixed for free.

Comment 37

11 months ago
(In reply to Jorg K (GMT+2) from comment #36)
> Landed already ;-( [RattyAway twaps JorgK with a wet trout].
> All the things you're suggesting refer to old code I didn't touch.
My apologies.

> Be happy, you got SM fixed for free.
Thank you.
(Reporter)

Comment 38

10 months ago
Contact Drag & Drop not Working in 49.0a1 = 64Bit
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 39

10 months ago
This bug is done and the patch has landed.
As far as I can see, the drag and drop between address books is working in the very latest Daily 49.0a1 (2016-05-30).

It is also working in the 64bit version which I have just tried.

One thing to look out for:
When viewing "All address books", moving an address will apparently do nothing, since it just moves from one address book to another but maintains its position in the "All address books" view. To see that it works, switch on the "Address Book" column or don't use the "All address books" view.

If there is still a problem, please report another bug with exact steps to reproduce (STR).
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago10 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.