1653647 - Edit vCard dialog does not have a title when creating new vCard

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Thomas D. (:thomas8)]

Attachment: Screenshot1: Edit vCard dialog without title

Seen on 79.0b1 (32-bit), Win10

STR:

• Account Settings > Edit Card

Actual:

• The vCard dialog does not have a title when empty (first use; will never be empty again as TB doesn't allow me to empty an existing vCard, which feels wrong, too).

Expected:

• Button label should be Edit vCard
• Virgin vCard dialog (when creating a new vCard) should have title: Edit vCard
• As with AB contacts, as user enters names, title should change dynamically to Edit vCard for {DisplayName}

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=16595a198a9070026821db5b25be53592f96f01e&tochange=299e88e21913bdcf79169bd0d84197ce32b1df07
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=855249e545c361516a65bcba8f5bc6b423e2d131&tochange=005ef1c2599217538ed001683d210bd4f2776311

Comment 2

[Magnus Melin [:mkmelin]]

Seems the issue is the identity.escapedVCard element has the string value "null", which is also found in the prefs written to disk. Not sure exactly what caused it go get there.

That the value is "null" will make the card parser not happy. But there are also other brokenness in this area, apparently since a very long time we're referencing field ids we do not have.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1653647_ab_vcard.patch

Not sure what was different from before, I guess the VCardUtils.jsm checks in this patch would make a difference. But like a wrote, not sure about the root cause. And we may have to run a migration to kill off "null" in the pref values.

For the non-vcard values, Nickname is vCard thing, as well as custom fields (should be x-foo properties). Secondary email we should be able to enable as email[1] but I guess that's not quite done yet. The chat part... needs a bunch of rethinking and I guess we should weed out irrelevant/dead/non-standard protocols we do not support. Or figure out what's the best mapping in proper vcard...

Any input?

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9168431 [details] [diff] [review]
bug1653647_ab_vcard.patch

That seems reasonable. Why are you changing it to disable the fields instead of hiding them?

I think abCardToVCard and vCardToAbCard were supposed to deal with nulls properly at some point but that must've got lost along the way. Wrapping in encodeURIComponent would still produce "null" anyway so we'd still have that problem. :-/

Comment 5

[Magnus Melin [:mkmelin]]

I'd like to avoid adding more wrapping boxes, which would kind of be needed to do the hiding. When I disable, I don't need to think about the label.

Comment 6

[Wayne Mery (:wsmwk)]

If this is causing bug 1653187 then severity s3 is more appropriate.

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: bug1653647_ab_vcard.patch

Slightly updated.
It's possible my initial take on the problem was a red herring: I tried but couldn't reproduce an identity.escapedVCard null value in the prefs. I guess it was in my testing profile and caused by some testing of whatever feature during the betas. So, I think we can just ignore that unless we find it's more widely spread.

I also added an hbox for the preferDisplayName - looked bad when opened from the account settings tab otherwise, due to the fatter subdialog styling making it wrap.

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9170893 [details] [diff] [review]
bug1653647_ab_vcard.patch

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

::: mail/components/addrbook/content/abCard.js
@@ +9,5 @@
>  var { MailServices } = ChromeUtils.import(
>    "resource:///modules/MailServices.jsm"
>  );
>  
> +var kNonVcardFields = ["SecondEmail", "ChatName", "preferDisplayName"];

SecondEmail works now, so it could be removed from this list.

@@ +173,5 @@
>      abPopup.hidden = true;
>      document.getElementById("abPopupLabel").hidden = true;
>    }
>  
> +  SetCardDialogTitle(gEditCard.card ? gEditCard.card.displayName : null);

gEditCard.card is always truthy, it's set earlier in this function.

@@ +179,5 @@
>    GetCardValues(gEditCard.card, document);
>  
> +  // Focus on first or last name based on the pref of which is first.
> +  var input = document.getElementById(
> +    gEditCard && gEditCard.displayLastNameFirst ? "LastName" : "FirstName"

Again, gEditCard is always truthy.

@@ +184,2 @@
>    );
> +  input.focus();

This doesn't appear to work, when opening from the account settings. I suspect it does work, but then it's overruled, so it looks like the timeout was doing something useful after all.

That aside, I think `input.select()` would be better.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: bug1653647_ab_vcard.patch

Thanks, making sure the card was set (or rather, not unset, set to null) was done in a later stage of the patch.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f58aa47173cb
use mime encoded version of the mailbox string where needed, so that non-ASCII or commas don't cause problems. r=pmorris
https://hg.mozilla.org/comm-central/rev/f13c60c8dbf0
fix Edit vCard dialog brokenness. r=darktrojan

Comment 12

[Magnus Melin [:mkmelin]]

Only bug https://hg.mozilla.org/comm-central/rev/f13c60c8dbf0 belonged in this bug.
Will back out the other and reland with the correct bug number.

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/613d10d61acc
Backed out changeset f58aa47173cb for landing with wrong bug number: bug 1653647, should have been bug 1656278)

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9171124 [details] [diff] [review]
bug1653647_ab_vcard.patch

[Approval Request Comment]
Regression caused by (bug #): unknown
User impact if declined: Can't edit vCard to send
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): not risky, it's quite broken as is

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9171124 [details] [diff] [review]
bug1653647_ab_vcard.patch

[Triage Comment]
Approved for esr78

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/d68bd3be7680