1584026 - Thunderbird 68 cannot import CSV address book on first attempt

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[EarlgreyTea]

Attachment: Sample Contacts.csv

A report was posted on the MozillaZine.jp forum that CSV exported from Windows Live Mail contacts cannot be imported into the Thunderbird's Addressbook.

I noticed that one of the CSV field names exported from Windows Live Mail is "電子メール アドレス". So I tried importing a CSV with field spaces removed into Thunderbird and it was successful.

Steps to reproduce:

1. Download Sample Contacts.csv.
2. Open Thunderbird's Address Book.
3. Select Tools > Import... > Addrerss Books > Text file (LDIF, .tab, .csv, .txt)
4. Open Sample Contacts.csv to import it.

Actual results:

No contacts are imported(See Addressbook(TB68).jpg).

Expected results:

Like Thunderbird 52 (See Addressbook(TB52).jpg), 5 contacts are imported.

Regression Infomation:

The result of executing mozregression-gui is as follows

• last good
  build_url: https://archive.mozilla.org/pub/thunderbird/nightly/2018/07/2018-07-18-10-53-45-comm-central/thunderbird-63.0a1.en-US.win64.zip
  changeset: 8dab948a10f073a46f13f55f94d1f6514c7360ac
  pushlog_url: https://hg.mozilla.org/comm-central/pushloghtml?fromchange=8dab948a10f073a46f13f55f94d1f6514c7360ac&tochange=4f12d77b4f9b6adaf06615c1c8cdc14de836dc1a

• first bad
  build_url: https://archive.mozilla.org/pub/thunderbird/nightly/2018/07/2018-07-20-10-01-33-comm-central/thunderbird-63.0a1.en-US.win64.zip
  changeset: 4f12d77b4f9b6adaf06615c1c8cdc14de836dc1a
  pushlog_url: https://hg.mozilla.org/comm-central/pushloghtml?fromchange=547144f5596c1a146b208d68d93950a6313080ca&tochange=4f12d77b4f9b6adaf06615c1c8cdc14de836dc1a

Comment 1

[EarlgreyTea]

Attachment: Addressbook(TB68).jpg

Comment 2

[EarlgreyTea]

Attachment: Addressbook(TB52).jpg

Comment 3

[Jorg K (CEST = GMT+2)]

Right, I tried

First Name,Last Name,E-mail Address
Anita,Jorgensen,[e-mail]
Anne-Mette,Olesen,[e-mail]
Dorena,Paschke,[e-mail]
Kemal,Celik,[e-mail]
Shiori,Inoue,[e-mail]
and nothing gets imported.

But that input data is wrong. The "E-mail Address" contains a space, as the bug title says. It works OK with First Name,Last Name,E-mailAddress.

I don't think we want to invest too much work here. The links in comment #0 don't work.

Alice, can you please give us the correct range and then we'll take a look.

Comment 4

[Alice0775 White]

What is problem?
I can import as expected using attached Sample Contacts.csv on Tb71.0a1(20190923003205) Windows10.

Comment 5

[Jorg K (CEST = GMT+2)]

Interesting. I can't, it doesn't work at all, nothing is imported. I was trying an en-US build of TB 68.1.1.

Comment 6

[Alice0775 White]

(In reply to Alice0775 White from comment #4)

What is problem?
I can import as expected using attached Sample Contacts.csv on Tb71.0a1(20190923003205) Windows10.

Wait.

Aha, I can reproduce with new profile.
However, After delete the failed Address Book("Sample Contacts"). And try again, it works as expected. So It seems to be a bug of some initialization function(i.e, the initial state of checkbox in the dialog isn't being set properly).

Anyway, I will continue to investigate regression range.

Comment 7

[Alice0775 White]

Regression window(from local cached mozregression):
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=48373a3601ff8eab496b2a744af7aaa1ee0cd9dc&tochange=a7d0257d9621817b6e6fc3094ebeec93097bc44d
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1174739835690f7f4024a913794e5cf35d1524a2&tochange=5a8107262015714d2907a85abc24c847ad9b32d2

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks, Alice. I don't see anything specific in that range. Looks like we have to debug it. But since it works without the space in the field name, there's not a lot of pressure, I assume.

Comment 9

[Alice0775 White]

Interesting,
Import AB first time, Initial state of checkboxes are all on. But if turning off all checkboxes, the import AB succeeds. --- so, it seems to indicate reverse state of the checkbox.

(In reply to Jorg K (GMT+2) from comment #8)

Thanks, Alice. I don't see anything specific in that range. Looks like we have to debug it. But since it works without the space in the field name, there's not a lot of pressure, I assume.

I think the space is not related to this. I tried dale the space in CSV. But same thing happens.

Comment 10

[Jorg K (CEST = GMT+2)]

Import AB first time, Initial state of checkboxes are all on. But if turning off all checkboxes, the import AB succeeds.

OK, this makes more sense. I also saw strange checkbox behaviour on the first import. So this is related to the listbox/richlistbox changes in bug 1475817. Oh joy, more de-XBL or de-XUL bustage:
a7d0257d9621817b6e6fc3094ebeec93097bc44d Geoff Lankow — Bug 1475817 - Part 11: Convert simple <listbox> to <richlistbox> in mailnews/import. r=Paenglab,jorgk

Magnus, who would be a good resource for this?

Comment 11

[Alice0775 White]

FYI, it works as expected if add a line as follows;

--- a/mailnews/import/content/fieldMapImport.js	2010-01-01 00:00:00.000000000 +0900
+++ b/mailnews/import/content/fieldMapImport.js	2019-09-26 22:37:54.353751400 +0900
@@ -93,16 +93,17 @@
   var checkboxCell = document.createXULElement("hbox");
   checkboxCell.setAttribute("style", "width: var(--column1width)");
   let checkbox = document.createXULElement("checkbox");
   checkbox.addEventListener("click", cellClicked);
   if (on) {
     checkbox.setAttribute("checked", "true");
   }
   checkboxCell.appendChild(checkbox);
+  checkboxCell.setAttribute("checked", on);
 
   var firstCell = document.createXULElement("label");
   firstCell.setAttribute("style", "width: var(--column2width)");
   firstCell.setAttribute("value", name);
 
   var secondCell = document.createXULElement("label");
   secondCell.setAttribute("class", "importsampledata");
   secondCell.setAttribute("flex", "1");

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks, Alice. I'll take a look later. I don't think that setting "checked" on the checkboxCell, which is a hbox, is correct. But the solution will be close to the code you pointed out.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1584026-fix-AB-import.patch

I'll let you review this. Surely seeing a listbox will make you all nostalgic ;-)
https://hg.mozilla.org/comm-central/rev/a7d0257d9621817b6e6fc3094ebeec93097bc44d#l2.21

When you do the import, the windows isn't refreshed with the imported content, you need to select another AB and then the imported one. That's for another bug.

You can land the patch if you want (assuming r+).

Comment 14

[Alice0775 White]

(In reply to Jorg K (GMT+2) from comment #12)

Thanks, Alice. I'll take a look later. I don't think that setting "checked" on the checkboxCell, which is a hbox, is correct. But the solution will be close to the code you pointed out.

umm, something tricky,
gListbox.selectedItem.firstChild / gListbox.getItemAtIndex(i).firstChild is pointing at <hbox style="width: var(--column1width)">

and finally the check status is gListbox.getItemAtIndex(i).firstChild.getAttribute("checked") in FieldImportOKButton method.

Comment 15

[Jorg K (CEST = GMT+2)]

You're saying that let on = gListbox.getItemAtIndex(i).firstChild.getAttribute("checked"); isn't right since is a hbox and not the checkbox?

I'm really not a front-end expert. I'll let Geoff deal with it. Does my patch not work for you?

Geoff, feel free to r- and come up with a better patch. I'm just looking at regressions and trying to get them fixes as well as possible.

Comment 16

[Geoff Lankow (:darktrojan)]

Comment on attachment 9096187 [details] [diff] [review]
1584026-fix-AB-import.patch

This wouldn't achieve anything. :-) `checkbox` is already unchecked on account of being freshly created.

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: 1584026-field-map-checkboxes-1.diff

This uses the result of the actual checkbox, not the cell containing it. I've also killed off the listener for toggling the checkboxes, since they are capable of doing that themselves.

Comment 18

[Jorg K (CEST = GMT+2)]

This wouldn't achieve anything. :-) checkbox is already unchecked on account of being freshly created.

Hmm, well, the observed behaviour was that on first run all boxes were checked and the patch fixed that. But that may be coincidental ;-)

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9096798 [details] [diff] [review]
1584026-field-map-checkboxes-1.diff

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

LGTM, r=mkmelin

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9096798 [details] [diff] [review]
1584026-field-map-checkboxes-1.diff

Sorry, but this doesn't work (unlike my patch). On first input with the sample AB attached here, you now have the first three rows checked instead of all rows without the patch. Nothing is imported. Works OK the second time.

Comment 21

[Jorg K (CEST = GMT+2)]

Did anyone test this?

Comment 22

[Jorg K (CEST = GMT+2)]

Uff, forgot to build :-(

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 9096798 [details] [diff] [review]
1584026-field-map-checkboxes-1.diff

Building with the patch helped ;-) - Sorry about the noise.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ba107919a387
Fix broken field map dialog checkboxes for address book import. r=mkmelin DONTBUILD

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5dea3f78c652
Follow-up: Fix linting. rs=white-space-only DONTBUILD

Comment 26

[Jorg K (CEST = GMT+2)]

TB 70 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/21607b2ee300199a8d2474f2f507b2b19908b875 (merged patch)

Comment 27

[Jorg K (CEST = GMT+2)]

TB 68.1.2 or TB 68.2:
https://hg.mozilla.org/releases/comm-esr68/rev/42ed08c599dfb41a076d21e5d2cd37023b9e99cd