Closed Bug 1584026 Opened 5 months ago Closed 5 months ago

Thunderbird 68 cannot import CSV address book on first attempt

Categories

(Thunderbird :: Address Book, defect)

defect
Not set

Tracking

(thunderbird_esr6870+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: earlgreypicard, Assigned: darktrojan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files)

Attached file 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

Attached image Addressbook(TB68).jpg
Attached image Addressbook(TB52).jpg
Keywords: regression
Version: unspecified → 68

Right, I tried

First Name,Last Name,E-mail Address
Anita,Jorgensen,Anita@contoso.com
Anne-Mette,Olesen,Anne-Mette@contoso.com
Dorena,Paschke,DorenaP@contoso.com
Kemal,Celik,Kemal@contoso.com
Shiori,Inoue,Shiori@contoso.com
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.

Flags: needinfo?(alice0775)
Status: UNCONFIRMED → NEW
Ever confirmed: true

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

Flags: needinfo?(alice0775) → needinfo?(jorgk)

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.

Flags: needinfo?(jorgk)

(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.

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.

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.

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?

Flags: needinfo?(mkmelin+mozilla)
Regressed by: 1475817
Summary: Thunderbird 68 cannot import CSV with field names that contain spaces → Thunderbird 68 cannot import CSV address book on first attempt

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");

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.

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+).

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #9096187 - Flags: review?(geoff)

(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.

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 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.
Attachment #9096187 - Flags: review?(geoff) → review-

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.

Assignee: jorgk → geoff
Attachment #9096798 - Flags: review?(mkmelin+mozilla)
Attachment #9096798 - Flags: approval-comm-esr68?
Attachment #9096798 - Flags: approval-comm-beta?

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 on attachment 9096798 [details] [diff] [review]
1584026-field-map-checkboxes-1.diff

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

LGTM, r=mkmelin
Attachment #9096798 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #9096798 - Flags: feedback-
Attachment #9096798 - Flags: approval-comm-esr68?
Attachment #9096798 - Flags: approval-comm-beta?

Did anyone test this?

Flags: needinfo?(geoff)

Uff, forgot to build :-(

Flags: needinfo?(geoff)
Comment on attachment 9096798 [details] [diff] [review]
1584026-field-map-checkboxes-1.diff

Building with the patch helped ;-) - Sorry about the noise.
Attachment #9096798 - Flags: feedback-
Attachment #9096798 - Flags: feedback+
Attachment #9096798 - Flags: approval-comm-esr68+
Attachment #9096798 - Flags: approval-comm-beta+

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5dea3f78c652
Follow-up: Fix linting. rs=white-space-only DONTBUILD
You need to log in before you can comment on or make changes to this bug.