1356881 - mailing to a MAC contacts group list leaves Send option grayed out

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Tyler]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170302120751

Steps to reproduce:

Write new message, Enter Mac Contacts distribution list name, Enter any subject. Try to Send message. Cannot because Send option remains greyed out. 


Actual results:

Cannot email message


Expected results:

Send button should no longer be greyed out.
Note: you can get around this by BCC to any email address.
Note2: Issue started after upgrading from v48 to v52

Comment 1

[Jorg K (CEST = GMT+2)]

What's a "Mac Contacts distribution list"? Is this a normal mailing list configured in the address book or some Mac-specific external "distribution list"?

We changed the way the send button is enabled. It needs to contain a "semi-valid" e-mail address, meaning something with a @ in it, or the name of a mailing list, which won't have the @.

Should there be an external Mac-specific mechanism, then we broke that.

Richard, can you try this for me.

Comment 2

[Richard Marti (:Paenglab)]

I also don't know what a "Mac Contacts distribution list" is. Here the Mac AB calls it Group. I never used the Mac AB but when I use a TB mailing list, the send button is enabled after choosing a list.

Comment 3

[Jorg K (CEST = GMT+2)]

Thanks, but can you try with a Mac AB and create a group. We have some integration with the Mac address book here:
mailnews/addrbook/src/nsAbOSXCard.mm which also contains code for groups.

So I can imagine that the new "ready to send" check is overzealous and doesn't realise that the string the user entered is a Mac-specific group name.

Before we changed it, the Send button was enabled when the field wasn't empty, no further checking done.

Comment 4

[Richard Marti (:Paenglab)]

I tried it but the Mac AB doesn't show in TB. In Security I allowed it for TB but still not showing.

Comment 5

[:aceman]

It is strange that this would appear only now between 48-52 as the disabling of Send button wasimplemented in bug 431217 for TB 23. Since then we are tweaking it to properly enable for various ways of adding recipients and various recipient types. Last time was in bug 1296535 where we added a mailinglist name. So maybe the Mac list/group also needs an exception.

The current check is like this:
    (isValidAddress(inputValue) ||
         MailServices.ab.mailListNameExists(inputValue.replace(/ *<.*>/, "")))) ||
        ((popupValue == "addr_newsgroups") && (inputValue != "")))

The description for mailListNameExists() says it checks mork/mdb based addressbooks. So maybe the OS X one is a different case.

Comment 7

[Jorg K (CEST = GMT+2)]

People start complaining about this, we need to do something about it.

Let's re-establish the previous behaviour for Mac.

Comment 8

[llmll]

Oh sorry for the "distribution", it slipped in for unknown reasons. I meant a contact group from macOS address book. You type the first letters of the group's name, it gets auto-completed by Thunderbird.

Then apparently the validity check sees no semi-valid-email address.

Comment 10

[:aceman]

I have prepared a testing build on try (I do not have a Mac):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e28882585449cfd38ee93b7daf8209ca8dce8401

Paenglab, sancus, can you please try using the OS X AB group with the build?

Comment 11

[Richard Marti (:Paenglab)]

Sorry, I can't make TB uses the system AB (the apps in Security are ticked). Normal contacts in it (only two as I never used it) are also not shown and the autocomplete always works.

If somebody can show me how to enable it, that would be great.

Comment 12

[Richard Marti (:Paenglab)]

I meant, the autocomplete with the TB ABs works.

Comment 13

[Richard Marti (:Paenglab)]

Now it works.

With my build in Our AB the system AB group is shown and the autocomplete works. With the test build it's not shown and then naturally also the autocomplete is not working. So for me my build is working.

Sancus can you check what the ldap_2.servers.osx.* prefs show?

Comment 14

[Andrei Hajdukewycz [:sancus]]

http://i.imgur.com/VZUDvdB.png

Here is what my ldap_2 prefs say.

Comment 15

[Andrei Hajdukewycz [:sancus]]

With this build, (In reply to :aceman from comment #10)
> I have prepared a testing build on try (I do not have a Mac):
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=e28882585449cfd38ee93b7daf8209ca8dce8401
> 
> Paenglab, sancus, can you please try using the OS X AB group with the build?

With this build, I don't see the JS errors in the error console anymore, but the OSX address book doesn't seem to work at all, I can't even access it by checking the TB address book. When I start this build, it tells me Daily.app wants to access my contacts and I click OK, but then nothing works.

I should point out that on earlybird 54.0a2 (2017-04-23) everything works correctly IF you restart Thunderbird AFTER adding the group in your Mac Contacts.app. I only see any of these problems if I try to make a group in Contacts.app and then use it in Thunderbird. Doing that breaks all of Thunderbird's interactions with the Mac Address Book. But if I restart Thunderbird, then I can mail to the group and autocomplete works too.

Comment 16

[Andrei Hajdukewycz [:sancus]]

Alright I misunderstood what was going on here because there's two different levels of failure.

If I make a group in Contacts.app while Thunderbird is open, that breaks Thunderbird's ability to read both contacts and groups from the Mac Address Book.

If I restart, however, groups seem to be recognized and Thunderbird can pull contacts from the Mac Address Book, however, you get into this state, where the group appears to be recognized but the Send button is still greyed out: http://i.imgur.com/NWf4rdY.png

Comment 17

[:aceman]

I have a new patch up at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b035615ce08aa80aa9889be77b9814c1ea00e703, could you please test it if the Send button still works?

Comment 18

[Richard Marti (:Paenglab)]

Again, the try build doesn't work (shows no group and can't therefore not enable the send button) but the self built does with your patch. The normal official Daily shows the group but the send button doesn't activate. So something strange with the try builds.

Comment 19

[:aceman]

That's great, thanks. (but I wonder what's wrong with the try builds).

Now while we are here, there may be the same problem with Outlook addressbook. It seems TB connects to it too, maybe similarly to the OS X addressbook. Can anybody try that out? Probably needs a Windows machine with Outlook installed and addressbook with some test contacts (and a mailinglist).

Comment 20

[Jorg K (CEST = GMT+2)]

For the record: TB access to the Outlook address book is documented here:
http://kb.mozillazine.org/Using_Outlook_and_OE_contacts_with_Thunderbird_or_Mozilla_Mail
That works in TB 45 and TB 52. So whatever broke Mac didn't break Windows.
I'd be curious to know where the Mac part got broken.

Comment 21

[:aceman]

(In reply to Jorg K (GMT+2) from comment #20)
> That works in TB 45 and TB 52. So whatever broke Mac didn't break Windows.
> I'd be curious to know where the Mac part got broken.

We check if a mailing list exists via nsAbManager::MailListNameExists(). And this is only implemented for MDB based addressbooks (descendants of nsIAbMDBDirectory). OS X AB does not inherit from it and I think Outlook doesn't either. So I wonder how Outlook still works (maybe it is shadowed with some internal MDB database?). Maybe you can check for some C++ voodoo in the files (mailnews/addrbook/src) ?

Comment 22

[:aceman]

Attachment: patch

This is my latest version. Based on previous comment, this patch implements nsAbManager::MailListNameExists for all addressbook types via a new hasMailListWithName() function on nsIAbDirectory. I supply a generic implementation in the parent class and MDB overrides it with the existing implementation via nsIAddrDatabase->FindMailListbyUnicodeName(). The generic implementation should cover OS X and Outlook and should properly bail out on ABs that do not have mailinglists (e.g. LDAP).

Paenglab, can you please check if this version still works for OS X AB?

Comment 23

[Jorg K (CEST = GMT+2)]

Well, as I said in comment #20: Accessing the Outlook address book works via some LDAP trick. The Mac address book as well?

I'd like to know what broke TB for Mac. Richard, can yo try in TB 45 on Mac whether this gives you access to groups. Also, I don't understand comment #13. What is "my" build and "test" build?

If this worked once, I don't understand why we need a whole heap of new code to get it working again. Comment #14 also suggests that the Mac stuff is done via LDAP just like Windows. So what changed there?

Comment 24

[Richard Marti (:Paenglab)]

TB 45 and 52 show the group, but 52 doesn't enable the send button when entering the group name. TB 45 does.

Jörg, you tested if TB enables the send button when entering the group name from Outlook?

Comment 25

[Jorg K (CEST = GMT+2)]

Yes, I did. So I really don't understand what's happened here and I don't understand why such a large patch is needed.

Comment 26

[:aceman]

I think we broke this in bug 1290733. We changed from enabling Send button on anything typed, to enable when something with @ was typed. That broke ALL mailinglists. We partially fixed it in bug 1296535, but that didn't cover OS X AB.

Comment 27

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #26)
> We partially fixed it in bug 1296535, but that didn't cover OS X AB.
So you're saying the
  MailServices.ab.mailListNameExists()
doesn't work for Mac but it works for Windows?
I double-checked, typing the mailing list name from Outlook auto-completes and enables the Send button.

So why not just fix MailServices.ab.mailListNameExists() for Mac instead of introducing a new scheme here with hasMailListWithName(), etc.

Comment 28

[:aceman]

It is a bit of code because it adds new functionality, implements the comment "// XXX Make this not MDB specific.". Then the feature is used to fix the bug.

Comment 29

[:aceman]

(In reply to Jorg K (GMT+2) from comment #27)
> (In reply to :aceman from comment #26)
> > We partially fixed it in bug 1296535, but that didn't cover OS X AB.
> So you're saying the
>   MailServices.ab.mailListNameExists()
> doesn't work for Mac but it works for Windows?

No. It only works for the internal TB addressbooks. Does not work for OS X AB. I don't know why it seems to work for Outlook (not Windows).

> I double-checked, typing the mailing list name from Outlook auto-completes
> and enables the Send button.
> 
> So why not just fix MailServices.ab.mailListNameExists() for Mac instead of

Ok, how to fix it?

> introducing a new scheme here with hasMailListWithName(), etc.

I fix it by implementing the missing functionality referenced by the comment.

Comment 30

[Richard Marti (:Paenglab)]

Comment on attachment 8864233 [details] [diff] [review]
patch

This works for me.

Comment 31

[Jorg K (CEST = GMT+2)]

OK, I've read up on it a bit. Looks like there is merit in the "new scheme" implementing
  // XXX Make this not MDB specific.
So if MailServices.ab.mailListNameExists() only worked for MDB (Mork DB, internal address book, I assume), how was it done on Windows, that is, how does the test shown in comment #5 pass? Mystery?

Comment 32

[:aceman]

Comment 5?

Comment 33

[Jorg K (CEST = GMT+2)]

No, comment #5 doesn't enlighten me. No word about Windows. Unless the LDAP stuff simulates a mork/mdb address book. I'll install Outlook and debug it myself :-(

Comment 34

[:aceman]

True LDAP seems not to support mailinglists so it shouldn't help here by simulating anything.

Yes, I don't know how it happens that you can use Outlook mailinglist and Send gets enabled. That is a mystery to me. I said that long ago :) It would be great if you could debug it on Windows. Thanks.

Comment 35

[Jorg K (CEST = GMT+2)]

I tried to avoid installing Outlook on my development machine. Looks like I failed :-(

Comment 36

[Jorg K (CEST = GMT+2)]

OK, I messed this up completely :-(

The Outlook address book when configured as per
http://kb.mozillazine.org/Using_Outlook_and_OE_contacts_with_Thunderbird_or_Mozilla_Mail
shows in the TB AB as "OP Contacts".
Using a mailing list from that address book does *NOT* enable the send button.
The address book also does *NOT* autocomplete.

I got confused since I also tested with the so-called "Windows Mail" application on Vista. That places contacts and groups/lists into C:\Users\<user>\Contacts. I must have imported those into a TB native address book, hence the autocomplete and the mailing list enabling the Send button.

Sorry about all my silly comments. I'll test the patch now re. Outlook.

Comment 37

[Jorg K (CEST = GMT+2)]

Hmm, for the testing above I used TB 52 (32bit). With my Daily (64bit) I get an error when I try opening the address book to check, grr.

Either there is no default mail client or the current mail client cannot fulfill the messaging request. Please run Microsoft Outlook and set it as the default mail client.

So I checked the try build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=16e7affc0ddf77ca629b2246a2a8c137f9fefea3

Yes! Using a mailing list from "OP Contacts" now enables the Send button. And the mailing list resolution also works when sending the message.

Sorry about all my ignorant comments and all the confusion. So the patch improves the selection of "foreign" mailing lists on both Mac and Windows (although the Outlook setup is really obscure).

Great job, Aceman!

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864233 [details] [diff] [review]
patch

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

I had to find one nit ;-)

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +498,5 @@
> +
> +      rv = directory->HasMailListWithName(aName, aExists);
> +      if (NS_SUCCEEDED(rv) && *aExists)
> +        return NS_OK;
> +    } while (false); // error checking

Nit: Not really sure what the comment tells me.

Comment 39

[:aceman]

(In reply to Jorg K (GMT+2) from comment #38)
> > +    } while (false); // error checking
> 
> Nit: Not really sure what the comment tells me.

Leftover from a more complicated condition cascade :) Now it is unneeded.

Comment 40

[:aceman]

Attachment: patch v1.1

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864387 [details] [diff] [review]
patch v1.1

Looks good to me, I've only checked the interdiff now.

Comment 42

[:aceman]

Thanks.

Comment 43

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/34153f64820674523909b789c68f58e8f0c1af01

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864387 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Regression caused by (bug #): Bug 1296535.

Comment 45

[Jorg K (CEST = GMT+2)]

Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/f46aa8903207e4f78a9ab3d6405b289d77c5f404

Comment 46

[Richard Marti (:Paenglab)]

Tested today Daily on macOS and the send button is activated with entering the group.

Comment 48

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #46)
> Tested today Daily on macOS and the send button is activated with entering
> the group.

Thanks for checking this. Could please also recheck with a try build (e.g. once https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a3788cf18d83b33d7ac9cd93f5b009c79980dac0 finishes) to see if a build made on try is working too? In the tests till now a try build wasn't working even with the patch.

Comment 49

[Richard Marti (:Paenglab)]

Tried the try build. Still the same problem, the group isn't shown in AB and does also not work in composer. :-(

Comment 50

[:aceman]

Thanks.

Do we file a bug that the build servers on try-comm-central differ from comm-central in some configuration?

Comment 51

[Jorg K (CEST = GMT+2)]

Hmm, that's a little hard to believe, both appear to be build on Mac OS X 10.7 (Lion) which according to the latest release notes we don't even support any more. Could it be that the try binary is not authorised to access the Mac address book?

Comment 52

[Richard Marti (:Paenglab)]

It has access. Normal contacts in this AB are accessible by TB.

Comment 53

[:aceman]

So with that build from try, you can see the OS X AB in TB and read the contacts, just that using a group in composition does not enable Send?

Comment 54

[Richard Marti (:Paenglab)]

I see and can use the macOS AB contacts but don't see it's group and hence can't enter it's name into the composer fields.

Comment 57

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/13cbad8926f8c0866fce33088dd358b92f5e9cf0

Note that there were changes to an IDL file here. I guess they were OK to uplift since we don't support binary extensions any more. To get this landed, I had to add ba= and IGNORE IDL to the commit message.