Olaf Fraczyk Reporter
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: version 2.0.0.14 (20080421)

Thunderbird expects that every address goes in separate (To, CC etc.) field.
It is OK for 1-3 addresses, but with more it is a real pain.
Almost every other software supports putting addresses in comma separated list.

Today it is possible to write such a list in Thunderbird but it is far from perfect:
1. If you hit "Enter" then the list is split in numerous "To" fields
2. As I reported in bug #440370 - LDAP autocompletion works for first address only
3. As I reported in bug #440376 - If you drag addresses from "Contacts" pane and you drop them in the same "To" field then no comma is added and mail is sent to the last recipient only - the rest is treated as recipients name.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Actual Results:  
Pain :)

Expected Results:  
To have it working like in Outlook or in Evolution or ... ;)

	Wayne Mery (:wsmwk)
	Comment 1 • 16 years ago

Olaf, I'm not defending the current implementation, but what problem do you encounter regarding one line/address?  Or are we talking strictly aesthetics?

	Olaf Fraczyk Reporter
	Comment 2 • 16 years ago

1. Habits - if people use other e-mail clients they expect that things work in similar way - please don't classify this simply as aesthetics.
For now I see the disadvantages below (the most severe for me are 2 and 5):

2. If you have more than 4 recipients you need to scroll - this is what makes me personally unhappy, in outlook you can enter more than 15 addresses (depending on address length:) per one recipient type (To, Cc, Bcc) before you need to scroll (the widget expands up to 4 rows before adding scrollbar).

Even if it didn't expand (as in Evolution (at least in 2.8) and in Thunderbird) you have plenty of space - it will fit at least 5-6 e-mail addresses. 

It is __much__ easier if you can see all the recipients at a glance. You won't miss any person to send, and what is more important - you won't send to a person that shouldn't get the mail :)

3. If you put several entries in "To" and then you change your mind and want to put some of them in "Cc" or "Bcc" it is easier to copy and paste than to change every "To,CC,Bcc" combo.

4. If you want to remove some addresses you need to select every one separately and delete. You can't select 5 addresses and delete them all. BTW. there is no way to delete unused fields, it is possible to delete only addresses.

5. If you send several messages to the same group of people (or a large portion of addresses is the same - eg. all addresses in CC). With "multi address" fields (like in Evo, Outlook) you can just select and do a copy/paste. In Thunderbird it is impossible.

To make this even harder, in Thunderbird the text in message header pane cannot be selected, so you can't select addresses from a message and put in address fields. I'll fill separate bug for it anyway.
Creating a list is not an option because they are often ad hoc lists - used for 5-6 messages. Doing Ctrl-C and Ctrl-V is much easier and quicker.

This is what hit me in 2 days.

	Wayne Mery (:wsmwk)
	Comment 3 • 16 years ago

>Almost every other software supports putting addresses in comma separated list.

Jeff, Does eudora support this?


(In reply to comment #2)
 For now I see the disadvantages below (the most severe for me are 2 and 5):
> 
> 2. If you have more than 4 recipients you need to scroll ...
> It is __much__ easier if you can see all the recipients at a glance. You won't
> miss any person to send, and what is more important - you won't send to a
> person that shouldn't get the mail :)

agree

> 3. If you put several entries in "To" and then you change your mind and want to
> put some of them in "Cc" or "Bcc" it is easier to copy and paste than to change
> every "To,CC,Bcc" combo.

yup
 
> 4. If you want to remove some addresses you need to select every one separately
> and delete. You can't select 5 addresses and delete them all. BTW. there is no
> way to delete unused fields, it is possible to delete only addresses.

sort of bug 252665.  I thought there was another bug but I'm not finding it.
 
> 5. If you send several messages to the same group of people (or a large portion
> of addresses is the same - eg. all addresses in CC). With "multi address"
> fields (like in Evo, Outlook) you can just select and do a copy/paste. In
> Thunderbird it is impossible.

Or save as template. Or view source + copy/paste.  Or edit as new.  Lots of workarounds

moding summary to better fit description

	Jeff Beckley
	Comment 4 • 16 years ago

(In reply to comment #3)
> >Almost every other software supports putting addresses in comma separated list.
> 
> Jeff, Does eudora support this?

Yes, it does.  This is very standard email practice since in transit the addresses are comma-separated lists.  People are used to seeing them this way when viewing a message, so it becomes natural to allow this in composition as well.

	Olaf Fraczyk Reporter
	Comment 5 • 16 years ago

(In reply to comment #3)
> Or save as template. Or view source + copy/paste.  Or edit as new.  Lots of
> workarounds
1. I saved as template. But where is it saved? How do I create new message from template? - OK - another bug - I assume that Templates should be a folder in "Local Folders". Unfortunately I don't have Templates there. And attempt to creating one tells me that such folder already exists :)
2. View source - the fields are quoted-printable or base64 encoded - no way
3. Edit as new - thanks - this works.

	Magnus Melin [:mkmelin]
	Comment 6 • 16 years ago

xref bug 392932

	Wayne Mery (:wsmwk)
	Comment 8 • 15 years ago

I could swear this is a dupe but I'm not finding it, so my recollection may be bad or perhaps I misinterpreted a related bug.

to add to Magnus's xref  Bug 392932 -  autocomplete multiple addresses on the same address line
 Bug 416711 -  no possibility to change delivery mode (to/cc/bcc) for multiple addresses at once 
 Bug 440410 -  When using commas to separate multiple addresses, there is no check if all of these are valid. 
 Bug 457872 -  Splitting of multiple mail addresses in the TO field is only done if Enter is pressed
 Bug 351863 -  thunderbird -remote mailto doesn't allow multiple addresses

	Karsten Düsterloh
	Comment 12 • 15 years ago

On a drive-by note: commas may appear in email addresses, so using comma as a separator isn't wise...

	Olaf Fraczyk Reporter
	Comment 13 • 15 years ago

Please read the RFC. They have to be quoted. Just like semicolons, etc. If you don't agree that they have to be quoted when typed into input field, then you are out of luck as any ASCII character is permitted.
The comma is explicite shown in RFC as address separator and all other e-mail clients I have ever seen, support using commas as address separator. Please keep this consistent and don't reinvent the wheel.

	mad.engineer
	Comment 15 • 14 years ago

I agree with Olaf. The current Message compose design of To:, Cc: fields is absolutely poor design IMHO. Every other client that I've used in the past allow multiple addresses via comma separate fields. If majority of the clients work the way it should be then they must be doing it right. That's why I dumped TB in favor of my previous client for now till this gets fixed.

	Bluelightav.org
	Comment 16 • 14 years ago

The space usage for the address field is not efficient the way TB3 uses it. I am getting some user feedback in office environments about the way TB
displays the recipients in the compose window. TB filling in each line only one
address leads to a long list vertically which is out of the viewable area and
needs quite some scrolling to manually check if one has entered all recipients.
That applies also when one wants to have a quick check if all people are
entered via a mailing list. The customer prefer the horizontal display of other
email clients more where email addresses are displayed in a row separated by a
comma. That way mailing list can be checked and easily temporarily modified not
touching the original mailing list.

Better space usage on small screens and better overview is what I wish to see :-)

	Vliegendehuiskat
	Comment 17 • 14 years ago

I agree on how Thunderbird 3 is pretty non-efficient with screen space.

It tries to be efficient because when you enter a lot of addresses you'll get to scroll or to drag the field down, which makes more lines appear.

However, when you enter only one address it still shows 4 address lines.

It would be nice if the default number of address lines becomes 1, so that you can also hide the unused address lines.

	Vliegendehuiskat
	Comment 18 • 14 years ago

I've discoverd a plugin called mailtweak that does exactly this.

http://mailtweak.mozdev.org/

It would be nice if some of its functionality would be built into Thunderbird.

	chrizilla
	Comment 19 • 14 years ago

#17 and #18 is a separate issue and should be filed as a separate request (in case the idea has not been brought up yet).
PS: I agree that it should be built into TB.

	Jason Daly
	Comment 20 • 14 years ago

I'm really quite surprised that this issue has never been addressed.  In my opinion this is Thunderbird's major weakness with regards to targeting a corporate user-base.  Corporate users will not adopt Thunderbird until users can:

- Enter all addresses horizontally on one line
- Click one of To:, Cc:, or Bcc: to open the default address book.

	Geert Poels
	Comment 22 • 14 years ago

The recipients UI works ok for a couple of recipients but basically it is flawed from an intuitive point of view.
Intuitively, you want to place each email address in a category (to, cc, bcc) and quickly see how is to be placed where. To also move or remove them when wrong.
Thunderbird UI approach fails on each of these.
Moving means cut/paste, even clicking it workes bizarre (I'd expect a single click to fully select, a second click to edit) and looking who's is which status is to be completely forgotten.
For those who'd really keep the current setup, keep the current look-and-feel as an option.

	Frank Breitling
	Comment 23 • 14 years ago

Terrible bug!

	Thomas D. (:thomas8)
	Comment 25 • 11 years ago

I wonder how hard this would actually be for someone with better coding skills & experience. We're already able to parse comma-delimited addresses. The UI needs radical change, but it will actually be simpler after that. I'd imagine that if we can autocomplete for single addresses, there must be a way to offer just what's being typed between two commas to autocomplete?

	KeithP
	Comment 26 • 11 years ago

Wow, can't believe this is still an issue. Thunderbird is on it's own with this one. This is the main reason I don't use it. I download every update to check to see if it's been implemented yet and every time I'm let down. Would love to use Thunderbird for my work email but can't due to this one very big annoying issue.

Come on, get it together guys. I see Postbox can do it, which is built on Thunderbird, so why can't you?

	Thomas D. (:thomas8)
	Comment 27 • 11 years ago

(In reply to KeithP from comment #26)
> Wow, can't believe this is still an issue. Thunderbird is on it's own with
> this one. This is the main reason I don't use it. I download every update to
> check to see if it's been implemented yet and every time I'm let down. Would
> love to use Thunderbird for my work email but can't due to this one very big
> annoying issue.

yes, it's weird (sigh...)

> Come on, get it together guys. I see Postbox can do it, which is built on
> Thunderbird, so why can't you?

Not enough manpower (now practically maintained by volunteers only), complex codebase, and too many bugs and RFEs to choose from that are equally or more annoying...

If you can come yrself or bring somebody with some coding skills in JavaScript/HTML to fix this, most welcome... :)

fwiw, are you aware that you can type or dump comma-separated addresses into one of TB's single fields, and TB will automatically resolve them into single addresses of same type?

	M. Grau
	Comment 28 • 11 years ago

@KeithP: Coming from Outlook, I had the same problem until I found this add-on: https://addons.mozilla.org/de/thunderbird/addon/mrc-compose (separate to/CC/BCC fields with comma-separated addresses and address book suggestions while typing). I have not had a long test phase with this add-on yet, but it seems to work okay so far. Should be Thunderbird default though, IMHO.

	Thomas D. (:thomas8)
	Comment 29 • 11 years ago

(In reply to M. Grau from comment #28)
> @KeithP: Coming from Outlook, I had the same problem until I found this
> add-on: https://addons.mozilla.org/de/thunderbird/addon/mrc-compose
> (separate to/CC/BCC fields with comma-separated addresses and address book
> suggestions while typing). I have not had a long test phase with this add-on
> yet, but it seems to work okay so far. Should be Thunderbird default though,
> IMHO.

Thanks M. Grau, that's helpful information. In fact, looking at the addon source code might be a good starting point for fixing this.

	KeithP
	Comment 30 • 11 years ago

@M. Grau: Great. Thanks for letting me know about the add on. It seems to do the trick. Still would like to see it as Thunderbird default though.

I have no coding skills and don't know anyone with any but hopefully someone can use the add on source code to get this implemented ASAP.

Thanks for your quick replies guys.

	michelr
	Comment 31 • 10 years ago

Hi,

I forgot to inform you that I created an addon that seems to fulfill your request :
https://addons.mozilla.org/fr/thunderbird/addon/mrc-compose/

Thanks to Andreas Wagner who implemented the LDAP support.
Since TB24, we have some small regressions, most annoying is the default focus that doesn't follow rules.
Hope it helps.
Michel

	Josiah Bruner [:jsbruner]
	Comment 32 • 10 years ago

FYI, I'm starting work on this now.

	Thomas D. (:thomas8)
	Comment 33 • 10 years ago

(In reply to Josiah Bruner [:JosiahOne] from comment #32)
> FYI, I'm starting work on this now.

Josiah, that's great news, and I think the majority of users will be truly greatful to you for normalizing and simplifying the composition header. So thanks a lot!

Btw, this has huge potential to build upon and push things somewhat further and make address handling more efficient wrt selecting, deleting, copying, changing recipient type etc. of individual addresses in composition header.

	David T. Macknet
	Comment 34 • 10 years ago

Thank you for working on this.

Two other usability issues with the scrolling: it's not always presented (no idea why - it's sometimes there, sometimes not, and I can't force it to appear when I want to add more addresses); and I've hit "reply-all" and subsequently changed my mind, after composing the reply, and found that it's much easier to cut / paste my reply than it is to go through deleting addresses.

	Dave Aton
	Comment 35 • 10 years ago

(In reply to Josiah Bruner [:JosiahOne] from comment #32)
> FYI, I'm starting work on this now.

Whew! Six years of begging and pleading, someone's finally taking it on. Thank you, Josiah, and DietySpeed!

	David E. Ross
	Comment 36 • 10 years ago

(In reply to Olaf Fraczyk from comment #2)
> 3. If you put several entries in "To" and then you change your mind and want
> to put some of them in "Cc" or "Bcc" it is easier to copy and paste than to
> change every "To,CC,Bcc" combo.

I strongly disagree.  It is easier for me to change "To:" to "CC:" than to cut-and paste.  

 
> 4. If you want to remove some addresses you need to select every one
> separately and delete. You can't select 5 addresses and delete them all.
> BTW. there is no way to delete unused fields, it is possible to delete only
> addresses.

This could instead be accomplished if MailNews allowed users to select multiple lines within the address area of the Compose window.  


> 5. If you send several messages to the same group of people (or a large
> portion of addresses is the same - eg. all addresses in CC). With "multi
> address" fields (like in Evo, Outlook) you can just select and do a
> copy/paste. In Thunderbird it is impossible.

Why not use a mailing list.  

I happen to like the current capability and thus request any implementation of this RFE be optional with a UI to enable or disable the option.  In that case, I would not be upset if this RFE were the default option.  

Also, if bug #119977 is not implemented prior to or concurrently with this bug, please do not implement in a way that breaks the PopMailListRecipients extension.  And if bug #933472 is not implemented prior to or concurrently with this bug, please do not implement in a way that breaks the "Reply to All as Cc" extension.

	:aceman
	Comment 37 • 10 years ago

OK, are there any mockups on how do you propose to solve this? Will there be a line for each of the recipient types (like in Outlook), of which there are about 6 now?

	Josiah Bruner [:jsbruner]
	Comment 38 • 10 years ago

Not yet, mockups will be available at: http://apps.josiahbruner.com/SingleLine.html as I work on them.

(It has very basic design currently), and I update it live, so feel free to follow the design.

	Richard Leger
	Comment 39 • 10 years ago

For reference as per my related comment in another bug reported:

https://bugzilla.mozilla.org/show_bug.cgi?id=1042780#c48
"...
- Replace the current To/Bcc/Cc separate fields for each email address set by a comma (or semi-colon) list of addresses instead (as usually used in alternative email client software). With a trailing X button for each address to allow deletion/removal or a drop down menu to: delete, copy email address, edit contact, add contact, etc...
..."

	Richard Leger
	Comment 40 • 10 years ago

In addition of proposed mockup for improvement in the Compose Window layout (see attachment 8507216 [details] and bug 1042780 comment 48) as possible suggestion, if dedicated fields are created for To,Cc,Bcc (with comma or semi-colon list of recipients), the Cc and Bcc fields could be hidden by default (with default setting manageable via end-user in options possibly) with two buttons appearing below the To field to make them appear if required, something like:

   From: [           ]
     To: [           ]
         [           ]
         (Cc) (Bcc)
   [                 ] 
   [ Body text       ]
   [                 ]

If you click on (Cc) button, field would deploy (same for Bcc) and looks something like:

   From: [           ]
     To: [           ]
         [           ]
     Cc: [           ]
         [           ]
         (Bcc)
   [                 ] 
   [ Body text       ]
   [                 ]

Re-clicking on Cc: (or Bcc:) would hide it again (and disable it/clear field content?)... with application remembering the last settings... to keep it the same for all following compose windows...

Easy and intuitive to use... but that may not be easy to program and implement in Thunderbird, would option be validated and accepted... :)

	Richard Leger
	Comment 41 • 10 years ago

(In reply to David E. Ross from comment #36)
> (In reply to Olaf Fraczyk from comment #2)
> > 3. If you put several entries in "To" and then you change your mind and want
> > to put some of them in "Cc" or "Bcc" it is easier to copy and paste than to
> > change every "To,CC,Bcc" combo.
> 
> I strongly disagree.  It is easier for me to change "To:" to "CC:" than to
> cut-and paste.  

If entered typed addresses and/or entries were to become some sort of contact objects (after text parsing) in the (To,Cc,Bcc) fields on which action can be applied via a trailing drop down menu for example (as suggested in comment 39) it may be possible as one can imagine to add the following actions:
- move to Bcc
- move to Cc

and/or also allow drag and drop(s) of one or more contacts (multiple contact move would require ability to select one or more contacts for actions: delete, add to contacts, etc... (among possible others...)

	:aceman
	Comment 42 • 10 years ago

I still see no mentioning of the "Reply-To", "Newsgroup" and "Followup-To" recipient types.

	Josiah Bruner [:jsbruner]
	Comment 43 • 10 years ago

Those will be included too, but will only be visible if relevant. Reply-To may be part of the To field somehow, but I'm not sure yet.

	Thomas D. (:thomas8)
	Comment 45 • 9 years ago

Josiah, are you still working on this?

This looks like a very important feature to improve/normalize the UX of TB composition in terms of ux-consistency, ux-efficiency, etc...

We should try to land this for the next big release after TB 38.

	Josiah Bruner [:jsbruner]
	Comment 46 • 9 years ago

(In reply to Thomas D. from comment #45)
> Josiah, are you still working on this?
> 
> This looks like a very important feature to improve/normalize the UX of TB
> composition in terms of ux-consistency, ux-efficiency, etc...
> 
> We should try to land this for the next big release after TB 38.

I am extremely busy between work and school now, and my main priority these days is reviews, so I find it unlikely I will get to this any time soon.

I would like it in 38 as well, but I don't know if that's possible at this point. Someone else can take this in the meantime if they so desire. Otherwise, it'll have to wait until I have some free time. I will continue working on it whenever I get a chance.

	:aceman
	Comment 47 • 9 years ago

Thomas correctly meant NEXT release after TB 38, so TB 45. I think nobody expects to get this into 38 as there would be very short testing. And this is a critical part of TB.

	Josiah Bruner [:jsbruner]
	Comment 48 • 9 years ago

(In reply to :aceman from comment #47)
> Thomas correctly meant NEXT release after TB 38, so TB 45. I think nobody
> expects to get this into 38 as there would be very short testing. And this
> is a critical part of TB.

Ah, missed that. Regardless, everything I said still stands.

	David E. Ross
	Comment 49 • 9 years ago

Will the implementation include a user-option option to keep the current design?  Will the implementation break the PopMailListRecipients or "Reply to All as Cc" extensions?

	michelr
	Comment 51 • 9 years ago

Hi,
FYI, I already wrote an addon that does what you requested :
https://addons.mozilla.org/fr/thunderbird/addon/mrc-compose/

With the upcoming 1.8 release, there will be full LDAP support, so it'll be able to replace existing UI.
I expect the 1.8 release to be announced in few days/weeks.
Until that, you can see code :
https://github.com/michelRenon/mrc_compose

I would be happy if that addon can be integrated (fully or partly) in Thunderbird.
(Jonathan Protzenko already contacted me nearly two years ago about integrating it in TB : I just had not enough time to dig into global TB build process)

	Hardi
	Comment 53 • 9 years ago

(In reply to michelr from comment #51)
> FYI, I already wrote an addon that does what you requested :
> https://addons.mozilla.org/fr/thunderbird/addon/mrc-compose/

Great addon thank you for this michelr!

Currently drag & drop copies (instead of moving) the selected address.
if the drag & drop can work like Outlook or Gmail it will be perfect.

	Ludovic Hirlimann [:Usul]
	Comment 54 • 9 years ago

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

	Dan
	Comment 55 • 9 years ago

This extension is UNSIGNED and therefore Thunderbird warns me NOT to install it.  How do I know the compiled code is not changed from github source and maybe it reads and phones home my email contents?

	Kent James (:rkent)
	Comment 56 • 8 years ago

Thunderbird should not be warning you to not install unsigned extensions, because we decided not to require signed extensions. If you are getting an error such as you report "Thunderbird warns me NOT to install it" could we have more details?

In any case, extension signing has caused lots of issues in Firefox, so we are unlikely to support it any time soon (if ever). Also, it does not really prove "the compiled code is not changed from github source" either.

	Dan
	Comment 57 • 8 years ago

(In reply to Kent James (:rkent) from comment #56)
> Thunderbird should not be warning you to not install unsigned extensions,
> because we decided not to require signed extensions. If you are getting an
> error such as you report "Thunderbird warns me NOT to install it" could we
> have more details?
> 
> In any case, extension signing has caused lots of issues in Firefox, so we
> are unlikely to support it any time soon (if ever). Also, it does not really
> prove "the compiled code is not changed from github source" either.

OK, I installed it.

	mad.engineer
	Comment 58 • 8 years ago

(In reply to Kent James (:rkent) from comment #56)
> Thunderbird should not be warning you to not install unsigned extensions,
> because we decided not to require signed extensions. If you are getting an
> error such as you report "Thunderbird warns me NOT to install it" could we
> have more details?
> 
> In any case, extension signing has caused lots of issues in Firefox, so we
> are unlikely to support it any time soon (if ever). Also, it does not really
> prove "the compiled code is not changed from github source" either.

Long shot, but I'll ask anyway. Any chance that this MRC Compose add-on can be included in the Core TB itself? Thanks

	Kent James (:rkent)
	Comment 59 • 8 years ago

(In reply to mad.engineer from comment #58)

> Long shot, but I'll ask anyway. Any chance that this MRC Compose add-on can
> be included in the Core TB itself? Thanks

I looked at the extension code, it is quite intrusive, so it is not as simple as just incorporating it. But I don't see anyone here objecting to the concept, so the addon might be used by someone as a basis to prepare a core patch. Patches welcome, in other words.

	michelr
	Comment 60 • 8 years ago

(In reply to Dan from comment #55)
> This extension is UNSIGNED and therefore Thunderbird warns me NOT to install
> it.  How do I know the compiled code is not changed from github source and
> maybe it reads and phones home my email contents?

Hi Dan,
Just FYI, that extension has no compiled code, so you can check yourself what it does.
From the .xpi downloaded :
- change the '.xpi' to '.zip'
- extract it
- then you can see all .xml and .js files and see that it does not steal data.
It also allows you to check that what you downloaded is exactly what is in github.

Cheers

	michelr
	Comment 61 • 8 years ago

(In reply to Kent James (:rkent) from comment #59)
> (In reply to mad.engineer from comment #58)
> 
> > Long shot, but I'll ask anyway. Any chance that this MRC Compose add-on can
> > be included in the Core TB itself? Thanks
> 
> I looked at the extension code, it is quite intrusive, so it is not as
> simple as just incorporating it. But I don't see anyone here objecting to
> the concept, so the addon might be used by someone as a basis to prepare a
> core patch. Patches welcome, in other words.

Hi, I'm the developer of MRC Compose.
I'm quite honored that you think to include it in TB.
But I have to warn you that it would break compatibility with some other addons, those that work on addressing widgets. Here are some addons that incompatible : flexible-identity, account colors, BorderColors GT

The last request is to add some 'objects' or 'tokens' in the fields. But it would require a great rewrite, and trying to embed jQuery+TokenInput. So not easy at all...

But currently, I have no time anymore to work on it. The best I can do is to provide some help to people wanting to work on it.
Cheers, Michel

	David E. Ross
	Comment 62 • 8 years ago

(In reply to Kent James (:rkent) from comment #59)
> I looked at the extension code, it is quite intrusive, so it is not as
> simple as just incorporating it. But I don't see anyone here objecting to
> the concept, so the addon might be used by someone as a basis to prepare a
> core patch. Patches welcome, in other words.  

I object!  See my comment #36 and comment #49.  I prefer the current line-per-addressee design.  As I indicated before, if this RFE is implemented, it should be an option and not a replacement of the current design.

	Jim Porter (:squib)
	Comment 63 • 8 years ago

(In reply to David E. Ross from comment #62)
> I object!  See my comment #36 and comment #49.  I prefer the current
> line-per-addressee design.  As I indicated before, if this RFE is
> implemented, it should be an option and not a replacement of the current
> design.

I doubt this will be an option, since TB needs fewer options, not more. But that doesn't preclude someone (even the TB team themselves!) from producing an add-on to support this. However, I think we can address your concerns even with the new UI:

(In reply to David E. Ross from comment #36)
> I strongly disagree.  It is easier for me to change "To:" to "CC:" than to
> cut-and paste.  

A good implementation of this would be just as easy to change To->CC as before. The design I have in mind is that each address is an atomic unit (basically...*). Just drag and drop the address in question.

> > 5. If you send several messages to the same group of people (or a large
> > portion of addresses is the same - eg. all addresses in CC). With "multi
> > address" fields (like in Evo, Outlook) you can just select and do a
> > copy/paste. In Thunderbird it is impossible.
> 
> Why not use a mailing list.  

There are times where you don't do this enough to warrant making a mailing list.

* I thought I had a comment in this bug that described my plan, but it seems I do not.

	Jim Porter (:squib)
	Comment 64 • 8 years ago

The design I have in mind for this is that each address is "bubbled" like in the message reader, so you can operate on it as though it's a single element, rather than a string of characters. This would make it easy to move addresses around (individually or in bunches), which should help to maintain the benefit of the current UI while also adding new capabilities.

I could probably take a look at bug 626532 sometime soon, which would make implementing such a UX a lot easier.

	Dan
	Comment 65 • 8 years ago

I especially like how this extension shows email list recipients after adding the email list.

	M Lopez-Ibanez
	Comment 66 • 7 years ago

A very annoying issue with the current UI is that when replying to multiple recipients, the UI does not show all the recipients (one needs to scroll). Moreover, if you remove one recipient, the list does not scroll up, it just leaves an empty space. The result is that I often end up not trimming enough the recipient list. With long lists, the process of removing recipients becomes very slow (there should be a one-click check-mark like in Gmail!).

Do I need to open a new PR or is this a duplicate of this one?

	M Lopez-Ibanez
	Comment 67 • 7 years ago

(In reply to Jim Porter (:squib) from comment #64)
> The design I have in mind for this is that each address is "bubbled" like in
> the message reader, so you can operate on it as though it's a single
> element, rather than a string of characters. This would make it easy to move
> addresses around (individually or in bunches), which should help to maintain
> the benefit of the current UI while also adding new capabilities.

The UI of Thunderbird Conversations for quick replies looks like this, but it does not allow drag&drop or editing the bubbles.
https://addons.cdn.mozilla.net/user-media/previews/thumbs/51/51379.png?modified=1331218902
https://addons.mozilla.org/en-US/thunderbird/addon/gmail-conversation-view/

	M Lopez-Ibanez
	Comment 68 • 7 years ago

(In reply to M Lopez-Ibanez from comment #66)
> A very annoying issue with the current UI is that when replying to multiple
> recipients, the UI does not show all the recipients (one needs to scroll).
> Moreover, if you remove one recipient, the list does not scroll up, it just
> leaves an empty space. The result is that I often end up not trimming enough
> the recipient list. 

I just installed: https://addons.mozilla.org/fr/thunderbird/addon/mrc-compose/ and it solves this problem wonderfully.

	Marco A.G.Pinto [:marcoagpinto]
	Comment 69 • 7 years ago

(In reply to M Lopez-Ibanez from comment #68)
> (In reply to M Lopez-Ibanez from comment #66)
> > A very annoying issue with the current UI is that when replying to multiple
> > recipients, the UI does not show all the recipients (one needs to scroll).
> > Moreover, if you remove one recipient, the list does not scroll up, it just
> > leaves an empty space. The result is that I often end up not trimming enough
> > the recipient list. 
> 
> I just installed:
> https://addons.mozilla.org/fr/thunderbird/addon/mrc-compose/ and it solves
> this problem wonderfully.

I installed it too... but it doesn't work well with Enigmail.

For several months that I have been waiting for a fix without success.

With the fix, it simply can't be used together.

	Marco A.G.Pinto [:marcoagpinto]
	Comment 70 • 7 years ago

With=without*

sorry for the typo.

	Annie Wellington
	Comment 71 • 7 years ago

Please provide the promised functionality of multiple addresses in one To: field
The current idea of separated fields is a pain and very strange

	Cigar Guy
	Comment 72 • 6 years ago

This is perhaps an implementation feature, but is really a pain. I would like to see the existing functionality also enhanced by allowing multiple emails in TO , CC , BCC fields 
Example
1@email.com, 2@email.com,3@email.com,4@email.com,5@email.com..... So on, so forth.... Almost all email software offers this feature of delimitting email fields by a COMMA and SemiColon. Why is this composition window any different and making UX difficult and frustrating

	M Lopez-Ibanez
	Comment 74 • 6 years ago

(In reply to M Lopez-Ibanez from comment #68)
> I just installed:
> https://addons.mozilla.org/fr/thunderbird/addon/mrc-compose/ and it solves
> this problem wonderfully.

Thunderbird 60 broke this feature of MRC Compose. See bug 1502391.

	M Lopez-Ibanez
	Comment 76 • 6 years ago

The github of MRC Compose is here: https://github.com/michelRenon/mrc_compose

It seems TB 60 broke two things in MRC Compose:
* The TO field cannot expand to more than one line anymore. The CC and BCC fields do expand.
* When a tooltip/warning is shown, the TO field becomes invisible. It works for CC and BCC fields.

The problem seems to be some recent change to the TO field in thunderbird.

	M Lopez-Ibanez
	Comment 77 • 6 years ago

Screenshot showing the problem. The CC and BCC fields work as described in this bug. The TO field doesn't. It was working before TB 60.

	Bob
	Comment 78 • 6 years ago

Second the observation from Lopez.
Actually, MRC compose should be mainlined into Thunderbird, at least an option. That's how the interface should have been. That's how most other email editors do it. It is easier to bulk edit emails (deleting, moving them between to:/cc:, or merging different branches of the same thread). 
Thunderbird should work well for the power users as well as those who only need to enter 1 or 2 email addresses.
Best!

	Dan
	Comment 79 • 6 years ago

Second the observation from Bob. Is MRC development dead?

	Richard Leger
	Comment 80 • 6 years ago

I agree with Bob and support his comment, as a daily user of Thunderbird I also think MRC shall be make mainline into Thunderbird especially for power users... but it would need to be well integrated and intuitive for end-users... I am happy to test and provide feedback via beta channel would it be implemented...

Taking into account suggestions from this bug report to maximise flexibility such as Comment 39 or Comment 40... would be nice as well... to improve the potential feature...

Copy/Past shall also be made possible... (of one or more contacts from within TB or outside sources) as well as drag and drop between TO,CC,BCC... as user see fit best... ideally...

Thanks for listening :-)

	M Lopez-Ibanez
	Comment 81 • 6 years ago

Although it is true the MRC Compose is a massive improvement over the current compose window of Thunderbird, modern email applications are much more advanced.

For an example, you just have to compose an email in GMail/Outlook or see this unmaintained add-on (developed by Mozilla staff but never merged into Thunderbird):  https://addons.thunderbird.net/en-US/thunderbird/addon/compose-for-thunderbird/

Or see the quick-reply compose window of Conversations (sadly not available for composing new emails): https://addons.thunderbird.net/user-media/previews/full/51/51379.png?modified=1530208682
https://addons.thunderbird.net/en-GB/thunderbird/addon/gmail-conversation-view/

(In reply to Dan from comment #79)
> Second the observation from Bob. Is MRC development dead?

https://github.com/michelRenon/mrc_compose/issues/35#issuecomment-385939943

	michelr
	Comment 82 • 5 years ago

Hi, I'm the author of MRC Compose.
First, thanks to all users of my addon.

The development is not dead, I don't have a lot of time to work on it.
But I'll look at the "TO" bug asap.
And there are some improvements that are planned (most important is CardBook compatibility).


About the inclusion in TB, it has already been discussed (comment #61)
Since that time (dec 2015 !), some things have changed  :
it would be possible to create new API to have MRC Compose and other addons (incompatible or not) work together :
https://mail.mozilla.org/pipermail/tb-planning/2018-December/006324.html
I think it is a huge positive step to allow inclusion in TB.
I you have any suggestion, please use the support email from addon page (https://addons.thunderbird.net/en-GB/thunderbird/addon/mrc-compose/)

Cheers,
Michel

	Richard Marti (:Paenglab)
	Comment 95 • 5 years ago

Comment on attachment 9100374 [details] [diff] [review]
440377-recipients-pills.patch

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

Thank you for doing this. This looks very promising and I think this is the way to go. Maybe it's only me but for me it looks a bit strange when the pills aren't in the textbox and the textbox moves to the right. 
What I found is, that tabbing doesn't focus the pills. Also do Mailing lists get the warning class.

::: mail/themes/shared/mail/messengercompose.css
@@ +501,5 @@
> +}
> +
> +.address-pill {
> +  border-radius: 9999px;
> +  padding: 0px 2px;

Please use zero without a unit.

@@ +508,5 @@
> +  background-color: rgba(0,0,0,0.1);
> +}
> +
> +:root[lwt-default-theme-in-dark-mode] .address-pill,
> +.address-pill:-moz-lwtheme {

Don't use :-moz-lwtheme here as this will apply to the pill also with a normal LW-theme which doesn't style the header, only the toolbars above.

@@ +518,5 @@
> +  background-color: rgba(255,0,0,0.1);
> +}
> +
> +:root[lwt-default-theme-in-dark-mode] .address-pill.warning,
> +.address-pill.warning:-moz-lwtheme {

Here too.

@@ +525,5 @@
> +}
> +
> +.address-pill:hover,
> +.address-pill:focus,
> +.address-pill:hover:focus,

:hover:focus isn't needed as both single selectors apply too.

	Paul Morris [:pmorris]
	Comment 97 • 5 years ago

Comment on attachment 9100374 [details] [diff] [review]
440377-recipients-pills.patch

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

I wrote an email to maildev with some thoughts about custom elements (subject: "custom elements, models, and views").  But that's a larger question/conversation.  Although it might be interesting to think about what more separation between model and view would look like in cases like this.

	Magnus Melin [:mkmelin]
	Comment 99 • 5 years ago

Comment on attachment 9100374 [details] [diff] [review]
440377-recipients-pills.patch

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

Quite a bit of work still to do, but looks like a promising start.

::: mail/base/content/mailWidgets.js
@@ +1726,5 @@
>    customElements.define("attachment-list", MozAttachmentlist, {
>      extends: "richlistbox",
>    });
> +
> +  class MozMailEmailRecipient extends MozXULElement {

MozMailAddress perhaps?
It could be good to follow the naming of parts, see https://tools.ietf.org/html/rfc5322#section-3.4 - down the line we want to support the group syntax too

@@ +1746,5 @@
> +      this.classList.add("address-pill");
> +      this.setAttribute("context", "emailAddressPopup");
> +      this.setAttribute("popup", "emailAddressPopup");
> +
> +      const label = document.createXULElement("label");

would use let for things like these. const is not commonly used for things like this in mozilla code

@@ +1823,5 @@
> +        "presenceTooltip"
> +      );
> +    }
> +
> +    _updateNodeAttributes(attrNode, attr, mappedAttr) {

Hmm, should be using inheritedAttributes functionality instetad for all of this I think.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6458,5 @@
> +  try {
> +    if (Services.prefs.getBoolPref("mail.autoComplete.highlightNonMatches")) {
> +      toAddrInput.highlightNonMatches = true;
> +    }
> +  } catch (ex) {}

shouldn't be in try/catch

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +1346,5 @@
>    return window.docShell.QueryInterface(Ci.nsILoadContext);
>  }
> +
> +function isEmptyOrSpaces(str) {
> +  return str === null || str.match(/^ *$/) !== null;

should be .test() instead of match() when you don't need the resulting match

@@ +1357,5 @@
> + * @param event       The DOM keypress event
> + * @param element     The element that triggered the keypress event
> + * @param parentID    The ID of the element's container
> + */
> +function recipientKeyPress(event, element, parentID) {

Need to handle comma as well. not sure if it's here or elsewhere

@@ +1363,5 @@
> +    case "Enter":
> +    case "Tab":
> +      // If the recipient input text is empty or contains only blank spaces
> +      // stop the execution of the script.
> +      if (isEmptyOrSpaces(element.value)) {

no need for this. just check if (!element.value.trim())

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +260,5 @@
>  <!ENTITY headersSpace.style "width: 9em;">
>  <!ENTITY fromAddr.label "From:">
>  <!ENTITY fromAddr.accesskey "r">
>  <!ENTITY toAddr.label "To:">
> +<!ENTITY toAddr.accesskey "T">

related to this, not sure you should use the recipient type selector.

	Magnus Melin [:mkmelin]
	Comment 104 • 5 years ago

Comment on attachment 9101029 [details] [diff] [review]
440377-recipients-pills.patch

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

Looking good. One think I notice is I can't really enter addresses not in the address book, they too need to become a pill. 
I think pills should be separated by commas, and typing in a comma should make whatever was typed in a pill too (tying into the previous sentence.)
A [x] icon at the end of each pill to remove it could be nice. Later on, maybe a nice tooltip too, but that would be for a followup.

I think the address type selector(s) should stay like they were so you can select To/CC/Bcc/Newsgroup/Followup/Reply-To, and others. Or be somehow else accessible. In the patch I can only add To.

	Richard Marti (:Paenglab)
	Comment 105 • 5 years ago

Comment on attachment 9101029 [details] [diff] [review]
440377-recipients-pills.patch

Looks good at this stage. The tabbing works now. When entering contacts and the autocomplete list is shown and I click on a contact in it all is okay but when I use ENTER the pill is created but email address of the contact is also shown behind the pill in the text box.

	Paul Morris [:pmorris]
	Comment 106 • 5 years ago

Comment on attachment 9101029 [details] [diff] [review]
440377-recipients-pills.patch

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

Looking good!  Everything works as described.  (I was able to make pills from non-address-book addresses just fine.)  Navigating pills with left and right arrow keys (in addition to tab and shift-tab) might be worth adding to the to-do list.  I'm still thinking about how to best do model/view separation with custom elements, but I'll post to that email thread with some thoughts on that.  So good to see this updated UI making its way into TB!

::: mail/base/content/mailWidgets.js
@@ +1890,5 @@
> +        this.classList.remove("error");
> +      }
> +
> +      if (
> +        this._isValidAddress(this.email) &&

No need to call _isValidAddress twice here, just call once and assign result to a local variable.  (Noticed this so thought I might as well comment, though I realize this is still WIP.)

	Richard Marti (:Paenglab)
	Comment 110 • 5 years ago

Comment on attachment 9106455 [details] [diff] [review]
440377-recipients-pills.patch

Looks very good. Only two things:
- the delete button inside the pill has a uneven border around it. On the right it is wider than top/bottom.
- when I edit a pill (not easy to find without a context menu :) ) it expands and shifts the following pills to the right. What about letting it the same width and only expand when the text inside will be wider when editing?

	Paul Morris [:pmorris]
	Comment 114 • 5 years ago

Comment on attachment 9106455 [details] [diff] [review]
440377-recipients-pills.patch

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

Looking good Alex!  It's great to see the progress on this.  I scanned through the code and made a few comments.  It worked well when I tried it out.  Here's a few little things I noticed and some other thoughts:

- Double-click to edit a pill, then click to put the cursor elsewhere, the pill stays in 'editing mode'.  In this way you can have multiple pills in editing mode at the same time.  Seems like it should exit editing mode when it loses focus?  (Probably just still to do.)

- When adding addresses, the cursor goes to the next line before you expect it to.  And when resizing the window the height of the "TO" field moves from one to two lines where you don't expect it.  Would it work to shrink the width of the text input element to avoid these things?

- Might not be worth it, at least in the first release of this, but users might want to be able to enter new addresses between existing pills.  So click between or use keyboard to navigate between and then start typing.  Something to keep in mind, if you're not already.

- Would be a good idea to develop this behind a temporary pref, so we could go ahead and land code incrementally as we go rather than all at once?  Then when it's ready, turn it on?

::: mail/base/content/mailWidgets.js
@@ +1773,5 @@
> +      this.setAttribute("originalInput", val);
> +    }
> +
> +    get originalInput() {
> +      return document.getElementById(this.getAttribute("originalInput"));

getElementById?  Seems odd here.

@@ +1781,5 @@
> +      return this.getAttribute("emailAddress");
> +    }
> +
> +    set email(val) {
> +      this.setAttribute("emailAddress", val);

Probably better for these identifiers to be the same like the others (here email != emailAddress).  Maybe consider "address" since it's shorter, with less room for confusion?  With "email" it might not be clear if it's the address or the whole message (e.g. "delete email").

@@ +1806,5 @@
> +    }
> +
> +    set displayName(val) {
> +      this.setAttribute("displayName", val);
> +    }

I wonder about how much we gain from these getters and setters that are just doing this.getAttribute/this.setAttribute... but fine to use them if you find them helpful.

@@ +1838,5 @@
> +    _setupEmailInput() {
> +      let pillCount = document.querySelectorAll("mail-address").length;
> +      this.appendChild(this.fragment);
> +      this.emailInput = this.querySelector(`input[is="autocomplete-input"]`);
> +      this.emailInput.setAttribute("id", `toAddrInput#${pillCount}`);

I'd avoid the "#" in an id.  I ran across this recently in the calendar code and it didn't work to do say `.querySelector("#toAddrInput#1")`.

@@ +1900,5 @@
> +      });
> +
> +      this.emailInput.popup.addEventListener("click", () => {
> +        this.emailInput.dispatchEvent(
> +          new KeyboardEvent("keypress", { key: "Enter" })

This seems a little odd to have a click event fire off a keypress event.  Wouldn't it be better to have both events calling the same function, if they are to do the same thing?  If there's a reason to do it this way, I'd suggest adding a comment.

@@ +1924,5 @@
> +      // });
> +    }
> +
> +    _isValidAddress(address) {
> +      return address.includes("@", 1) && !address.endsWith("@");

Hm, I wonder if we already have some code for validating addresses?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6478,5 @@
>  
>  function setupAutocomplete() {
> +  let toAddrInput = document.getElementById("toAddrInput");
> +  toAddrInput.popup.addEventListener("click", event => {
> +    toAddrInput.dispatchEvent(new KeyboardEvent("keypress", { key: "Enter" }));

Same question as above on click -> keypress events.

::: mail/components/compose/content/messengercompose.xul
@@ +2021,5 @@
> +                            minresultsforpopup="2"
> +                            ignoreblurwhilesearching="true"
> +                            onfocus="highlightAddressContainer(this);"
> +                            onblur="resetAddressContainer(this);"
> +                            onkeypress="recipientKeyPress(event, this, 'toAddrContainer');"/>

Hm, unfortunate that this is repeated in the CE and here in the XUL.  Not a big deal, but I wonder if it would be worth dynamically inserting it instead.

	Paul Morris [:pmorris]
	Comment 120 • 5 years ago

Comment on attachment 9107655 [details] [diff] [review]
440377-recipients-pills.patch

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

Looking good!  I tried all the things you listed and everything worked well.  I noticed some minor glitches and potential refinements that could be addressed here or in a follow-up.  

- How to resize the header area to make it scrollable?  I wasn't able to get this to happen.

- I was able to tab to the Cc, Bcc, etc. labels, but hitting enter when they are selected doesn't do anything yet.

- Do we want to alert the user that they are deleting addresses by closing Cc, Bcc, etc. or allow them to undo it after the fact?

- Backspace or delete to remove all pills.  If one is in editing mode, you get stuck on it.  Backspace (or delete) stops having any effect.

- Start editing a pill, delete all text in it, hit enter, nothing happens. I'd expect the pill to be deleted.

- Click a pill to select it.  Hit tab to move to the next pill.  The first pill stays selected in addition to the next pill.

- If a pill that's not the first or last pill is in editing mode and you use the arrow keys to try and navigate to it and past it, it doesn't work like with tab and shift+tab. When you get to that pill you can't arrow key away from it, only tab into it to edit it some more.  Arrow keys should work like tab and shift+tab for pills in editing mode.

- When a pill is in editing mode, maybe show a "checkmark" button (or similiar, in the place where the "X" button is), that would do the same as hitting "enter" on keyboard?  Then if I switch away from an editing pill, I can get it out of editing mode directly by using the mouse.

- If the last pill is in editing mode, you can't move the cursor to the text input to add another address without taking the pill out of editing mode first.

- It seems a little odd to have more than one pill in editing mode, but maybe it's fine and just a matter of getting used to it.  But if more than one pill can be in editing mode, it would be good to differentiate a selected editing pill from a non-selected editing pill, so you can tell where you are when tabbing through them.

	Richard Marti (:Paenglab)
	Comment 121 • 5 years ago

Comment on attachment 9107655 [details] [diff] [review]
440377-recipients-pills.patch

Looking good.
- When editing a pill and the text is longer than the pill, the text goes outside of the pill.
- I'm not so happy with the Cc: Bcc: line. It looks for me a bit like text that is there by accident. What about a second textfield with the menulist in front to choose the desired sending option, like the ones we use in our actual composer? Defaulting to Cc:?
- In the line above I have Newsgroups: and Followup-To: too but they don't work.
- After pressing F9 you have the Contacts sidebar with the buttons "Add to To:", "Add to Cc:" and "Add to Bcc:" but they don't work.

	Richard Marti (:Paenglab)
	Comment 124 • 5 years ago

Comment on attachment 9109194 [details] [diff] [review]
440377-recipients-pills.patch

Looks good. Looks almost complete.
The .address-container doesn't work well with dark theme, can be fixed later.
Mailing lists pills are coloured red like invalid addresses.

	Magnus Melin [:mkmelin]
	Comment 126 • 5 years ago

Comment on attachment 9109194 [details] [diff] [review]
440377-recipients-pills.patch

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

I don't particularly like how the Cc Bcc Reply-t Newsgroup Followup-to field names are lined up under the To line. 
Many webmails at least seem to favour putting them rightmost on or above the To/From. Not sure where we should put them, especially as we have more fields than typical. Also we'd not want to have the Newsgroup and Followup-To ones there showing for all mails.

For the To row, what's not working atm is at least entering addresses that aren't in your ab already. And commas between addresses should work.

The Subject "box" seem to have lost its top border

	Paul Morris [:pmorris]
	Comment 137 • 5 years ago

Comment on attachment 9110172 [details] [diff] [review]
440377-recipients-pills.patch

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

Nice work!  It's looking good to me.  I haven't read through the code yet, just tried the UI.  Sent a message, checked reply/forward, etc.  All seems in working order.   

One tiny nit: usually cut comes before copy, but currently copy comes before cut in the drop down menu.

I'm guessing the request for a try run was so that people can download the built binary and try it out, without having to build it locally.

	Richard Marti (:Paenglab)
	Comment 142 • 5 years ago

Comment on attachment 9110415 [details] [diff] [review]
440377-recipients-pills.patch

Looks almost ready.
- The fields are no more vertically aligned in columns because the text fields begins directly after the To: label.
- When I DnD a valid address from sidebar the address is shown red first until I press the ENTER button or click in a other text field. A valid address should never be red and when dropping a address from the sidebar it should change immediately convert to a pill.

	Magnus Melin [:mkmelin]
	Comment 146 • 5 years ago

Comment on attachment 9110415 [details] [diff] [review]
440377-recipients-pills.patch

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

Skimmed through this, some comments below:

::: mail/base/content/mailWidgets.js
@@ +1789,5 @@
> +    }
> +
> +    get originalInput() {
> +      return document.getElementById(this.getAttribute("originalInput"));
> +    }

The originalInput is really confusing. Maybe it just needs a better name and documentation. It's just the base name?

@@ +1882,5 @@
> +      this.addEventListener("dblclick", event => {
> +        this.startEditing(event);
> +      });
> +
> +      this.addEventListener("keypress", event => {

a lot of cases under this. a switch would be more readable

@@ +1948,5 @@
> +
> +      this.emailInput.addEventListener("keypress", event => {
> +        setTimeout(() => {
> +          this._finishEditing(event.key);
> +        }, 1);

you can leave out the 1ms

@@ +2123,5 @@
> +      this.focus();
> +    }
> +  }
> +
> +  customElements.define("mail-address", MozMailAddress);

maybe MailAddressPill + mail-address-pill would be better

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3486,5 @@
>  /**
> + * Create the recipient label to add in the messenger compose dialog.
> + *
> + * @param  labelID     The unique identifier of the email header
> + * @return label       The newly create XUL label

Please for all the documentation use the proper JSDoc style, so:
``` 
 * @param {string} labelID - The unique identifier of the email header.
 * @return {string} The newly create XUL label.
```

@@ +3492,5 @@
> +function createRecipientLabel(labelID) {
> +  let label = document.createXULElement("label");
> +  label.setAttribute("id", labelID);
> +  label.setAttribute("value", `${labelID}:`);
> +  label.setAttribute("onclick", `showAddressRow(this, 'addressRow${labelID}')`);

add an event listener instaed

@@ +3497,5 @@
> +  label.setAttribute(
> +    "onkeypress",
> +    `showAddressRowKeyPress(event, this, 'addressRow${labelID}')`
> +  );
> +  label.setAttribute("inputelement", `${labelID}AddrInput`);

We should avoid putting random attributes on standard elements like this. Is there a better way to handle this?

@@ +6312,5 @@
>    // Collapsing the menuitem only prevents it getting chosen, it does not
>    // affect the menulist widget display when Newsgroup is already selected.
>    for (let item of newsTypes) {
>      item.collapsed = hideNews;
>    }

please remove addrWidget and change this all to

for (let item of document.querySelectorAll("#addr_newsgroups,#addr_followup") {
  item.collapsed = hideNews;
}

@@ +6341,5 @@
> +      "#bccAddrInput",
> +      "#replyAddrInput",
> +      "#newsgroupsAddrInput",
> +      "#followupAddrInput",
> +    ]);

looks wrong, querySelectorAll takes a string not an array.

I'd inline the inputs variable

@@ +6545,5 @@
> +    "#newsgroupsAddrInput",
> +    "#followupAddrInput",
> +  ]);
> +
> +  for (let input of inputs) {

same

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +189,4 @@
>    }
> +
> +  let container = element.parentNode.parentNode;
> +  let pills = container.querySelectorAll("mail-address");

inline

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +480,5 @@
>  ## Identity matching warning notification bar.
>  ## LOCALIZATION NOTE(identityWarning): %S will be replaced with the identity name.
>  identityWarning=A unique identity matching the From address was not found. The message will be sent using the current From field and settings from identity %S.
> +
> +confirmRemoveRecipientRowTitle=Confirm Row Removal

Remove <fieldname>

@@ +481,5 @@
>  ## LOCALIZATION NOTE(identityWarning): %S will be replaced with the identity name.
>  identityWarning=A unique identity matching the From address was not found. The message will be sent using the current From field and settings from identity %S.
> +
> +confirmRemoveRecipientRowTitle=Confirm Row Removal
> +confirmRemoveRecipientRowBody=All the addresses written in this field will be removed. Do you want to proceed?

Should say which field

	Richard Marti (:Paenglab)
	Comment 157 • 5 years ago

Comment on attachment 9111839 [details] [diff] [review]
440377-ui-solution1.patch

Only f+ because it's Linux only yet. Looks good, thanks.
I write more in the next comment with a screenshot.

	Paul Morris [:pmorris]
	Comment 159 • 5 years ago

Comment on attachment 9111839 [details] [diff] [review]
440377-ui-solution1.patch

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

Looks good, and I like the new UI refinements.  I'm in agreement with Richard's points and also found the drop down shadows to be too prominent.  Otherwise it appears to work well.

However, I noticed one problem that I think should be addressed before landing this.  I cut and pasted a large number of comma separated addresses, say 50-100, into the To field and then hit enter to turn them into pills.  The field expanded vertically for all the pills to fit, and that pushed the rest of the UI off the bottom of the window, and there was no way to scroll to bring it back into view.

	Richard Marti (:Paenglab)
	Comment 161 • 5 years ago

Comment on attachment 9111838 [details] [diff] [review]
440377-recipients-pills.patch

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

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +310,5 @@
>  
> +#identityLabel-box,
> +#subjectLabel-box,
> +.address-label-container {
> +  width: 8em;

Instead of hard coding the width here, you should probably do it like before with style="&headersSpace.style;" to make the width localizable.

	Richard Marti (:Paenglab)
	Comment 167 • 5 years ago

Comment on attachment 9112393 [details] [diff] [review]
440377-recipients-pills.patch

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

::: mailnews/addrbook/content/abMailListDialog.js
@@ +781,5 @@
> + * Delete recipient row (addressingWidgetItem) from UI.
> + *
> + * @param {<html:input>} inputElement  the recipient input element
> + *                                     (textbox-addressingWidget) whose parent
> + *                                     row (addressingWidgetItem) will be deleted.

There is no textbox-addressingWidget anymore. Is this parameter still valid?

	Richard Marti (:Paenglab)
	Comment 168 • 5 years ago

Comment on attachment 9112429 [details] [diff] [review]
440377-ui-solution1.patch

Yes, this is the way I think we should go.

Would it be somehow possible to give the "Add Cc:" etc. labels a "drop-on" attribute when I'm over them with a address? Then we could give the user a feedback if the address is on the right label to add.

	Magnus Melin [:mkmelin]
	Comment 170 • 5 years ago

Comment on attachment 9112393 [details] [diff] [review]
440377-recipients-pills.patch

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

Didn't test extensively yet. Below some code comments.
Autocomplete by adding comma + entering the next person doesn't work unless it's the first comma. I think we should also be consistent with commas and maybe add them between each recipient.

Leaving a pill in editing mode when you click outside works oddly. I know there were some use cases listed in comments above, but it's unexpected. And if you, say, move on to body - has it taken affect or not...?

::: mail/base/content/mailWidgets.js
@@ +1756,5 @@
> +
> +  class MailAddressPill extends MozXULElement {
> +    static get inheritedAttributes() {
> +      return {
> +        ".emaillabel": "crop,value=label",

since this is a a new element, instead of mapping value to label, we could just use label. I think that's clearer too

@@ +1783,5 @@
> +      this._setupEventListeners();
> +      this.initializeAttributeInheritance();
> +    }
> +
> +    // Set the ID of the autocomplete input field of the recipient row.

JSDoc style documentation for the function comments please

@@ +1830,5 @@
> +      return !this.emailInput.hasAttribute("hidden");
> +    }
> +
> +    get pills() {
> +      return this.parentNode.querySelectorAll("mail-address-pill");

I don't think this belongs here. An element is self-contained and shouldn't be allowed to look into the parent.

@@ +1834,5 @@
> +      return this.parentNode.querySelectorAll("mail-address-pill");
> +    }
> +
> +    get allPills() {
> +      return document.querySelectorAll("mail-address-pill");

.... and not into the whole document scope either ;)

@@ +1838,5 @@
> +      return document.querySelectorAll("mail-address-pill");
> +    }
> +
> +    get allSelectedPills() {
> +      return document.querySelectorAll(`mail-address-pill[selected]`);

same here

@@ +1841,5 @@
> +    get allSelectedPills() {
> +      return document.querySelectorAll(`mail-address-pill[selected]`);
> +    }
> +
> +    get fragment() {

There are some more efficient ways for this, like https://searchfox.org/mozilla-central/rev/073b138dcba41cd3f858522e5f0a9ee73e39afa0/browser/base/content/tabbrowser-tab.js#72

@@ +1881,5 @@
> +      this.addEventListener("dblclick", event => {
> +        this.startEditing(event);
> +      });
> +
> +      this.addEventListener("keypress", this._handleKeyPress);

seems you should inline _handleKeyPress

@@ +1885,5 @@
> +      this.addEventListener("keypress", this._handleKeyPress);
> +
> +      this.emailInput.addEventListener("keypress", event => {
> +        // We need the setTimeout to wait for the autocomplete to run its
> +        // original methods before using the correct value.

That's unfortunate. There's probably some better way. Maybe as a followup

@@ +1941,5 @@
> +          this.checkKeyboardSelected(event, this.nextSibling);
> +          break;
> +
> +        case "Home":
> +          this.parentNode.firstChild.focus();

the parentNode should be in charge of this functionality

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4033,5 @@
> +      );
> +      let isMailingList =
> +        listNames.length > 0 &&
> +        MailServices.ab.mailListNameExists(listNames[0].name);
> +      let isNewsgroup = address.originalInput.id == "newsgroupsAddrInput";

followup is also for newsgroups. 
I don't think we should be looking at ids here. Can you add some suitable class instead, like "nntp-input"?

@@ +6332,5 @@
>    }
>    // If there is no News (NNTP) account existing then
>    // hide the Newsgroup and Followup-To recipient type in all the menulists.
> +  for (let item of document.querySelectorAll(
> +    "#addr_newsgroups,#addr_followup"

such a class would be useful here too

@@ +6359,5 @@
>      }
>  
> +    for (let input of document.querySelectorAll(
> +      "#toAddrInput,#ccAddrInput,#bccAddrInput,#replyAddrInput,#newsgroupsAddrInput,#followupAddrInput"
> +    )) {

and here

@@ +6587,5 @@
> +
> +/**
> + * Select all the pills in the recipient container if they exist.
> + *
> + * @param input      The autocomplete input field

@paran {HTMLElement} input - The autocomplete input field.

But is there something off here? Should it be selectRecipientPill? (without s). If not, how can you focus()?

@@ +7058,5 @@
>      document.getElementById("appcontent")
>    );
>  }
>  
> +function GetMsgToRecipientElement() {

maybe we can just get rid of this... there's no need for caching and all it does is look up the element by id which is super fast

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +47,5 @@
> +        .map(fullValue =>
> +          headerParser.makeMimeAddress(fullValue.name, fullValue.email)
> +        )
> +        .join(", ");
> +    } catch (ex) {

what problem are we expecting?

@@ +172,5 @@
>      return;
>    }
>  
> +  let element;
> +  let label = document.getElementById(recipientType);

you can move this down to where it's first used

@@ +264,5 @@
>    if (dragSession.isDataFlavorSupported("text/x-moz-address")) {
>      validFlavor = true;
>    }
>  
>    if (validFlavor) {

just use 
if (dragSession.isDataFlavorSupported("text/x-moz-address")) 


... and get rid of the validFlavor var

@@ +283,4 @@
>      dragSession.getData(trans, i);
> +    let dataObj = {};
> +    let bestFlavor = {};
> +    let len = {};

please drop the len, see bug 1600606

@@ +367,5 @@
> + * in the Message Compose window.
> + *
> + * @param {Event} event - The DOM keypress event
> + * @param {HTMLElement} element - The element that triggered the keypress event
> + * @param {string} parentID - The ID of the element's container

nit: add periods to these at the end of the sentence

@@ +532,5 @@
> +/**
> + * Activate the pill's EditMode from the context menu.
> + *
> + * @param {HTMLElement} element - The element from which the context menu was
> + *                                opened.

I think for cases like this, the nicer row wrapping is to just indent by two on the next line, like

@param
  opened.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +484,5 @@
> +## Recipient pills fileds.
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle): %S will be replaced with the field name.
> +confirmRemoveRecipientRowTitle=Remove %S
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle): %S will be replaced with the field name.
> +confirmRemoveRecipientRowBody=All the addresses written in the %S field will be removed. Do you want to proceed?

remove the "written"?

::: mail/themes/shared/mail/messengercompose.css
@@ +514,5 @@
> +}
> +
> +.address-row.hidden {
> +  visibility: hidden;
> +  display: none !important;

hmm, both visibility and display?
and is the important neeeded?

	Alessandro Castellani [:aleca] Assignee
	Comment 172 • 5 years ago

Comment on attachment 9112673 [details] [diff] [review]
440377-ui-solution1.1.patch

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

Good stuff, thank you so much for taking care of this.
I'll merge this edits into the main patch so we have a solid and unique starting point to use for the UI solution 2.

Regarding your question:
"Would it be somehow possible to give the "Add Cc:" etc. labels a "drop-on" attribute when I'm over them with a address?"

For sure, I will try to implement something like that in the follow up patch to enable drag&drop of pills between fields.

	Richard Marti (:Paenglab)
	Comment 178 • 4 years ago

Comment on attachment 9113385 [details] [diff] [review]
440377-recipients-pills.patch

Looks good to me.

	Richard Marti (:Paenglab)
	Comment 179 • 4 years ago

Comment on attachment 9113386 [details] [diff] [review]
440377-ui-solution2.patch

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

This works for me too.
I'm not a big fan of this because when a address while dropping isn't correctly placed over the Cc, Bcc buttons, the address is added to the To field. Solution 1 hasn't this issue. Also when a user uses full screen window on a wide screen, the additional recipient buttons are far away.
And the recipient types in the overflow arrow aren't direct drop targets, two additional mouse clicks are needed to add the field.
The `extraRecipientsPanel` could get a --arrowpanel-padding: 4px; like we have for the `reorderAttachmentsPanel`.

I give ui-r+ because it works like the solution 2 proposal but I'm still in favour of solution 1.

::: mail/themes/shared/mail/messengercompose.css
@@ +632,5 @@
>  
>  .address-extra-recipients {
> +  margin-block: 4px;
> +  margin-inline-start: 4px;
> +	align-self: end;

Here a tab slipped in.

	Magnus Melin [:mkmelin]
	Comment 181 • 4 years ago

Comment on attachment 9113385 [details] [diff] [review]
440377-recipients-pills.patch

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

This thing is huge... so not easy to review. Some smaller and larger things yet to address, see below.

* Regarding the commas, I think we should show them between addresses (but not in red like now). This makes it more clear, and also allows for more advanced addressing in the future, like group syntax.
* select all + copy: please make the pasted addresses separated by ", " and not just ", "
* newsgroups/followup-to fields should not autocomplete to news addresses, not to email addresses (originally implemented in bug 61491)

::: mail/base/content/mailWidgets.js
@@ +1754,5 @@
>    customElements.define("attachment-list", MozAttachmentlist, {
>      extends: "richlistbox",
>    });
> +
> +  class MailAddressPill extends MozXULElement {

please add JsDoc documentation for the class.

@@ +1774,5 @@
> +      this.emailLabel = document.createXULElement("label");
> +      this.emailLabel.classList.add("emaillabel");
> +
> +      this.emailDeleteImage = document.createXULElement("image");
> +      this.emailDeleteImage.classList.add("emailDelete");

nit: would be nicer with css classes all-lowercase-with-dash instead, like delete-pill-icon (it's also for newsgroups, so not necessarily an email)

@@ +1928,5 @@
> +          this.startEditing(event);
> +          break;
> +
> +        case "Delete":
> +          this.removePills(this.nextSibling);

these would probably want to be nextElementSibling and such?

@@ +1999,5 @@
> +
> +    clearSelected() {
> +      for (let pill of getAllPills()) {
> +        pill.removeAttribute("selected");
> +      }

You moved the getAllPills out of the pill class, but you still have the implementation acting on more than this pill inside the class, which is wrong. Maybe there needs to be a mail-pills container class for this functionality?

@@ +2033,5 @@
> +        this.clearSelected();
> +      }
> +    }
> +
> +    removePills(element) {

please document

@@ +2066,5 @@
> +        return;
> +      }
> +
> +      for (let pill of getAllPills()) {
> +        pill.finishEditing("Escape");

might be nicer to make the key be optional

@@ +2137,5 @@
> +      if (!isValid && !isMailingList && !isNewsgroup) {
> +        this.classList.add("error");
> +      } else {
> +        this.classList.remove("error");
> +      }

for this and below, it would be shorter with toggle

this.classList.toggle("error", !isValid && !isMailingList && !isNewsgroup)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2321,5 @@
> +
> +  // Wait for the autocomplete input to attach all the controllers.
> +  setTimeout(() => {
> +    CompFields2Recipients(gMsgCompose.compFields);
> +  });

This is not really appropriate, and will likely cause non-determinism => problems in tests at least

@@ +3387,5 @@
> +      let addressingWidgetLabels = document.getElementById(
> +        "addressingWidgetLabels"
> +      );
> +      let recipientsContainer = document.getElementById("recipientsContainer");
> +      let other_headers_Array = other_headers.split(",");

can we change the variable snake namings while touching this

@@ +3393,1 @@
>        for (let i = 0; i < other_headers_Array.length; i++) {

just do 
for (let header of other_headers.split(",")) {

}

@@ +3493,5 @@
> +function createRecipientLabel(labelID) {
> +  let label = document.createXULElement("label");
> +  let addLabel = getComposeBundle().getString("recipientAddLabel");
> +  label.setAttribute("id", labelID);
> +  label.textContent = ` ${addLabel} ${labelID}`;

please use appropriate l10n functionality to achieve this, per my previous review comment

@@ +3525,5 @@
> +  row.setAttribute("id", `addressRow${labelID}`);
> +  row.classList.add("addressingWidgetItem", "address-row", "hidden");
> +
> +  let firstCol = document.createXULElement("hbox");
> +  firstCol.setAttribute("align", "top");

top shouldn't be used anymore, see bug 1599548. should be "start" now. 
you have some more of this later

@@ +3992,5 @@
> + */
> +function focusAddressInput(event) {
> +  let container = event.originalTarget;
> +  if (container.classList.contains("address-container")) {
> +    let input = container.lastChild;

please make sure to use the Element versions, like lastElementChild
Not using that will cause problems when in an html document (where space nodes matter)

@@ +4026,5 @@
> +    if (!parent) {
> +      continue;
> +    }
> +
> +    let addresses = parent.querySelectorAll(".address-pill");

could just inline addresses

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +36,5 @@
>  
> +  for (let pill of document.getElementsByTagName("mail-address-pill")) {
> +    let fieldValue = pill.fullAddress;
> +    let recipientType = pill.getAttribute("recipienttype");
> +    let recipient = null;

move down two lines to where you first use it

@@ +440,5 @@
> +  pill.label = address.toString();
> +  pill.emailAddress = address.email || "";
> +  pill.fullAddress = address.toString();
> +  pill.displayName = address.name || "";
> +  pill.originalInput = element.id;

I'd suggest not having the originalInput. Looks like you could do without: store the aria-labelled-by + whether it's a newsgroup and whatever you need right here.

@@ +528,5 @@
> +  container.removeAttribute("focused");
> +}
> +
> +/**
> + * Activate the pill's EditMode from the context menu.

edit mode?

@@ +539,5 @@
> +  if (element.tagName == "mail-address-pill") {
> +    element.startEditing(event);
> +  } else {
> +    editAddressPill(element.parentNode, event);
> +  }

whops. I thing you want 
let pill = element.closest("mail-address-pill");

@@ +551,5 @@
> + */
> +function copyEmailNewsAddress(element) {
> +  if (!element) {
> +    return;
> +  }

wouldn't thing we run into that.

@@ +553,5 @@
> +  if (!element) {
> +    return;
> +  }
> +
> +  if (element.tagName == "mail-address-pill") {

here too, just look up closest and don't recurse

@@ +584,5 @@
> + * Delete the selected pill/pills.
> + *
> + * @param {XULElement} element - The element from which the context menu was
> + *   opened.
> + * @param {Event} event - The DOM event.

no such thing here

@@ +589,5 @@
> + */
> +function deleteAddressPill(element) {
> +  if (!element) {
> +    return;
> +  }

I would expect we never get there do we?

@@ +591,5 @@
> +  if (!element) {
> +    return;
> +  }
> +
> +  if (element.tagName == "mail-address-pill") {

closest

@@ +644,5 @@
> + * @param {XULelement} element - The clicked label.
> + * @param {string} labelID - The ID of the label to show.
> + */
> +function hideAddressRow(element, labelID) {
> +  let container = element.parentNode.parentNode;

for clarity it might be easier to use closest() here, matching on some class

@@ +645,5 @@
> + * @param {string} labelID - The ID of the label to show.
> + */
> +function hideAddressRow(element, labelID) {
> +  let container = element.parentNode.parentNode;
> +  let pills = container.querySelectorAll("mail-address-pill");

please move down to where it's first used

@@ +671,5 @@
> +  }
> +
> +  // Reset the original input.
> +  let input = container.querySelector(`input[is="autocomplete-input"]`);
> +  input.textValue = "";

for the inputs, you want input.value ?

@@ +706,5 @@
> + *
> + * @param {XULElement} pill - The mail-address-pill element.
> + */
> +function setFocusOnFirstPill(pill) {
> +  pill.parentNode.firstChild.focus();

firstElementChild

::: mail/components/compose/content/messengercompose.xul
@@ +759,5 @@
> +            class="menuitem-iconic"/>
> +  <menuitem id="menu_delete" label="&deleteCmd.label;"
> +            accesskey="&deleteCmd.accesskey;"
> +            oncommand="deleteAddressPill(document.popupNode)"
> +            class="menuitem-iconic"/>

the context menus typically don't have the menuitem-iconc, so I guess we should skip it here too

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +258,5 @@
>  <!--LOCALIZATION NOTE headersSpace.style is for aligning  the From:, To: and
>      Subject: rows. It should be larger than the largest Header label  -->
> +<!ENTITY headersSpace.style "width: 8em;">
> +<!ENTITY recipientAdd.label "Add">
> +<!ENTITY fromAddr.label "From">

you can't change the localization value without changing the key, goes for all the blow too.

::: mailnews/addrbook/content/abMailListDialog.js
@@ +481,5 @@
> + * Returns the recipient type popup for a row.
> + *
> + * @param row  Index of the recipient row to return. Starts at 1.
> + * @return     This returns the menulist (not its child menupopup), despite the function name.
> + */

please remove the extra spaces after return