Closed Bug 1356881 Opened 4 years ago Closed 3 years ago

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

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird_esr5254+ fixed, thunderbird53 wontfix, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 54+ fixed
thunderbird53 --- wontfix
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: bug31, Assigned: aceman)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

8.06 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
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
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.
Flags: needinfo?(richard.marti)
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.
Flags: needinfo?(richard.marti)
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.
I tried it but the Mac AB doesn't show in TB. In Security I allowed it for TB but still not showing.
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.
Blocks: 1296535, 431217
Summary: mailing to a MAC contacts distribution list leaves Send option grayed out → mailing to a MAC contacts group list leaves Send option grayed out
Duplicate of this bug: 1358411
People start complaining about this, we need to do something about it.

Let's re-establish the previous behaviour for Mac.
Flags: needinfo?(acelists)
See Also: → 1358625
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.
No longer blocks: 1358627
Duplicate of this bug: 1358627
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?
Flags: needinfo?(sancus)
Flags: needinfo?(richard.marti)
Flags: needinfo?(acelists)
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.
Flags: needinfo?(richard.marti)
I meant, the autocomplete with the TB ABs works.
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?
http://i.imgur.com/VZUDvdB.png

Here is what my ldap_2 prefs say.
Flags: needinfo?(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.
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
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?
Flags: needinfo?(richard.marti)
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.
Flags: needinfo?(richard.marti)
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).
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.
(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) ?
Attached patch patch (obsolete) — Splinter Review
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?
Assignee: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8864233 - Flags: review?(richard.marti)
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?
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?
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.
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.
Blocks: 1290733
(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.
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.
(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 on attachment 8864233 [details] [diff] [review]
patch

This works for me.
Attachment #8864233 - Flags: review?(richard.marti) → feedback+
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 5?
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 :-(
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.
I tried to avoid installing Outlook on my development machine. Looks like I failed :-(
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.
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 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.
Attachment #8864233 - Flags: review+
(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.
Attached patch patch v1.1Splinter Review
Attachment #8864233 - Attachment is obsolete: true
Attachment #8864387 - Flags: review?(jorgk)
Comment on attachment 8864387 [details] [diff] [review]
patch v1.1

Looks good to me, I've only checked the interdiff now.
Attachment #8864387 - Flags: review?(jorgk) → review+
Thanks.
Component: Untriaged → Address Book
Keywords: checkin-needed
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Target Milestone: --- → Thunderbird 55.0
Version: 52 Branch → 52
https://hg.mozilla.org/comm-central/rev/34153f64820674523909b789c68f58e8f0c1af01
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8864387 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Regression caused by (bug #): Bug 1296535.
Attachment #8864387 - Flags: approval-comm-esr52?
Attachment #8864387 - Flags: approval-comm-beta+
Tested today Daily on macOS and the send button is activated with entering the group.
Duplicate of this bug: 1363068
(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.
Tried the try build. Still the same problem, the group isn't shown in AB and does also not work in composer. :-(
Thanks.

Do we file a bug that the build servers on try-comm-central differ from comm-central in some configuration?
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?
It has access. Normal contacts in this AB are accessible by TB.
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?
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.
Duplicate of this bug: 1366380
Duplicate of this bug: 1366525
Attachment #8864387 - Flags: approval-comm-esr52? → approval-comm-esr52+
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.
You need to log in before you can comment on or make changes to this bug.