Closed Bug 440377 Opened 12 years ago Closed 7 months ago

Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line/row per address

Categories

(Thunderbird :: Message Compose Window, enhancement, P2)

enhancement

Tracking

(thunderbird_esr68 wontfix, thunderbird72 wontfix)

RESOLVED FIXED
Thunderbird 73.0
Tracking Status
thunderbird_esr68 --- wontfix
thunderbird72 --- wontfix

People

(Reporter: olaf, Assigned: aleca)

References

(Blocks 15 open bugs, Regressed 2 open bugs, )

Details

(Keywords: ux-efficiency, ux-minimalism, Whiteboard: [parity-all-other-mailers][parity-postbox][relnotetb78])

Attachments

(8 files, 27 obsolete files)

39.69 KB, patch
Paenglab
: ui-review+
mkmelin
: feedback+
Details | Diff | Splinter Review
13.58 KB, image/png
Details
226.27 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
77.62 KB, image/png
Details
15.01 KB, image/jpeg
Details
15.16 KB, image/jpeg
bugzilla2007
: ui-review-
Details
54.77 KB, image/png
Details
57.99 KB, image/png
Details
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 ... ;)
Olaf, I'm not defending the current implementation, but what problem do you encounter regarding one line/address?  Or are we talking strictly aesthetics?
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.
Version: unspecified → 2.0
>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
Severity: normal → enhancement
Component: Message Compose Window → MailNews: Composition
OS: Windows 2000 → All
Product: Thunderbird → Core
QA Contact: message-compose → composition
Hardware: PC → All
Summary: using address fields (To, Cc,Bcc) is a pain → allow address fields (To, Cc,Bcc) to have multiple addresses in comma separated list
Version: 2.0 → unspecified
(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.
(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.


Product: Core → MailNews Core
Duplicate of this bug: 479997
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Summary: allow address fields (To, Cc,Bcc) to have multiple addresses in comma separated list → allow address fields (To, Cc,Bcc) to have multiple addresses in comma separated list on a single line
Summary: allow address fields (To, Cc,Bcc) to have multiple addresses in comma separated list on a single line → address fields (To, Cc, Bcc) should be one field with multiple comma separated addresses - not one line per address
Duplicate of this bug: 506203
Duplicate of this bug: 416711
Duplicate of this bug: 528008
On a drive-by note: commas may appear in email addresses, so using comma as a separator isn't wise...
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.
Duplicate of this bug: 553133
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.
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 :-)
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.
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.
#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.
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.
Duplicate of this bug: 597790
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.
Terrible bug!
Duplicate of this bug: 495241
See Also: → 535484
Summary: address fields (To, Cc, Bcc) should be one field with multiple comma separated addresses - not one line per address → Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line per address
Summary: Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line per address → Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line/row per address
Blocks: 272318
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?
Depends on: 392932
Whiteboard: [patchlove]
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?
(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?
Whiteboard: [patchlove] → [patchlove][parity-all-other-mailers][parity-postbox]
@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.
(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.
Whiteboard: [patchlove][parity-all-other-mailers][parity-postbox] → [patchlove][proof of concept/workaround: addon, see URL][parity-all-other-mailers][parity-postbox]
@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.
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
Blocks: 395606
FYI, I'm starting work on this now.
Assignee: nobody → josiah
Status: NEW → ASSIGNED
(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.
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.
Blocks: 260564
Blocks: 933472
(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!
(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.
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?
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.
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...
..."
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... :)
(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...)
I still see no mentioning of the "Reply-To", "Newsgroup" and "Followup-To" recipient types.
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.
Whiteboard: [patchlove][proof of concept/workaround: addon, see URL][parity-all-other-mailers][parity-postbox] → [patchlove][proof of concept/workaround: addon, see URL][parity-all-other-mailers][parity-postbox][ux-feature-wanted-38]
Duplicate of this bug: 1100103
No longer blocks: 1106457
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.
Flags: needinfo?(josiah)
(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.
Assignee: josiah → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(josiah)
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.
(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.
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?
Duplicate of this bug: 1077673
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)
Duplicate of this bug: 1159652
(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.
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
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?
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.
(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.
(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
(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.
(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
(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
(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.
(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.
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.
I especially like how this extension shows email list recipients after adding the email list.
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?
(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/
(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.
(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.
With=without*

sorry for the typo.
Please provide the promised functionality of multiple addresses in one To: field
The current idea of separated fields is a pain and very strange
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
Duplicate of this bug: 1446424
(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.
Duplicate of this bug: 1502391
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.
Attached image Screenshot_20181027_223333.png (obsolete) —
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.
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!
Second the observation from Bob. Is MRC development dead?
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 :-)
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
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

Following with interest. I was a user of MRC, but had to drop it as it is currently incompatible with Cardbook (which provides two way integration with could address books, like google's).

MRC with the fix to allow these address books (https://github.com/michelRenon/mrc_compose/issues/35 ) would be a massive improvement to Thunderbird.

Assignee: nobody → alessandro
Duplicate of this bug: 1535503

This bug is long overdue.
Here's a mockup I did for the redesign of the address area in the compose dialog.
attachment 9052074 [details]

Summary: Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line/row per address → Redesign message compose recipients fields
Priority: -- → P2

(adding back the old summary for findability)

Summary: Redesign message compose recipients fields → [Bug 440377] Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line/row per address
Duplicate of this bug: 392932
Summary: [Bug 440377] Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line/row per address → Redesign recipients address fields (To, Cc, Bcc) as single-line input fields for multiple comma separated addresses - not one line/row per address
Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

All right, here's a first and super broken patch as a proof of concept for this new implementation.
I'm asking early feedback before going to deep.

This is just a test patch with some early work, almost everything is broken and not functional at all, so, be careful. These are the things I did and looking for feedback.

  • Only 1 autocomplete input, no duplication of rows (the CC, BCC, etc, will have their own fields).
  • On enter or autocomplete select, the value gets converted into a custom element.
  • The new CE is a blatant copy of the email address field in the Message Header. For now is super broken, but the objective is to have the same features of the Message Header, like adding an address to the Address Book, the same context menu, etc.
  • Invalid emails or not matching the address book will be highlighted in red.
  • The field can handle multiple addresses at once pasted in, separated via comma or semicolon.
  • After every enter, the focus stays on the input field to allow a continuous typing experience.
  • I also did a bit of dark mode styling, but nothing too crazy.

That's is so far, pretty barebone initial patch.
The UI is pretty ugly right now and nothing really works, but as I said, this is just for an early evaluation.

What do you guys think about the approach from a code POV?
Does it look decent so far?
Is this a good direction?

Attachment #9100374 - Flags: feedback?(richard.marti)
Attachment #9100374 - Flags: feedback?(paul)
Attachment #9100374 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attached video input-pills.webm (obsolete) —

Uploading a quick screencap to showcase it.

Attachment #9020565 - Attachment is obsolete: true

As far as I can see from the attached video sample, this is really awesome!
The interface gives a good experience, expecially to unexperienced users.
Good job 👍

Please keep us updated on your progress; I'm ready to test it when you have a testable version.

Result and video are great... well done! Very promising...

Perhaps I would suggest to set the recipient name in bold perhaps so to create some sort of visual "separation" or "distinction" (both between name and address and between Pills) without adding any extra spaces... this would help to review entries visually... especially when numerous... if that make any sense...

Would it now also be possible to copy and paste multiple addresses from a draft email to a new one at once?

Other ideas for later... if you would consider...

What about coloring Pills other than grey to see them better at least?

One color per Addressbook perhaps? Or at leat to distinguish those coming from Addressbook, Auto-Collected and New (maybe not red as this color in associated with error)?

When typing recipient not selected from auto-completion (e.g test@test.com), one could imagine that pressing space bar any time after @ would act as comma, semi-colon or enter and convert entered data into a Pills (if valid or tolerable mistake) which may be even easier and more natural way to separate recipients perhaps... in addition of other options available...

Also only converted and valid "name <address>" shall be converted in Pills and saved in auto-completion (upon sending? or conversion into Pills?) for new recipient... faulty once shall not be converted, saved or usable (or at own risk?)... a warning would raise upon
conversion/validation... but still allowing end-user to correct or continue at his own "risk" ;-)

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.
Attachment #9100374 - Flags: feedback?(richard.marti) → feedback+

This is looking good overall, a nice and promising start on a valuable feature! I have a few thoughts.

Glancing over the code I noticed two places where the focus shifts to the subject line. Ideally there would be only one place where that happens. Should be do-able.

As a user I share the concern expressed above about not being able to copy a group of addresses that have been entered. (It's one of my frustrations with the current one-address-per-row UI.) It looks like pasting a group of addresses in would work (right?), but copying them back out looks like a more difficult problem -- if we want to be able to do that and have the pill functionality.

If we can't support selecting and copying (and deleting?) groups of pills, then we might consider offering other options, like maybe a button to "copy all to clipboard" or maybe a toggle that switches the view between "pill view" and "plain text view". Some users might prefer the plain text option. If a plain text view is something we want to offer, then it might make sense by starting with it (simpler) and then adding the pill view.

I suppose you could let backspace delete the previous pill when there's no text in the input field.
I assume it will be possible to edit an address after it has become a pill.

Finally, I have some thoughts about our use of custom elements that are a broader / another topic. I'll save that for an email or something else that's not this bug. (We're about to hit triple digit comments on this bug, so might be worth making it a meta bug at some point.)

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.
Attachment #9100374 - Flags: feedback?(paul) → feedback+

Perhaps I would suggest to set the recipient name in bold perhaps so to create some sort of visual "separation" or "distinction"

That's a good point, and really easy to implement. I'll see how it looks at a later stage, when thorough UI polish is needed.

Would it now also be possible to copy and paste multiple addresses from a draft email to a new one at once?

Yes. It will be possible to select all the pills to copy and paste them into another draft email.

What about coloring Pills other than grey to see them better at least?
One color per Addressbook perhaps? Or at leat to distinguish those coming from Addressbook, Auto-Collected and New (maybe not red as this color in associated with error)?

I'll explore better colouring at a later stage, but I'm not excluding these suggestions, thanks.

Also only converted and valid "name <address>" shall be converted in Pills and saved in auto-completion (upon sending? or conversion into Pills?) for new recipient... faulty once shall not be converted, saved or usable (or at own risk?)... a warning would raise upon
conversion/validation... but still allowing end-user to correct or continue at his own "risk" ;-)

The pill won't be created unless there's a valid email address in the string (I'm coding this right now).
Fixing a mistaken "saved in auto-completion" action is something we should tackle in a separated bug.

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.

Yeah, that's just temporary as proof of concept. The final implementation won't show a separated textbox but rather the entire row will be clickable to add more addresses.

Tabbing doesn't focus the pills.

Yes, that's something I'll fix once tackling the keyboard navigation of these elements.

Also do Mailing lists get the warning class.

Yup, I still working on that :D

As a user I share the concern expressed above about not being able to copy a group of addresses that have been entered. (It's one of my frustrations with the current one-address-per-row UI.) It looks like pasting a group of addresses in would work (right?), but copying them back out looks like a more difficult problem -- if we want to be able to do that and have the pill functionality.

Copy and pasting multiple pills at once will be possible.

If we can't support selecting and copying (and deleting?) groups of pills, then we might consider offering other options...

All these things will be available and will emulate the same behaviour of dealing with a simple text string.

I suppose you could let backspace delete the previous pill when there's no text in the input field.

Yes

I assume it will be possible to edit an address after it has become a pill.

And yes

Thank you all for the feedback, I'll keep working and upload another WIP patche once I reach a good state.

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.
Attachment #9100374 - Flags: feedback?(mkmelin+mozilla) → feedback+

Strongly disagree with this initiative as now stated; and - this is a key aspect of Thunderbird UI which merits a wider surveying of users IMHO.

The opening comment says that one-row-per-address is

OK for 1-3 addresses, but with more it is a real pain.

Not it isn't - in itself. It has issues which may need to be addressed - but going many-address-per-line is trading one set of problems for another. I will elaborate below, based on Olaf's list of arguments. But the main point is that it is difficult and inconvenient to search through multiple lines with variable number of recipients each of varying length and possibly different languages. The fact that Thunderbird does not make you do that (when writing email at least) - is a benefit of Thunderbird over, say, MS Outlook and Evolution. Don't take it away please

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

And so does Thunderbird. You can type that in or copy-paste it and it'll get adjusted to the one-address-per-line view. If you want to see all the addresses narrowly-spaced together - that could be a feature I suppose, but not replacing what we have now.

Olaf presented several arguments against the current UI approach; let me address them.

  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.

The important habits are for Thunderbird users, not for MS Outlook users; and immitating MS Outlook is not a principle to go by. Also note that when writing formal letters, we typically have one recipient per line - that is the actual habit to uphold.

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

When you have more than 20 or 30 recipients, you would need to scroll with multiple addresses per line. Plus, the non-scrolling search becomes difficult faster.

It is much easier if you can see all the recipients at a glance.

No. It would be easier if we could see them all aligned. But on a single line - we don't really see them at a glance. Unless there are few of them, in which case we usually see them at a glance as things stand already.

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 :)

You very well might - since it's easier to miss people in non-aligned layout. Also, with many recipients - you need to scroll anyway, and scrolling is much easier when there's alignment.

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

But if you move a person from "To" to "CC" it is more difficult, since instead of just two clicks - or even a single click, see: https://bugzilla.mozilla.org/show_bug.cgi?id=251917

  1. If you want to remove some addresses you need to select every one separately and delete.

That is not inherent to a one-address-per-line layout. This can, and should, be changed. I'm pretty sure there's a bug page about that.

BTW. there is no way to delete unused fields, it is possible to delete only addresses.

You don't need to "delete unused fields"; there are infinitely many unused rows at the bottom of the recipient list.

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

This is just the combination of the previous issue and the next issue.

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.

Indeed, that should be addressed - we should be able to copy multiple recipients when reading messages as well. For now it's possible by viewing the source and copying the text from there... :-(

A few final points:

  • One line/field per recipient means you can order your recipients either by category (To/CC/BCC), but also in other ways (e.g. alphabetically), while one line per category forces an ordering.
  • I would have liked to see better real-estate use by making it possible for users to indicate they want to add recipients in another column, splitting the recipient list section of composition window vertically (but not right from the get-go so it doesn't look like a bingo card).
  • A one-line-per-category preview of the recipients would also be nice, but not as the only way to enter them.
  • aleca - putting aside the fact that I don't like the basic notion, it seems nicely implemented. I don't mean to disparage your great work!

100 comments....yay...
Thanks for your feedback Eyal, but it's missing completely the scope of this patch.

Strongly disagree with this initiative as now stated; and - this is a key aspect of Thunderbird UI which merits a wider surveying of users IMHO.

The proposed UI with a thorough explanation of the various usability features was released here: https://thunderbird.topicbox.com/groups/ux/T37151b06e9361fac/work-started-on-redesign-recipients-address-fields-to-cc-bcc-etc

It was reviewed and approved by the core team and the technical director, and the exploration of this solution was initially introduced 7 months ago in bug 1535503.

All your points and issues are mostly related to the way you're personally used to use the compose message, how you feel comfortable, and the workarounds you learned with time to overcome some issues.
Those are not an indication of good usability and intuitive UI.

The proposed solution introduces many usability improvements, and helps streamlining the UI for multiple scenarios, while the current implementation comes with too many "IFs" and "BUTs", and it doesn't scale gracefully.

I'm sure you'll have many further points, so please keep writing in the topicbox thread and let's limit this bug to code related feedback.
Thanks.

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

All right, this is starting to get into a pretty good shape.
Here's the full list of what's working:

  • Pasting multiple addresses will be converted into pills.
  • Pills have focus.
  • Double Click or hit Enter on a Pill turns it back into an editable autocomplete input.
  • Hitting Enter after editing a pill will update the values and switch back to a pill.
  • Hitting Escape while editing a pill will revert to the previous value.
  • Backspace from the main autocomplete input selects the last pill.
  • Delete or Backspace while a pill is focused deletes it.
  • Navigate all the pills with Tab and Shift+Tab.
  • Warning colour if an address is valid but not in the address book.
  • Error colour if an address is not valid.

What's missing

  • Add the other rows through links.
  • Links will be populated based on user settings (Reply-To, Followup-To, etc).
  • Ability to collapse/expand long list of pills.
  • Select multiple pills at once with the usual various shortcuts.
  • Drag&Drop one or multiple pills within the same container to reorder.
  • Drag&Drop one or multiple pills between various containers.

More things to code

  • Update all the tests
  • Generate pills when forwarding/editing an email
  • Enable context menus for the pills like in the email header.

I'm sure I'm missing a million of other things, but slowly, step by step, this is getting pretty.

Attachment #9100374 - Attachment is obsolete: true
Attachment #9101029 - Flags: feedback?(richard.marti)
Attachment #9101029 - Flags: feedback?(paul)
Attachment #9101029 - Flags: feedback?(mkmelin+mozilla)

(In reply to Alessandro Castellani (:aleca) from comment #102)

  • Backspace from the main autocomplete input selects the last pill.

Hitting Backspace in such situation shall delete the last pill (1) not select it... pressing left arrow (or shift+tab) instead would select it...

That would seems a more natural and expected behaviour from Backspace key...

Alternatively (or in addition) if it was a pill just converted, hitting Backspace shall move back the pill in edit mode (2)... with the pointer set after last character... it would act like erasing last space comma or semi-colon that triggered pills conversion... but only if it was just last edited/converted... otherwise behaviour (1) may prevail...

Just a thought...

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.
Attachment #9101029 - Flags: feedback?(mkmelin+mozilla) → feedback+
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.
Attachment #9101029 - Flags: feedback?(richard.marti) → feedback+
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.)
Attachment #9101029 - Flags: feedback?(paul) → feedback+

It is great to see improvement in this bug. I'd strongly recommend installing MRC Compose for comparison. Features of MRC Compose that help a lot in "more than one address per line" mode:

  • Default text that informs users of how to input addresses.
  • Ability to show/hide CC, BCC, Reply-to and other fields.
  • Counter of recipients on the left of the corresponding field (helps to see if one has double paste a very long list of recipients)
  • Ability to configure which address books are searched (LDAP is too slow for me, but sometimes I need to enable it)
  • Sorting criteria (Popularity!)

(In reply to Alessandro Castellani (:aleca) from comment #102)

  • Select multiple pills at once with the usual various shortcuts.

It will be useful if keyboard shortcuts will work exactly as if the list of pills were words separated by ",":

At start of a pill:

  • backspace: convert the previous pill to text, pressing backspace again will start deleting text
  • ctrl-backspace: delete previous pill
  • ctrl-del: delete next pill
  • left-arrow: convert the previous pill to text and place the cursor under the last letter (in case I did a typo while writing the address)
  • ctrl-left-arrow: convert the previous pill to text and place the cursor at the start of the pill
  • ctrl-shift-left-arrow: select the previous pill (one should be able to do this several times to select several pills, like selecting several words)

In the middle of the text of a pill:

  • home-end: move to start/end of pill
  • ctrl-left/right-arrow: move between words (as usual)
  • "," close the pill, start a new one

At any point:

  • ctrl-home: convert the first pill to text and move the cursor under the first letter.
  • ctrl-end: convert the last pill to text and move the cursor under the last letter:
  • shift-ctrl-home: select all pills from current position to the first one (thus, end -> shift-home selects all pills, the Ctrl+C/X copies/cuts all addresses, separated by commas)
  • shift-ctrl-end: select all pills from the current position to the last one
  • Ctrl-Z undo last edit (undo delete pill/s, undo typed pill/s)

I'm sure you get the idea. This is the standard navigation/editing shortcuts, just that pills are treated as whole words unless they are being edited. Otherwise, navigating/editing many addresses will become a pain and all the benefits of having addresses in a single line (like MCR Compose does) will be lost.

(It would be nice to add to the whish-list to have X and contact photos in the pills like these chips here: https://material-ui.com/components/chips/ )

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

More improvements to the recipient pills.

This updated patch brings lots of accessibility improvements and various fixes.

  • You can navigate through the various pills with the arrow keys other than TAB and SHIFT+TAB.
  • Added the X icon to delete pills.
  • Fixed the email address re-appearing after a pill was created.
  • Fixed a couple of conflicts with the autocomplete while editing pills.
  • Improved UI and colours between the different states.

I have another WIP patch that adds the various CC, BCC, etc, almost ready to be uploaded, but I need a bit more time to fix a couple of quirks.

Attachment #9101029 - Attachment is obsolete: true
Attachment #9106455 - Flags: feedback?(richard.marti)
Attachment #9106455 - Flags: feedback?(paul)
Attachment #9106455 - Flags: feedback?(mkmelin+mozilla)
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?
Attachment #9106455 - Flags: feedback?(richard.marti) → feedback+

And I forgot to write that the text box is taller than the From menulist and the subject text box. Maybe reduce the space above/below the pill to make it less tall.

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.
Attachment #9106455 - Flags: feedback?(paul) → feedback+

Thanks all for the positive feedback.
Here are some notes regarding what your wrote.

(In reply to Richard Marti (:Paenglab) from comment #110)

the delete button inside the pill has a uneven border around it. On the right it is wider than top/bottom.

This should be fixed in the upcoming patch, but anyway I'm not super focused on pixel perfection across OSs, so something will definitely look not aligned until the final patch.

when I edit a pill (not easy to find without a context menu :) ) it expands and shifts the following pills to the right.

Fixed in the upcoming patch, the width of the editing area should respect the length of the pill.
I'm also adding a context menu with the "Edit" item. I was also thinking, would be worth considering showing a pencil icon on hover? So the user can click that to edit the address? More discoverable than a double or right click.

(In reply to Paul Morris [:pmorris] from comment #114)

Double-click to edit a pill, then click to put the cursor elsewhere, the pill stays in 'editing mode'. ..Seems like it should exit editing mode when
it loses focus? (Probably just still to do.)

I'm not sure about this as it might be super annoying for the user. Think about the scenario where you're writing an address, and you need to copy it from another window, or maybe you click somewhere else to view another email and confirm the address, and the pill reverts back losing your temporary editing.
Maybe I can implement a condition in which if the email address is valid while losing focus, the pill reverts back, but if it's not, it remains in edit mode.

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?

This is an annoying limitation I'm not sure how to overcome as a wider input is necessary for the autocomplete panel. That panel is as wide as the input field, so we can't shrink it down too much otherwise the autocomplete list can get unreadable.

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.

That'd be nice, but I'm not sure it's possible with the current autocomplete field. Maybe for the future.

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?

I'm not against this, I'll let Magnus decide for this matter.

Regarding code reviews

Indeed, there are a lot of quirky decisions and repeated code, but that's normal for now.
For now, I'm more interested in making this work and have a working patch for the entire email sending workflow.
Lots of things are probably useless and redundant, so apologies if I'm giving you stomachache :P

Stay tuned for another patch.

Think about the scenario where you're writing an address, and you need to copy it from another window, or maybe you click somewhere else to view another email and confirm the address, and the pill reverts back losing your temporary editing.

Ah, good point. Suggestion withdrawn. That is too bad about the width of the autocomplete panel.

Exciting stuff happening here, thanks everyone, and special thanks to Alessandro for taking on the beast!

1.) Could we fine-tune the click pattern for "edit pill" & friends a bit?

Single-click on pill: Select pill (select more pills with Ctrl+click/shift-click)
Single-click on selected pill: Edit pill ***
Double-click on pill: open associated contact (create new contact if not yet associated)

Rationale: Exploit users' habitual click-patterns from other, similar tasks, e.g. selecting and renaming files in Explorer (ux-consistency).

*** "Edit pill" probably refers to a direct, one-time edit, right? As opposed to changing a possibly associated contact in the address book.

2.) I'm not so sure yet about the best behaviour for outside clicks while editing pill (leaning towards exiting edit mode), but definitely we can't have a situation where multiple pills are in edit mode, as was pointed out above.

3.) Thunderbird users have been with neat, vertically aligned addresses forever; I'd think that for many users this may have become more than habit formation, they might actually appreciate this as a distinctive feature. The UX is poor not mainly because of the vertical alignment, but because of technical design and behaviour shortcomings, e.g. that we can't batch-change multiple addresses from To to BCC (in fact, we can't even select and copy multiple addresses). Also note that some of the behavioural shortcomings have been addressed in the current release version, e.g. recipients can now be navigated with cursor up/down, click to select single recipient has been implemented (pill-style), DEL and Backspace now allow controlled and predictable sequential deletion of multiple recipients with 2 keystrokes each.

I recommend:

  • allowing some way of vertically aligning/sorting/etc. recipients in the new pill scheme of things (not doing so may significantly irritate existing users). This might be as "simple" as having a "legacy view" which only allows one pill per line. As others have suggested, we should try to separate views from content/roles, and explore creative variants like multi-column views of pills.
  • to ease the transition for existing users, allow a two-click (or single-click) way of changing the recipient type of each pill (contextual action, hover menu etc.).
  • addressing the existing UX shortcomings in the pill scheme (provide multi-select and allow multi-selection-actions like mass-changing recipient type, mass-deletion, mass-copy etc.

I'm aware that some of these might require an incremental approach, but we should ensure now that the new design allows for such.

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

All right, here's another big improvement.

Keyboard navigation

  • Now you can navigate between pills with the arrow keys and tab.
  • If you hold CTRL, you can select multiple pills both with the click and the arrow navigation.
  • CTRL+A will select all the pills if one is already selected (I need to add this to the input field as well).
  • You can delete multiple selected pills.

Context Menu

  • Pills have context menu which allow to edit, delete, and copy the email address.

Cc, Bcc, etc.

  • The other fields are there and can be revealed by clicking on the label.
  • You can hide the field back, and get prompted with a confirm dialog if the field has addresses in it.
  • The header area grows to accommodate all the pills and fields, but as soon as you resize it, the area turns scrollable.

What's missing

  • Making this thing actually work and send emails.
  • Populate the fields when forwarding an email.
  • Add Drag&Drop for the pills.

As usual, the overall code is very dirty, with lots of repetitions and some nasty solutions. I'm doing it this way for the sake of having a fully working prototype which we can later iterate to optimize.

The next patch I'll upload should come with everything currently missing, and we can start the review process.

Thank you again everyone for the insightful feedback and your patience on waiting on this monolith.

Attachment #9100375 - Attachment is obsolete: true
Attachment #9106455 - Attachment is obsolete: true
Attachment #9106566 - Attachment is obsolete: true
Attachment #9106567 - Attachment is obsolete: true
Attachment #9106455 - Flags: feedback?(mkmelin+mozilla)
Attachment #9107655 - Flags: feedback?(richard.marti)
Attachment #9107655 - Flags: feedback?(paul)
Attachment #9107655 - Flags: feedback?(mkmelin+mozilla)

Hi, I'm the developer of MRC Compose.

Alessandro, from what I saw with screenshots, you're doing a great job ! Thank you to take care of such an important point in Thunderbird.
Are you ok if I try to integrate some parts of your prototype (ideas/code) in MRC Compose ? It's something I wanted to do from the beginning, but it requires so much low-level handling that I always postponed it.
Would you like to be notified of any progress I may make ?

Cheers,
Michel

PS: MRC Compose is now compatible with TB68 and can search in Cardbook !

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.
Attachment #9107655 - Flags: feedback?(paul) → feedback+
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.
Attachment #9107655 - Flags: feedback?(richard.marti) → feedback+

(In reply to Paul Morris [:pmorris] from comment #120)

If I may suggest as an end-user...

  • 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?

Hiding Cc,Bcc,etc... shall not remove content in the first place... it should just hide it... addresses shall only be deleted if delete action is triggeref by end-user.

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

For me a pill in edit mode is just a pill converted back to simple editable text (no pill anymore), as it was before it became a pill. It would be much simpler behaviour in concept. We should try to keep To,Cc,Bcc as close as possible to editable text fields, the pill system being just a visual and management aid.

Hitting backspace would simply erase text... if text become empty and you keep backspacing, I would expect previous pill (if exist) to convert back to text, and characters to start being deleted again...

Similar behaviour with the delete key.

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

If pill in edit mode become just text, if empty text I would expect the pill to no longer exists (effectively deleted). You would just end up with curser between two pills. Therefore hitting enter having no effect an empty text being not a valid email address to convert back into pill.

In a similar way, if I place cursor between two pills and hit Backspace (or Delete), it would convert previous (or next) Pill into editable text that you can further delete (per character) or amend... in a very similar way to editing a simple text...

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

A bug it seems... hitting tab shall shall move selection to next tab. Previous selected pill shall remain selected only if shift+tab wss pressed together.

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

If pill is convert back to text (edit mode) I would expect arrows to only allow navigation within the editable text.

Though one could imagine that if you reach start (or end of text) and keep pressing left arrrow (or right arrow):

  • if edited text email address is valid, it would convert the text into a pill, and set the previous (or next pill) into edit mode
    OR
  • if not valid, do nothing (just raise alert that address is invalid and must be valid)
  • 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.

For me editing mode should just be text mode... no longer a pill... therefore there should be no need for such checkmark... just nee to hit Enter to convert text back into pill (or via click outside editided text).

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

Again editing mode shall just be text mode.

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

As edit mode is just text mode it allow to enter (or edit) multiple addresses at once... especially handy when copy/pasting multiple addresses from wherr else...

Though I would expect to only be able to edit one pills at a time after convertion not multiple unless they are next to next...

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

Another patch, another iteration.

What's new

  • Now you can CTRL+C and CTRL+X on the pills to copy or cut them, it works for single or multiple selected pills at once.
  • The various selection interactions should have been fixed.
  • Add addresses from Address Book has been fixed.
  • Hitting Enter on and an empty pill while editing will delete that pill.

Pills interactions

  • If you're editing a pill and you keep hitting backspace or delete the pill WILL NOT be deleted. This is to prevent accidental and unwanted deletion while clearing out wrong addresses.
  • While in editing mode, the arrow keys won't allow switching from a pill to another. This is to prevent accidentally leaving the pill which it can be extremely annoying. Also, hijacking the arrow keys inside an input field is not great.

Alerting the user when removing a row with pills in it
Yes, we should absolutely prompt the user with a confirm, and we should clear that row if the user decides to remove it.
Imagine the scenario where the user is forwarding an email with 100+ addresses inside the CC container. The user deletes that container because it doesn't need it and starts editing the content. Then, he realizes he needs to CC that email to another contact and reopens the CC container, finding once again the 100+ addresses he though he deleted.
The very own X button, which implies the deletion of a row, can't trick the user by not actually deleting the content within.
It's linear, makes more sense, and it avoids us developers doing a check on submit like "the CC field has 100+ addresses, but it's hidden, so don't use them".

Since this patch is getting pretty big, I won't implement the Drag&Drop feature in here, but I will do that as a follow up bug after this first iteration lands on daily.

Attachment #9107655 - Attachment is obsolete: true
Attachment #9107655 - Flags: feedback?(mkmelin+mozilla)
Attachment #9109194 - Flags: feedback?(richard.marti)
Attachment #9109194 - Flags: feedback?(paul)
Attachment #9109194 - Flags: feedback?(mkmelin+mozilla)
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.
Attachment #9109194 - Flags: feedback?(richard.marti) → feedback+

Looks good. Here are a few new things I noticed, but nothing that should block this from landing, just potential follow-up polishing stuff.

  • Maybe add "cut" to pill context menu.
  • After cutting and pasting a grey background, in-address-book pill the background was orange on the second instance, so not recognized as in the address book.
  • Should you be able to tab to the "X" button for removing Cc/Bcc/Reply-to row? Currently you can't tab to them.
Attachment #9109194 - Flags: feedback?(paul) → feedback+
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
Attachment #9109194 - Flags: feedback?(mkmelin+mozilla)

(In reply to Richard Marti (:Paenglab) from comment #124)

The .address-container doesn't work well with dark theme, can be fixed later.

Ah yes, I will definitely need your expertise to be sure everything looks top notch in dark mode.

Mailing lists pills are coloured red like invalid addresses.

This should have been fixed in the upcoming patch.

(In reply to Paul Morris [:pmorris] from comment #125)

Maybe add "cut" to pill context menu.

Done.

After cutting and pasting a grey background, in-address-book pill the background was orange on the second instance, so not recognized as in the address book.

Fixed.

Should you be able to tab to the "X" button for removing Cc/Bcc/Reply-to row? Currently you can't tab to them.

Good call, done.

(In reply to Magnus Melin [:mkmelin] from comment #126)

I don't particularly like how the Cc Bcc Reply-to Newsgroup Followup-to field names are lined up under the To line.

Indeed, that's not an optimal placement. I'm experimenting with other locations, one of which is inline the To field, right aligned.
Also, I'm thinking about "hiding" the less common labels behind a dropdown menu. I'm not sure yet, still trying to find the best solution.

Also we'd not want to have the Newsgroup and Followup-To ones there showing for all mails.

That shouldn't be the case as I've updated the code in the hideIrrelevantAddressingOptions() to not show those fields based on the account type.

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

That works for me, can you elaborate on that? If you write a new address and you hit Enter, what happens?

And commas between addresses should work.

That works for me as well. If I write multiple addresses separated by a comma and then hit Enter, all those addresses are converted into pills.

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

It never had a top border. It has always been the bottom border of the field above.
Moving the Cc, Bcc, etc, labels to the right should fix that visual "issue".

I'm still struggling a bit with the email submission and the automatic fill-up of the fields on replied or forwarded emails.
Those methods are tightly coupled with so much stuff that it's giving me a hard time.

Sorry for the delay. A new patch ready for review will be uploaded ASAP.

Try this: add an address with autocomplete, then try to add an address that's not in the addressbook.
You shouldn't have to press enter either. Once you add a comma it's the next address.

Ah, I see, that's a UX issue.

In order to solve this I've implemented the creation of the pill when the comma character is pressed, or onblur of the input field with a valid email address.
You're right, users shouldn't be force to hit Enter or Tab to create a pill.

Thanks for the detailed explanation.

(In reply to Alessandro Castellani (:aleca) from comment #129)

Ah, I see, that's a UX issue.

In order to solve this I've implemented the creation of the pill when the comma character is pressed...

Would semi-colon ';' be usable as email address separator? Basically having same effect as comma ','? This may bring some flexibility especially for copy/paste, if not directly by end-user possible habits...

When To, Cc, Bcc are empty would there be any hint text in the field to indicate to 'Enter email addresses comma or semi-colon separated' as a tip? May be handy for new or accustomed users ;-)

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

All right, I think it's time to start the first of (I suspect) many reviews.

The whole workflow works now.
Sending emails work.
The various forward, reply, etc, work.
Writing to an entire list work.

Old methods
I started removing the unused methods of duplicating rows and checking for dummy_rows, but I think I'll stop for now as some of those methods are tightly coupled with the Address Book methods.
I will take care of those in a follow up clean-up patch.

UI and UX
Currently not at a great state, but if it's OK for you Magnus, I'd like to focus on landing a working and usable patch, with all the tests green, to then follow up with a UI-centric patch on a different bug.
Having the working prototype will help me to define better what we need and how it should be improved.

Tests
All the tests are failing, obviously, but I will hold off on those until this patch gets an r+.

What's missing
Dropping email addresses on a field is broken, but it should come in the next patch.

Thanks everyone for reviewing this beast.

Would semi-colon ';' be usable as email address separator?

Done :D

Attachment #9109194 - Attachment is obsolete: true
Attachment #9110172 - Flags: review?(mkmelin+mozilla)
Attachment #9110172 - Flags: feedback?(richard.marti)
Attachment #9110172 - Flags: feedback?(paul)

Re semi-colon: I think you shouldn't. Pasting will do the right thing nowadays. Eventually we want to support group syntax and I wouldn't want to have users wrongly trained to use it.

(In reply to Magnus Melin [:mkmelin] from comment #132)

Pasting will do the right thing nowadays.

You mean it would convert semi-colon into commas and convert addresses into pills on the fly?

Eventually we want to support group syntax

How using semi-colon as separator prevent group syntax?

Is there an RFC or other reference that indicate/recommand semi-colon as email separator for group/distribution list?

Is your idea to be able to create an auto-completion group by just typing email addresses separated by semi-colon?

and I wouldn't want to have users wrongly trained to use it.

You may be right on this, maybe then accept it as a valid separator but don't encourage it...?

This would bring flexibility - some email client use semi-colon as separator or accept it as alternative to coma...

Unless of course you have other plan/idea...

Just try copy-pasting something with semi-colon and you'll see we make that right (already on release).

Group syntax is RFC 5322, see "Group Addresses". Like "Family: Foo <foo@example.com>, Bar <bar@example.com>"; "Friends: Joe <joe@example.com";

Could someone kindly offer a try build? TIA.

(In reply to Thomas D. from comment #135)

Could someone kindly offer a try build? TIA.

I'll do that later once the majority of the tests are fixed, and a couple of missing features are added to the patch.
Probably before the end of this week.

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.
Attachment #9110172 - Flags: feedback?(paul) → feedback+
Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

Here's another patch with more improvements:

  • Address Book lists are now recognized and properly styled.
  • Drag and Drop of addresses from the Address Book sidebar to the field works, as well as dropping them on top of the labels.
  • All the Address Book methods have been moved to the dedicated JS file. The addressingWidgetOverlay.js now handles only the compose dialog methods.

Here's a try run with all the builds:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2bb73e8838882784652dd6e5d91640d5474bb9c2

Be aware that:

  • All the tests will fail.
  • The UI is not final and many improvements will come in the next days.

Please, test it and report issues!
I'm sure this patch will introduce many many many regressions, so, ignoring the fact that this bug is gonna probably reach 200+ comments, I'd like to ask anyone interested to grab a build, test it, and report any issue or bug you stumble upon.

Attachment #9110172 - Attachment is obsolete: true
Attachment #9110172 - Flags: review?(mkmelin+mozilla)
Attachment #9110172 - Flags: feedback?(richard.marti)
Attachment #9110415 - Flags: review?(mkmelin+mozilla)
Attachment #9110415 - Flags: feedback?(richard.marti)

I'm testing the patch, and it works really well! I like the UI design, too.

A few minor glitches that I noticed and that should be easy to fix:

  • It's too happy to make entries without email address. E.g. "where.com" results in "where.com <>" in a red pill (where.com <> [x]). I don't think such entries with syntactically invalid email address should be created at all. As pill, they look valid, even if red. And as pill, they are hard or impossible to fix. I think they should just stay text until they are fixed.
  • It's a little to quick to end the entry and create a pill. E.g. on my keyboard, the comma , (which creates a new pill) is right next to . (which I need for an email address), and I've repeatedly accidentally create a new pill when I didn't want to. Once it's a pill, it's hard to fix. Not sure how I'd solve that.
  • Backspacing into a pill should not delete the pill, but re-enter edit mode and delete only the last characters. Probably similar for other key strokes like arrow keys and others.

All these are minor "editor rules" issues. Technically, this works really well, from what I'm seen in my quick test. Good job!

Style (sorry for the bike-shedding):

  • I don't like the colors, esp. the yellow. I'd stick to a more conventional grey. Color doesn't fit here.
  • The contrast between foreground color and background is too low.
  • If you use red for error (I'd rather not create a pill at all in this case, see above), make the red much more flashy.

(In reply to Ben Bucksch (:BenB) from comment #139)

  • Backspacing into a pill should not delete the pill, but re-enter edit mode and delete only the last characters.

Please don't. Backspacing onto a pill should delete the pill, and consecutive backspaces should delete pills backwards one by one. Backspacing to delete character-wise only makes sense for a single pill in edit mode. It's highly unlikely that user needs to edit more than one or two odd pills in a delete-per-character fashion, whereas it's much more likely that recipients need to be removed. We have just introduced the distinction in bug 1527547 to improve the UX-efficiency of removing recipients, and it works well: For editing a single recipient, backspace deletes characterwise until that recipient (display name and email address text) is completely deleted. Pressing backspace again on an empty recipient row will remove the row and start removing recipients one by one. I haven't tried pills yet, so I'll comment again after that, but I believe wrt keyboard deletion pills should behave similar to the current release version behaviour, where my patch of bug 1527547 has created a precursory pill-style experience.

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.
Attachment #9110415 - Flags: feedback?(richard.marti) → feedback+

(In reply to Ben Bucksch (:BenB) from comment #139)

It's too happy to make entries without email address. E.g. "where.com" results in "where.com <>" in a red pill (where.com <> [x]).

Good call. Text won't be converted into pills if the address is not valid.

It's a little to quick to end the entry and create a pill. E.g. on my keyboard, the comma , (which creates a new pill) is right next to . (which I need for an email address), and I've repeatedly accidentally create a new pill when I didn't want to. Once it's a pill, it's hard to fix. Not sure how I'd solve that.

If this happens, you can easily edit the pill by using the left arrow key to select the pill, then press Enter to enable edit mode, and now you can edit the pill address.
Anyway, this problem should be solved because even if you hit , accidentally, the pill won't be created because the string doesn't contain a valid email address.

(In reply to Richard Marti (:Paenglab) from comment #142)

  • The fields are no more vertically aligned in columns because the text fields begins directly after the To: label.

The UI is not polished at all and I'll need to iterate on it a bit more to find a better overall solution.

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.

We found out that the patch was outdated, so these issues have been fixed in the most recent patch.


I have a little problem with character encoding. When the pill gets created from an address with special characters (eg. ö) those characters turn into a question mark.
That's pretty odd as I'm simply storing those values into a label inside a custom element.
Does anyone know if custom elements need an encoding attribute, or the UTF-8 charset needs to be specified somewhere?

I'm attaching a screenshot of the issue right after this message.

Thanks everyone for the feedback and the help.

Attached image wrong-charset.png (obsolete) —

Here's the outcome of an address saved into a label value.

Please also check with these 4 characters: ŐőŰű

https://www.compart.com/en/unicode/U+0150
https://www.compart.com/en/unicode/U+0151
https://www.compart.com/en/unicode/U+0170
https://www.compart.com/en/unicode/U+0171

For an test email address, please try:
ÁáÉéÍíÓóÖöŐőÚúŰű, Testing <ÁáÉéÍíÓóÖöŐőÚúŰű@example.com>

What do you think?

Thank you

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
Attachment #9110415 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

All right, here's an updated patch with everything that was highlighted by Magnus.
I also fixed the wrong charset when dealing with special characters.

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

Yeah, I'm not happy with the name either.
That is the ID of the autocomplete html:input where the pill was generated from. I need that to ensure keyboard navigation and focusing on it if all the pills are delete.
Which name would make more sense? I added a comment but I'm not sure it's clear.

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

True, sorry about that. I ended up using the control attribute to reference the html:input. I didn't need a custom attribute at all.

Onto fixing all the failing tests.

Attachment #9110415 - Attachment is obsolete: true
Attachment #9110641 - Attachment is obsolete: true
Attachment #9111018 - Flags: review?(mkmelin+mozilla)

Had to interrupt my holiday to check this out. Looking good :-)

Oops, spoke to early. Try this: Create a Cc field. Summon the addressing side bar or open an address book. Try dragging an address to the Cc field :-( - I don't want to mention that this malfunction could be quite fatal when a Bcc address is added to the To field.

(In reply to Jorg K (GMT+1) (PTO to 1st Dec 2019, not reading bugmail) from comment #149)

Oops, spoke to early. Try this: Create a Cc field. Summon the addressing side bar or open an address book. Try dragging an address to the Cc field :-( - I don't want to mention that this malfunction could be quite fatal when a Bcc address is added to the To field.

Ah, I see why this is happening.
Most likely you didn't drop the address exactly on the label, which it doesn't grow 100% of the column's width, and by default if a drop area is not recognized it falls back to the To field.

So, it's a matter of CSS and ensuring the the entire address row is accepted as a correct drop area, and not just the label or the input field.
Thanks for testing it and reporting this.

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

I started tackling the majority of the failing tests and I'm moving forward towards a green try run.

While updating tests, I was able to nail down some quirks and unexpected wrong behaviours, hopefully with the remaining tests to fix, I will lower the number of potential regressions to a manageable amount.

Here's a fresh try-build for anyone interested in testing it: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=16ba5865ee16850d0f618d7e49c900a83ac6b126

Attachment #9111018 - Attachment is obsolete: true
Attachment #9111018 - Flags: review?(mkmelin+mozilla)
Attachment #9111490 - Flags: review?(mkmelin+mozilla)

I was able to nail down some quirks and unexpected wrong behaviours

Including fixing comment #149? I was dragging onto the field, not the label, BTW.

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

This is ready for a full review as all the tests on Linux have been fixed.
I've also been using daily with this patch applied all day to send emails and I didn't stumble upon any issue...so far.

Here's the try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=37715c9cc02dcb65deeb5c40b1f40a728a9c6a94

There are a couple of tests failing on macos, which I will fix tomorrow since I have to update the UI and dark theme variation for that platform.

Attachment #9111490 - Attachment is obsolete: true
Attachment #9111490 - Flags: review?(mkmelin+mozilla)
Attachment #9111838 - Flags: review?(mkmelin+mozilla)
Attached patch 440377-ui-solution1.patch (obsolete) — Splinter Review

Here's a patch to apply on top of the original one to implement a slicker and more refined UI.
This is good to test only on Linux for now as I will be tackling macos issues tomorrow.

Attachment #9111839 - Flags: ui-review?(richard.marti)
Attachment #9111839 - Flags: feedback?(paul)
Attachment #9111839 - Flags: feedback?(mkmelin+mozilla)

Repeating: Have you addressed comment #149? (see comment #152).

(In reply to Jorg K (GMT+1) (PTO to 1st Dec 2019, not reading bugmail) from comment #155)

Repeating: Have you addressed comment #149? (see comment #152).

Yes. Sorry, I meant to answer your question in my previous comment but I forgot.
I tested it on Linux and macos and it works. Now, it shouldn't matter where you drop the address on the entire row, the field should always be the expected one.

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.
Attachment #9111839 - Flags: ui-review?(richard.marti) → feedback+
Attached image linux-UI-27.11.png (obsolete) —

Issues I found, not fatal and some could be done in follow-up patches/bugs.

  • I think, the drop-shadow of the boxes is too dark. First when I saw it, I thought there must be a popup below that doesn't open correctly ( I was in the "From" menulist). With dark theme the drop-shadow is very hardly visible.
  • It seems depending on the recipient pill's length it calculates if a second pill will match on the same line. If not it expands already the text box to a second line (see screenshot Bcc field) which looks a bit weird when there is still enough space to enter an address. I think, it should only expand when the cursor touches the box on the end.
  • It would be great when we have a feedback, like the mouse hover feedback, when I try to drop an address on the "Add Cc", "Add Bcc" etc. buttons.
  • The text boxes grow by 1px when the first address converts to a pill (no pill in box, 29px, with pill 30.33px).
  • Dragging the pills to re-order/move to an other box isn't possible. For me okay for a follow-up bug.
  • Also for a follow-up bug: maybe add a "Edit Contact" menu item like we have on the message header address.
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.
Attachment #9111839 - Flags: feedback?(paul) → feedback+

(In reply to Richard Marti (:Paenglab) from comment #158)

Thanks for the feedback, I'll fix what you reported.
Here some thoughts on what you suggested.

It seems depending on the recipient pill's length it calculates if a second pill will match on the same line. If not it expands already the text box to a second line (see screenshot Bcc field) which looks a bit weird when there is still enough space to enter an address. I think, it should only expand when the cursor touches the box on the end.

That's an issue Paul pointed out before, and I agree that it looks awful. The problem is that I need a min-width for the input field in order to have a decently sized autocomplete popup. If I let the main input field shrink to fit inline with previous pills, the autocomplete popup will be too small since it inherits the width from the input field.
I'll see if I can force the popup to use the entire row container (which is styled to look like an input field but is not) as a width reference.

It would be great when we have a feedback, like the mouse hover feedback, when I try to drop an address on the "Add Cc", "Add Bcc" etc. buttons.
Dragging the pills to re-order/move to an other box isn't possible. For me okay for a follow-up bug.

Yeah. Both these edits will come in a follow up bug where I'll focus only on implementing a proper Drag& Drop experience.

(In reply to Paul Morris [:pmorris] from comment #159)

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.

Uh oh, that's not supposed to happen, and it's definitely a bug.
I have a check in place to see if the header area grows taller than 2/3 of the window height whenever a pill is created, and if it does it should stop growing and add overflow scroll.
I'll fix that.

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.
Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

Here's another patch with a fix for the issue reported by Paul.
The try run is looking pretty green, except for a couple of failures I'm not sure how to deal with: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d13eaf801f2df3b4d15e51bd0e1b3aaffe0a7055

X3 failure: security/manager/ssl/tests/unit/test_intermediate_preloads.js
How can I run this locally and debug it? It's part of m-c.

Z4 failure: That's a whole crash of the app happening on macos only. But when I test it locally I can't seem to recreate the issue and all the tests pass. Any advice?

Attachment #9111838 - Attachment is obsolete: true
Attachment #9111838 - Flags: review?(mkmelin+mozilla)
Flags: needinfo?(richard.marti)
Flags: needinfo?(paul)
Attachment #9112393 - Flags: review?(mkmelin+mozilla)

(In reply to Alessandro Castellani (:aleca) from comment #162)

X3 failure: security/manager/ssl/tests/unit/test_intermediate_preloads.js
How can I run this locally and debug it? It's part of m-c.>

./mach test security/manager/ssl/tests/unit/test_intermediate_preloads.js

Probably not related though

Z4 failure: That's a whole crash of the app happening on macos only. But when I test it locally I can't seem to recreate the issue and all the tests pass. Any advice?

Probably not from your patch.

X3 failure: security/manager/ssl/tests/unit/test_intermediate_preloads.js
Ignore. The person keeping the tree green should have filed a bug and fixed it. Started this morning, Thu, Nov 28, 06:06:07.

I see both X3 and Z4 happened in other try runs without my patch applied.
Thanks both for the info.

Flags: needinfo?(richard.marti)
Flags: needinfo?(paul)
Attached patch 440377-ui-solution1.patch (obsolete) — Splinter Review

And this is the updated UI patch with the following changes:

  • Light and Dark theme styled for Linux and macos (Windows currently missing).
  • Added shortcuts for macos.
  • Prevent input field from showing an empty blank row.
  • The autocomplete popup is attached to the row container and not the small input field.

I'm currently working on the other UI solution, which will come on a patch to apply on top of this.

Since the other UI variation only changes the location of the other recipient labels, I will not upload it until this one gets reviewed and approved in terms of colours and sizing.

Attachment #9111839 - Attachment is obsolete: true
Attachment #9111935 - Attachment is obsolete: true
Attachment #9111839 - Flags: feedback?(mkmelin+mozilla)
Attachment #9112429 - Flags: ui-review?(richard.marti)
Attachment #9112429 - Flags: review?(mkmelin+mozilla)
Attachment #9112429 - Flags: feedback?(paul)
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?
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.
Attachment #9112429 - Flags: ui-review?(richard.marti) → ui-review+
Attached patch 440377-ui-solution1.1.patch (obsolete) — Splinter Review

Added the Windows styles. Also removed a lot of no more used code. I also tried to align vertically the labels to the text inside the fields.

I had to move some code from shared to the platform files to not to have to revert styles we don't need?

On Mac I added the box-shadow: var(--focus-ring-box-shadow); to the boxes to make it look more like the default focus on Mac.

Attachment #9112673 - Flags: review?(alessandro)
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?
Attachment #9112393 - Flags: review?(mkmelin+mozilla) → feedback+

(In reply to Magnus Melin [:mkmelin] from comment #170)

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.

Can you elaborate on this? Sorry, but I don't think I understood what problem you encountered.
The comma is currently handled in 2 scenarios:

  • While typing, if you press comma, the test before gets converted into a pill.
  • When pasting multiple addresses separated by a comma, those get converted into pills on Enter or blur.

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

All right, too many people gave the same feedback on this, so I guess we can see how the users react on a pill exiting edit mode on blur.

::: mail/base/content/mailWidgets.js

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

Could you point me to an example? I don't think I got what you mean, sorry.

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

I moved the get pills(), get allPills(), and get allSelectedPills() methods to the AddressingWidgetOverlay.js file

@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()?

When the focus is on the html:input and the user hits CTRL+A I'm moving the focus on the latest pill present in the container.
All the available pills in the same row get the selected attribute, but the focus should move on one pill in order to handle the mass cut, copy, and delete actions.

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

The gMsgToRecipientElement variable is used a couple of times in the SwitchElementFocus() method to handle the focus via F6.
How would you suggest updating that method?

All the other points have been feedback.
I'm gonna merge the UI patch into this in order to have everything together into a single patch and avoid dealing with merging issues while updating the base patch.

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.
Attachment #9112673 - Flags: review?(alessandro) → review+

(In reply to Alessandro Castellani (:aleca) from comment #171)

(In reply to Magnus Melin [:mkmelin] from comment #170)

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.

Can you elaborate on this? Sorry, but I don't think I understood what problem you encountered.
The comma is currently handled in 2 scenarios:

  • While typing, if you press comma, the test before gets converted into a pill.

It doesn't seem to work if you autocompleted, get the pill, and then add a comma (doesn't trigger autocomplete when you write then). Also for cases you come back to fill in more addresses.

All right, too many people gave the same feedback on this, so I guess we can see how the users react on a pill exiting edit mode on blur.

I think that would be preferable yes.

::: mail/base/content/mailWidgets.js

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

Could you point me to an example? I don't think I got what you mean, sorry.

For the above mapping it means you'd set pill.value="foo" to get the label to be foo. Seems more readable to just have it be pill.label="foo" to set the label. So the rule would be ".emaillabel": "crop,label",.

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

I moved the get pills(), get allPills(), and get allSelectedPills() methods to the AddressingWidgetOverlay.js file

@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()?

When the focus is on the html:input and the user hits CTRL+A I'm moving the focus on the latest pill present in the container.
All the available pills in the same row get the selected attribute, but the focus should move on one pill in order to handle the mass cut, copy, and delete actions.

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

The gMsgToRecipientElement variable is used a couple of times in the SwitchElementFocus() method to handle the focus via F6.
How would you suggest updating that method?

Use document.getElementById to get it when needed.

On a more general note, the "Add Newsgroup" and "Add Followup-To" headers are now quite in your face, so to speak. They used to be much more hidden. I'm not sure what you have planned for them? The other UI alternatives you sent would have been nicer re that. As is, I think this will be confusing to many, who have no idea even what a newsgroup is.

For the pill styling, maybe they should be less rounded in the corners?

For newsgroups, the confirmation of removing the field does not show.

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

I implemented the full UI for solution 1 in this new patch, and fixed almost everything that was reported.
Here's a couple of follow ups:

It doesn't seem to work if you autocompleted, get the pill, and then add a comma (doesn't trigger autocomplete when you write then). Also for cases you come back to fill in more addresses.

Why would you type a comma after a pill is created? After an address is converted into a pill, it should be natural to just type a new address since the previously typed value was converted, and there's no need to write a comma between pills.
Sorry if I misunderstood what you wrote.

For the above mapping it means you'd set pill.value="foo" to get the label to be foo. Seems more readable to just have it be pill.label="foo" to set the label. So the rule would be ".emaillabel": "crop,label".

That's not really what I'm doing.
The pills structure is the following:

<pill>
  <label class="emaillabel"/>
  <html:input>
</pill>

So when I set pill.label="foo", which is what I'm doing when creating a new pill, the child <label> gets that attribute and adds it to its value:

<pill label="foo">
  <label class="emaillabel" value="foo"/>
  <html:input>
</pill>

Is this a wrong approach?

I removed the gMsgToRecipientElement variable. Do you want me to do the same for gMsgIdentityElement and gMsgSubjectElement since they're doing the same thing?

On a more general note, the "Add Newsgroup" and "Add Followup-To" headers are now quite in your face, so to speak. They used to be much more hidden. I'm not sure what you have planned for them? The other UI alternatives you sent would have been nicer re that. As is, I think this will be confusing to many, who have no idea even what a newsgroup is.

This patch comes with the proposed solution 1 applied.
I'm now working on a dedicated patch for solution 2, with the labels inline the To field so we can test both by simply apply the secondary patch on top of this.

I know you're not a fan of this solution, but I wanted to avoid working on solution 2 before the overall code and base structure was close to a somewhat final state. :)

For the pill styling, maybe they should be less rounded in the corners?

I recommend to leave it like that. The rounded border style is a pretty common solution in other email clients, and the fact that it's not used anywhere else in the software it makes the pill a more recognizable element and unique to an email address.

For newsgroups, the confirmation of removing the field does not show.

Fixed.

Attachment #9112393 - Attachment is obsolete: true
Attachment #9112429 - Attachment is obsolete: true
Attachment #9112673 - Attachment is obsolete: true
Attachment #9112429 - Flags: review?(mkmelin+mozilla)
Attachment #9112429 - Flags: feedback?(paul)
Attachment #9113340 - Flags: feedback?(richard.marti)
Attachment #9113340 - Flags: feedback?(mkmelin+mozilla)
Attached patch 440377-ui-solution2.patch (obsolete) — Splinter Review

And here's the patch with solution 2 for the UI.
This patch puts the most common extra recipients inline with the To container, and collects extra recipients inside a popup panel.

Richard, let me know if I messed something up, especially on Windows.
Cheers

Attachment #9113379 - Flags: ui-review?(richard.marti)
Attachment #9113379 - Flags: feedback?(mkmelin+mozilla)
Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

I had to rebase the patch and fix various merging issues with the latest updates that landed on trunk.

Attachment #9113340 - Attachment is obsolete: true
Attachment #9113340 - Flags: feedback?(richard.marti)
Attachment #9113340 - Flags: feedback?(mkmelin+mozilla)
Attachment #9113385 - Flags: feedback?(richard.marti)
Attachment #9113385 - Flags: feedback?(mkmelin+mozilla)

And also this one is rebased.
Here's also a try run, let's see how this one goes: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0c8944c58d1c7f43fbd6299ae93dec5f66ff6805

Attachment #9113379 - Attachment is obsolete: true
Attachment #9113379 - Flags: ui-review?(richard.marti)
Attachment #9113379 - Flags: feedback?(mkmelin+mozilla)
Attachment #9113386 - Flags: ui-review?(richard.marti)
Attachment #9113386 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9113385 [details] [diff] [review]
440377-recipients-pills.patch

Looks good to me.
Attachment #9113385 - Flags: feedback?(richard.marti) → feedback+
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.
Attachment #9113386 - Flags: ui-review?(richard.marti) → ui-review+

(In reply to Alessandro Castellani (:aleca) from comment #174)

Why would you type a comma after a pill is created?

I'd like the commas to be visible. Why you'd write it - well if you know commas separate emails, it's a natural thing to do so it shouldn't send you off to an error mode.

Is this a wrong approach?

Ah, I misread. Please ignore.

I removed the gMsgToRecipientElement variable. Do you want me to do the same for gMsgIdentityElement and gMsgSubjectElement since they're doing the same thing?

Sure, but feel free to do it in a followup.

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
Attachment #9113385 - Flags: feedback?(mkmelin+mozilla) → review-
Comment on attachment 9113386 [details] [diff] [review]
440377-ui-solution2.patch

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

I do like this better (didn't look much at the code yet)

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +257,5 @@
>  <!-- Headers -->
>  <!--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 extraRecipients.tooltip "More recipients">

"More addressing fields"
Attachment #9113386 - Flags: feedback?(mkmelin+mozilla) → feedback+

(In reply to Alessandro Castellani (:aleca) from comment #174)

Created attachment 9113340 [details] [diff] [review]
440377-recipients-pills.patch
I implemented the full UI for solution 1 in this new patch, and fixed almost everything that was reported.

This sounds like an interesting candidate for a try build to test UI1. I guess testing outdated try builds wouldn't help. Not sure how hard it is to create try jobs, but regular try builds for this sensitive change to the traditional recipients area would be very useful and appreciated.

(In reply to Thomas D. from comment #183)

This sounds like an interesting candidate for a try build to test UI1. I guess testing outdated try builds wouldn't help. Not sure how hard it is to create try jobs, but regular try builds for this sensitive change to the traditional recipients area would be very useful and appreciated.

Sorry, I forgot to paste the link.
I actually run a full try build pretty much every day for these updates.

macos and Linux try build for solution 1: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fffa69633898192ebc30dbc2646189db98c304ab

all platforms try build for solution 2: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0c8944c58d1c7f43fbd6299ae93dec5f66ff6805

(In reply to Alessandro Castellani (:aleca) from comment #177)

Created attachment 9113386 [details] [diff] [review]
440377-ui-solution2.patch
Here's also a try run, let's see how this one goes: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0c8944c58d1c7f43fbd6299ae93dec5f66ff6805

So I've finally had a chance to test this out for the first time (looking at UI-2).
Impressive it is, a fine work, and hard labor; thanks a lot Alessandro! :-)

As a general caveat, I believe that we should not underestimate the impact of this change to existing users who have developed their workflows in the current "vertical" recipient UI since time immemorial. Which should be an impetus to be as convincing as possible with a fine-tuned, flawless, maximally convenient, and value-added UX.

Here's some remarks/issues of my first-impression ui-review (with a special focus on keyboard operation)...

  1. Pls implement Shift+Movement keys for selecting multiple consecutive pills
  2. Pls remove Ctrl+Movement keys for selecting multiple consecutive pills, and allow selection of non-consecutive pills instead. Pls try keyboard selection methods in windows explorer; let's be consistent! For Ctrl+Movement keys, the current selection must be preserved, and we need a focus ring (only!) when moving to the next pill, which can then be selected (and de-selected) with Space bar.
  3. Pls distinguish the effects of hover vs. select. See windows explorer. Otherwise Ctrl+Left-click to select becomes a guessing game.
  4. Hand cursor? Is it a link?
  5. Per my comment 117, I still feel that: Double-click is non-standard and undiscoverable to edit the label of an object. Typically, single-click selects, another single-click edits, and double-click opens. Compare e.g. the behaviour of file name labels in Windows Explorer ;-)
  6. For windows users, pls implement F2 to edit the label (which is the Windows default shortcut for editing texty things, cf. renaming files, editing Excel cells, etc.)
  7. Behaviour inconsistency:
  • Type foo@bar.com, press Enter -> get cursor to continue (correct). vs.
  • Cursor onto existing pill {foo@bar.com}, press Enter to edit, Re-Type, press Enter -> still stuck on the pill. This should return the cursor to continue, just as it did for the first round of typing.
  1. Pressing Backspace or DEL on an empty pill in edit mode should remove the pill and just show cursor, similar to the current behaviour when pressing DEL or Backspace on an empty row (consider recipient rows a vertical precursory version of pills) - possible with event.repeat, see patch of Bug 1527547.
  2. Cursoring onto a single pill pretends to select it, but doesn't. Please do select. Just like... drumroll... Windows Explorer. We really want to use Ctrl+X and Ctrl+C on that single pill.
  3. Please allow me to enter |Johnson, Peter| even when I already have |Johnson, Jane| in my address book. Current: comma after a name which has a (single?) partial autocomplete match will create the pill prematurely: {Johnson}
  4. Do we need a permanent delete button on CC and BCC rows? I think it's enough to show the delete button when the row (or the CC label) is hovered or focused. We've just de-cluttered the recipient area, and the general trend in UX is hiding what's not immediately needed in the given context.
  5. Thou shalt not kill:
  • have a couple of pills

  • start typing another one:
    {pill-1} {pill-2} ... {pill-N} John D|

  • notice that you want Jane Doe instead, hold Backspace to correct what you just typed...

    BOOM! All pills gone. :-(( Please don't. Backspace must break/stop after deleting the unfinished pill. Only after releasing and re-pressing the backspace key, that's when we want to start deleting other pills. Use event.repeat as above in my point 8, see patch of Bug 1527547.

So much for now. It's a lot of work. Again, much appreciated! :-)

(In reply to Alessandro Castellani (:aleca) from comment #184)

macos and Linux try build for solution 1: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fffa69633898192ebc30dbc2646189db98c304ab

Unfortunately not testable for Windows-only consumers like myself :/

Thanks Magnus for the review.
I fixed pretty much everything except the originalInput, which now I'm defining as a scoped variable in the pill CE constructor.
I tried not to have it at all, but a pill needs to know from which html:input element was originated from to get attributes and proper focus handling between elements, and I couldn't figure out a way to achieve that without bleeding into the document from within the pill CE.

Also, for the getAllPills and similar methods, I think we can have those into a parent CE which wraps the entire recipients area, but I'd like to tackle that in a follow up patch. Would that be OK?

Thomas, thanks for the detailed feedback, I applied pretty much all those improvements, which were minor thanks to the somewhat modular approach I tried to give to this thing.

A couple of notes:

Per my comment 117, I still feel that: Double-click is non-standard and undiscoverable to edit the label of an object. Typically, single-click selects, another single-click edits, and double-click opens.

I think at the pill as a closed element, that's why the double-click makes sense for me, since you need to "open" the element to access the input field within. I see how it can be seen a bit of a learning curve, but I'd like to land the patch with this approach, and gather feedback from users on daily.

Please allow me to enter |Johnson, Peter| even when I already have |Johnson, Jane| in my address book. Current: comma after a name which has a (single?) partial autocomplete match will create the pill prematurely: {Johnson}

That's a bit of an annoying issue with the autocomplete. I'll see if I can do something about it, otherwise we'll need to deal with this in a followup patch.

What's missing

  • I'm trying to find a decent solution to all those setTimeout(), trying to avoid using them.
  • Implement the event.repeat to prevent unwanted deletions.
  • Fix the remaining couple of failing tests.

A new patch a new try run will come later tomorrow.

Side note
Many more improvements could be done to this patch, that's why for future additions and suggested edits, I will open dedicated follow up bugs I will deal with once this has landed.
This patch is already massive and pretty hard to review, and I'm sure a bunch of regressions will unfortunately come with it, so I think it's better to have something landing at a state we're pretty satisfied with in order to allow everyone to test it and keep iterating throughout smaller patches.

Cheers.

(In reply to Alessandro Castellani (:aleca) from comment #187)

Also, for the getAllPills and similar methods, I think we can have those into a parent CE which wraps the entire recipients area, but I'd like to tackle that in a follow up patch. Would that be OK?

Sure, but please add comments in the code too about this, and reference the bug to fix it.

(In reply to Alessandro Castellani (:aleca) from comment #187)

Thomas, thanks for the detailed feedback, I applied pretty much all those improvements

Awesome!!! Looking forward to your next Windows try build.

A couple of notes:

Per my comment 117, I still feel that: Double-click is non-standard and undiscoverable to edit the label of an object. Typically, single-click selects, another single-click edits, and double-click opens.

I think at the pill as a closed element, that's why the double-click makes sense for me, since you need to "open" the element to access the input field within. I see how it can be seen a bit of a learning curve, but I'd like to land the patch with this approach, and gather feedback from users on daily.

Agreed, thank you for sharing the rationale, now it makes sense. In fact, my postulate was inaccurate: double-click is default action (which may or may not be "open"), must match ENTER on keyboard, and our default action here is "Edit pill" - fair enough.

  1. So now we have:
  • Double-click on pill -> Edit pill
  • Enter on selected pill -> Edit pill
    What if you just add this as a complementary method:
  • Single-click on selected pill -> Edit pill (like mouse-renaming of files in Win Explorer)
    I'm suggesting this here (as opposed to followup bug) because I think it could significantly ease the transition from the current "editable text" recipients: Click once expecting to edit text -> pill selected (fail, but there must be a way!?). Stubbornly click again on selected pill -> pill in edit mode (success, even somewhat discoverable). In fact, that mimics the current behaviour exactly, as we're already auto-selecting recipients "pill-style" since my bug Bug 1527547. With the current iteration of the patch here, it may feel too hard/undisdoverable for users how to break that resistance of pills against being edited... ;-)
  1. As a side note (for followup), here's some ideas how to enable "Edit (address book) Contact":
  • Pill context menu: Edit Contact (vs. Edit Recipient)

  • Alt+Enter / Ctrl+I on selected pill -> Edit Contact (default shortcuts for "Properties"; try it in TB's contacts side bar)

    I'm hesitating a bit because I'm not sure about the correlation between a recipient email address and contact card(s) in the AB, which is currently flawed in the ancient design of TB's address book. If the correlation can be made in a predictable way which does not hinder the new AB design, the above should be implemented. Oh, in a followup bug...

Blocks: 1601740
Blocks: 1601745
Blocks: 1601748
Blocks: 1601749
Depends on: 1601830
Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

I may have solved the setTimeout issue, as I stated in bug 1601830, forcing the blur() seems to do the trick.
I cleaned up some methods and started writing more comments to properly describe what it's happening.

I'll finish writing comments to cover all the methods I introduced, and fix the newly implemented mochitests.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=674bb73bd2b093deb8170dc8ecd7d67b5d1239a9

Attachment #9113385 - Attachment is obsolete: true
Attachment #9114050 - Flags: review?(mkmelin+mozilla)
No longer depends on: 392932
Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

I made some progress on this thanks to the suggestion from bug 1601830.
I also fixed some mochitests and other random intermittent tests I stumbled upon, and commented pretty much everything.

Magnus, please take a look at what I did with the observer inputObserver and let me know if you think this is a correct implementation.

I'm not asking for a full review because c-c has been busted almost all afternoon and I couldn't run proper indepth tests.

Attachment #9114050 - Attachment is obsolete: true
Attachment #9114050 - Flags: review?(mkmelin+mozilla)
Attachment #9114312 - Flags: feedback?(mkmelin+mozilla)
Blocks: 1602372
Blocks: 1602397
Blocks: 1602431
Blocks: 1602442
Blocks: 1602451
Blocks: 1602456
Blocks: 1602461
Blocks: 1602470
Blocks: 1602477
Blocks: 1602492
Attached image postbox-pills.png

Can I suggest something terrible? Postbox has had this function for years. Maybe it wouldn't be totally absurd to check how their stuff is working.

Blocks: 1602567

(In reply to Jorg K (GMT+1) (PTO to 15th Dec 2019, sporadically reading bugmail) from comment #192)

Created attachment 9114666 [details]
postbox-pills.png
Can I suggest something terrible? Postbox has had this function for years. Maybe it wouldn't be totally absurd to check how their stuff is working.

Absolutely. Some interesting ideas there, like the dropdown context which reduces the delete button clutter at the cost of two-click recipient deletion. A bug which eats recipients upon shift-selection (shift-select last recipient, then shift+home). Incomplete selection experience (no Ctrl-selection at all). Alessandro's interim stage is already better than theirs.

Another point of comparison: gmail's webmail composition interface. Far from perfect, but nice reduced and calm layout of pills, and some good behaviour there.

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

I think this is ready for a full review, and at a stage where we can land it so I can start working on the follow up bugs to improve it.

Thanks to the "autocomplete-did-enter-text" event and the handleEnter() method, I was able to remove all those awful timeouts and event dispatching, as well as hijacking the enter keypress or the click event inside the popup.
Everything looks and feels more linear.

Can I suggest something terrible? Postbox has had this function for years. Maybe it wouldn't be totally absurd to check how their stuff is working.

For sure. I've actually been using and testing various apps to write down the various pros and cons of similar solutions, and Postbox is one of them.
This patch is more of an "initial working implementation" of the pill system, in order to have something complete and fully working to land, before continuing onto more thorough and detailed patches.

Thomas, thanks for filing those bugs, which I will be ignoring for now until this patch lands, but definitely a much needed overview of what needs to be done.

Here's the latest try-run with many improvements, and hopefully all the tests fixed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eef5e4aa573cad31df6382748f984d696b654752

If, for whatever reason, that try-run fails or it's busted, here's a fairly recent one with some small missing improvements, but still better than last week: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dce9bf4efcc506e81a1404791a843f19a6cdfac7

Attachment #9114312 - Attachment is obsolete: true
Attachment #9114312 - Flags: feedback?(mkmelin+mozilla)
Attachment #9114788 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9114788 [details] [diff] [review]
440377-recipients-pills.patch

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

comm/mail/test/browser/composition/browser_sendButton.js is failing on all platforms on the try run. But not locally it seems... :/

I think it's pretty close to be in land-able shape.

::: mail/base/content/mailWidgets.js
@@ +1773,5 @@
> +        return;
> +      }
> +
> +      // Used to store the html:input element from where the pill was generated.
> +      this.originalInput;

this doesn't do anything. move the documentation elsewhere?

@@ +1804,5 @@
> +
> +      Services.obs.addObserver(
> +        this.inputObserver,
> +        "autocomplete-did-enter-text"
> +      );

Observers and custom elements have a life cycle problem, so I think you need to set up a window unload event listener too, where you remove the observer. disconnectedCallback isn't called at that time, so otherwise we leak.

::: mail/test/mozmill/composition/test-draft-identity.js
@@ +256,5 @@
>          false
>        );
>      }
>  
> +    close_compose_window(cwc);

looks like a wrong change. we should still test and ensure there is no prompt, which is what the false did

::: mail/test/mozmill/composition/test-forwarded-eml-actions.js
@@ +144,5 @@
>          subjectText
>      );
>    }
>  
> +  close_compose_window(compWin);

same here

::: mail/test/mozmill/composition/test-save-changes-on-quit.js
@@ +234,5 @@
>    let msg = select_click_row(0);
>    assert_selected_and_displayed(mc, msg);
>  
>    let nwc = open_compose_new_mail();
> +  close_compose_window(nwc);

and these... here the sole reason for the test is to verify there were no prompts ;)
Attachment #9114788 - Flags: review?(mkmelin+mozilla) → feedback+

(In reply to Magnus Melin [:mkmelin] from comment #195)

comm/mail/test/browser/composition/browser_sendButton.js is failing on all platforms on the try run. But not locally it seems... :/

Yeah, this is super annoying, but I'll get to the bottom of it.

Observers and custom elements have a life cycle problem, so I think you need to set up a window unload event listener too, where you remove the observer. disconnectedCallback isn't called at that time, so otherwise we leak.

Ah, I didn't know that, thanks for the heads up.
Does something like that look good?

// Remove the observer for each pill as the disconnectedCallback() might not
// be called on time and we might leak.
  for (let pill of getAllPills()) {
    Services.obs.removeObserver(
      pill.inputObserver,
      "autocomplete-did-enter-text"
    );
  }
Blocks: 1602901

To clarify, that should be for the window unload event, so that we make sure to kill off any observer references.

Sure, I added that into the connectedCallback() method of the pill CE.
I also have an observer for the main input field, which gets removed on windows unload, unrelated to the pills.

Attached patch 440377-recipients-pills.patch (obsolete) — Splinter Review

Patch updated with all tests passing on the recent try run, except for those intermittent failures which shouldn't be related to my patch.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bec9f1c7f543c51307397ea1cafbfe321c2db83e

Attachment #9114788 - Attachment is obsolete: true
Attachment #9115034 - Flags: review?(mkmelin+mozilla)

If an pill (i.e. email address) has an associated photo, could the photo please show when the mouse hovers above the pill. Thank you

Blocks: 1603051
Comment on attachment 9115034 [details] [diff] [review]
440377-recipients-pills.patch

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

::: mail/base/content/mailWidgets.js
@@ +1807,5 @@
> +        "autocomplete-did-enter-text"
> +      );
> +
> +      // Remove the observer on window unload as the disconnectedCallback()
> +      // might not be called on time and we might leak.

Slightly wrong comment. disconnectedCallback will never be called when closing a window, so we might therefore leak if XPCOM isn't smart enough. This is not a problem for normal event listeners, they go away with the dom.

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +256,5 @@
>  
>  <!-- Headers -->
>  <!--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;">

I think this needs a bump too (to be sure)

::: mail/test/browser/composition/browser_saveChangesOnQuit.js
@@ +232,5 @@
>    let msg = select_click_row(0);
>    assert_selected_and_displayed(mc, msg);
>  
>    let nwc = open_compose_new_mail();
> +  close_compose_window(nwc);

please add back the false argument, so it's properly checking there was no prompt

@@ +237,3 @@
>  
>    let rwc = open_compose_with_reply();
> +  close_compose_window(rwc);

same here

@@ +240,3 @@
>  
>    let fwc = open_compose_with_forward();
> +  close_compose_window(fwc);

and here 

.... and all the other cases where that got removed

::: mail/test/browser/composition/browser_sendButton.js
@@ +179,5 @@
>    check_send_commands_state(cwc, true);
>  
> +  // Clear the CC list.
> +  clear_recipient(cwc);
> +  // No other pill is there, Send should become disabled.

. not ,
Attachment #9115034 - Flags: review?(mkmelin+mozilla)
Blocks: 1603166

Updated, and here's the new try-run.

Attachment #9115034 - Attachment is obsolete: true
Attachment #9115307 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9115307 [details] [diff] [review]
440377-recipients-pills.patch

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

I'll do the minor adjustments below, and go ahead with landing this. Nothing major lacking that we don't already know about and knowingly will defer to follow-ups.
r=mkmelin

::: mail/base/content/mailWidgets.js
@@ +1854,5 @@
> +    /**
> +     * Check if the pill is currently in "Edit Mode", meaning the label is
> +     * hidden and the html:input field is visible.
> +     *
> +     * @returns {bool} If the pill is currently beign edited.

{boolean}
And a typo: beign

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +17,2 @@
>  /**
> + * Convert all the written recipients into string and store them intro the

into

@@ +320,5 @@
> +/**
> + * Find the autocomplete input when an address is dropped in the compose header.
> + *
> + * @param {XULElement} target - The element where an address was dropped.
> + * @param {String} recipient - The email address dragged by the user.

In general strings should be lowercase {string}, unless it actually should be a String object. Not a very notable distinction though...

@@ +444,5 @@
> +/**
> + * Add a new "address-pill" to the parent recipient container.
> + *
> + * @param {HTMLElement} element - The element that triggered the keypress event.
> + * @param {Boolean} automatic - Set to true if the change of recipients was

boolean
Attachment #9115307 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/019eaf0f9cf7
Implement new recipients address fields. r=mkmelin, ui-review=Paenglab

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 73.0
No longer blocks: 395606
No longer depends on: 626532
Regressions: 1603526
Depends on: 1603563
Regressions: 1603572
  1. No need for a static To:-Field in Newsgroups
  2. Cursor stays at a reply in the To:-Field and not in the Body.
  3. The whole change looks terrible and I see no advantage in it.
  4. Please build in a Backdoor to the old functionality, or the possibility to customize it.
  5. Thanks

While it's landed we know there are still some work to be done, so stay tuned.
Keeping the old version in parallel would be too large of an undertaking. There are at least a few features/bug fixes that will be much easier to do with this new UI but would have been more difficult to get right UI wise in the old system. E.g. bug 271405.

Blocks: 1603728
Regressions: 1603781

(In reply to ruediger.lahl from comment #207)

  1. No need for a static To:-Field in Newsgroups

I will explore the possibility of hiding the "To" field if the "Newsgroup" field is revealed.

  1. Cursor stays at a reply in the To:-Field and not in the Body.

Fixed in bug 1603526.

  1. The whole change looks terrible and I see no advantage in it.

The current UI is not final and many more iterations and improvements are needed.
Can you elaborate on the "no advantage statement"?
We implemented this for these reasons:

  • Ability to write multiple recipients in the same field.
  • Cut, Copy, Paste, and move multiple recipients from a field to another with one action (eg. 100+ addresses from To to CC).
  • More compact and streamlined UI (still a WIP).
  • Simpler and cleaner code based on Custom Elements without node cloning and duplicated methods.
  • Making life easier for developers.
  1. Please build in a Backdoor to the old functionality, or the possibility to customize it.

Like Magnus said in comment 208.

  1. Thanks

While we know we won't be able to satisfy everyone, I'd like to reiterate that we're implementing these changes to improve the usability and stability of the software. Simply saying "the old is good don't change it" is not enough.

Please, take a look at the multiple follow up bugs we created to continue working on this area, and feel free to provide constructive feedback and suggestions on how to make it better.

Regressions: 1603863

(In reply to Alessandro Castellani (:aleca) from comment #209)

(In reply to ruediger.lahl from comment #207)

  1. No need for a static To:-Field in Newsgroups

I will explore the possibility of hiding the "To" field if the "Newsgroup" field is revealed.

TB can answer to 'Newsgroup' or 'Reply to Sender only' via Menu->Message. 'To Sender only' is no default in Newsgroups and should only be a hidden feature, even as 'Reply to all'.

  1. Cursor stays at a reply in the To:-Field and not in the Body.

Fixed in bug 1603526.

Great.

  1. The whole change looks terrible and I see no advantage in it.

The current UI is not final and many more iterations and improvements are needed.
Can you elaborate on the "no advantage statement"?
We implemented this for these reasons:

  • Ability to write multiple recipients in the same field.

Was ever available with mail1@adress; mail2@adress; mail3@adress; mail3@adress

  • Cut, Copy, Paste, and move multiple recipients from a field to another with one action (eg. 100+ addresses from To to CC).
    ctrl-a - ctrl x(v) works between the fields.
  • More compact and streamlined UI (still a WIP).

Compact is a single Button that rolls out an Menu, like the old UI does. I have all under the Mouse if I need it, by clicking on [To:]

  • Simpler and cleaner code based on Custom Elements without node cloning and duplicated methods.

I don't understand that. Custom Elements like additional Entries/Buttons can be generated by Entries in user.js like: user_pref("mail.compose.other.header", "Supersedes,Control,X-No-Archive");

  • Making life easier for developers.

Okay.

  1. Please build in a Backdoor to the old functionality, or the possibility to customize it.

Like Magnus said in comment 208.

The ability to customize should be given.

  1. Thanks

While we know we won't be able to satisfy everyone, I'd like to reiterate that we're implementing these changes to improve the usability and stability of the software. Simply saying "the old is good don't change it" is not enough.

In this case the old is (IMHO) better then the new.

Please, take a look at the multiple follow up bugs we created to continue working on this area, and feel free to provide constructive feedback and suggestions on how to make it better.

Okay and thank you.

Blocks: 1478948

Hello @aleca,

Like mentioned on IRC yesterday, the "reply to" pill appears in red.

I am not sure if it is a bug or a feature.

See the screenshot.

Flags: needinfo?(alessandro)

Thanks for the feedback Marco.
That's not red but orange, which means the address you're using is not currently stored in your address book.

I know it looks confusing, but that's just temporary as a better UI is getting explored in the followup bug 1601748.

Flags: needinfo?(alessandro)
Blocks: 1603563
No longer depends on: 1603563
Regressions: 1607526

[Feature request] In capsule itself, add an option toggle to, cc, bcc [like close button on end, in front To/CC/BCC] if available, it will be very nice.

Hmm, I don't have the latest build, but in my local build of a few days old, there is not [x] to delete the pill. There's only a context menu to delete, cut, edit, etc. So what's the design to change a To to a CC? Cut and paste? Put that onto the context menu: "Move to CC"? What are the plans?

(Pasting a pill seems to paste raw text in red, and I have to hit enter to turn it into a pill. Sorry if the question is silly, I haven't followed the action closely in all related bugs, but that seems to be bug 1602456).

Flags: needinfo?(bugzilla2007)
Flags: needinfo?(alessandro)

Hmm, I don't have the latest build, but in my local build of a few days old, there is not [x] to delete the pill. There's only a context menu to delete, cut, edit, etc. So what's the design to change a To to a CC? Cut and paste? Put that onto the context menu: "Move to CC"? What are the plans?

The [X] was removed to clear UI noise, prevent accidental address deletion while selecting a pill, and to behave more like a desktop app and not a web app.
The plans for the "move to another recipient" option are the following:

We could consider adding a a menu item into the context menu as you suggested, if that's the case of the request.
I'm against adding labels or extra items in the pills unless absolutely necessary as it creates a lot of noise and makes the keyboard accessibility a bit of a nightmare.

Flags: needinfo?(alessandro)

OK, well, yes, adding it to the context menu might be useful.

Flags: needinfo?(bugzilla2007)

(In reply to Jorg K (GMT+1) (PTO to 26th Jan 2020, sporadically reading bugmail) from comment #216)

OK, well, yes, adding it to the context menu might be useful.

Confirmed, could really be useful, as keyboard users like blind couldn't do drag and drop and copy/paste from one field to another one with the keyboard would be really practical.

Just a concept. First box area for selection[for example click box one turn blue fill. so in selection mode]. second area mail address. single click start edit. Third area options[cut/copy/move to To/move to To Cc/move to To BCC etc.].

Concept 2.
To/cc/Bcc will have only single input box. In front of address pill we can change.

(In reply to Jorg K (GMT+1) (PTO to 26th Jan 2020, sporadically reading bugmail) from comment #216)

OK, well, yes, adding it to the context menu might be useful.

Confirmed, could really be useful, as keyboard users like blind couldn't do drag and drop and copy/paste from one field to another one with the keyboard would be really practical.(In reply to Jaise from comment #218)

Created attachment 9121222 [details]
mail addressconcept 1.jpg

Just a concept. First box area for selection[for example click box one turn blue fill. so in selection mode]. second area mail address. single click start edit. Third area options[cut/copy/move to To/move to To Cc/move to To BCC etc.].

I imagine a simple stuff in the contextual-menu "move to". It's more consistent with what TB does in message.

It is OK.

In my context, our organization have aged people [say 45-55]. so teach them again new method may be problem [say double click to edit, right click to get option etc]. At least if downward arrow is there, they see & click.

Blocks: 1609647

(In reply to Alex ARNAUD from comment #217)

(In reply to Jorg K from comment #216)

OK, well, yes, adding it to the context menu might be useful.

Confirmed, could really be useful, as keyboard users like blind couldn't do drag and drop and copy/paste from one field to another one with the keyboard would be really practical.

Alex Arnaud, we are still working on improving the keyboard experience, and I'm known for my focus on ensuring correct and efficient keyboard UX. Please note that at this early stage, we still have a number of related bugs (see bugs in the "Blocks" section of this bug). Right now, as a keyboard user (including the blind), you can already select pills using some of the known keyboard methods like space bar to select or deselect, Shift+cursor for contiguous selection, etc. Then you can use Ctrl+X to cut, tab to move focus into the next recipient type (two tab stops after bug 1602372), Ctrl+V to paste. I think that's acceptable, but it could be even easier from context menu. We'll explore doing the context menu "Move to..." in bug 1609647.

As we're still in the process of getting the code and behaviour right, details of ARIA requirements may not immediately get the attention they deserve. Please bear with us, but do feel free to file respective bugs if they haven't been filed yet.

(In reply to Jaise from comment #218)

Created attachment 9121222 [details]
mail addressconcept 1.jpg

Just a concept. First box area for selection[for example click box one turn blue fill. so in selection mode]. second area mail address. single click start edit. Third area options[cut/copy/move to To/move to To Cc/move to To BCC etc.].

Hi Jaise, thank you for sharing your ideas! (Longer discussions would probably need another platform).
I can see where you are coming from with your concept 1, which is one possible way of doing it, and certainly deserves consideration.
Concept 1 is trying to make 3 different actions available by single-click on a certain subsection of a pill:

  • Select (click left corner of pill)
  • Edit (click middle part of pill)
  • Context menu (click dropdown arrow in right corner of pill)

On the other hand, there's a worthwhile objection from Alessandro, our UX lead (comment #215):

I'm against adding labels or extra items in the pills unless absolutely necessary as it creates a lot of noise and makes the keyboard accessibility a bit of a nightmare.

So I'm not sure if the benefits of your concept 1 are strong enough to justify the costs.

So far, we're a desktop application, so imo context menu is a well-known and efficient way of accessing contextual functionality.
I've previously advocated for the single-click-to-edit myself, but easy selection of pills looks like an even more important aspect. Consider that editing a pill has easy access with Enter (and F2 on Windows), and it may not be the most frequent action on pills.
Your concept 1 requires a lot of user attention for all use patterns because each pill is now divided into three different click targets, also making pills significantly bigger and more noisy.

Comment on attachment 9121223 [details]
mail address concept 2.jpg

(In reply to Jaise from comment #219)
> Created attachment 9121223 [details]
> mail address concept 2.jpg
> 
> Concept 2. 
> To/cc/Bcc will have only single input box. In front of address pill we can change.

It's probably better to file such concepts in their own dedicated bug / RFE and make that dependent on this bug to create the link.

Concept 2 is not feasible in terms of UX design (wontfix) as it duplicates and/or obfuscates recipient type selection by reintroducing the old and unsorted way of defining recipient types. One of the key benefits introduced by this bug is that recipients of different types are now sorted together into their own respective box, so that you can see e.g. all of your BCC recipients at a glance. The answer to this problem of transitioning from the legacy UX is bug 1609647, having a context menu entry to change the recipient flavor of each pill, which will move that pill into the right container.
Attachment #9121223 - Flags: ui-review-
Regressions: 1609894
Regressions: 1609895
Regressions: 1609901
Regressions: 1609928
Blocks: 1609958
Blocks: 1609974
Blocks: 1609977
Blocks: 1610883
Regressions: 1610771
Regressions: 1603166
Blocks: 499766
Blocks: 1612172
Blocks: 1612186
Regressions: 1612369
Blocks: 1612761
Blocks: 1612765
Duplicate of this bug: 1613017

Hard to follow all this "pill" stuff, but having problems in beta. First, want to enter multiple addresses on one line but need single character to complete current address and start next. Have tried several chars for this, but have not found one that works. Nothing in Beta Notes. Second, appears to be discussed above, not easy to move address to different line (was easy before). Third, installation of beta wiped out toolbar customization (may not be related to this "enhancement"). Fourth, the "Cc" etc, buttons to create the lines are in an odd place. Would like the option to just auto add the two lines and hide the buttons.

First, want to enter multiple addresses on one line but need single character to complete current address and start next. Have tried several chars for this, but have not found one that works.

I'm not sure I understand your problem.
If you start typing an address, does it prompt the autocomplete? Once the autocomplete gives you the right address, you can hit enter or comma and start typing a new address, which should prompt again the autocomplete.
Are you trying to enter multiple addresses all at once by pasting a string of emails?

not easy to move address to different line (was easy before)

You can cut and paste that address (or multiple addresses at once) from a row to another. Before it was pretty much impossible to do it quickly for more than 1 address at a time.
Drag & Drop functionality is getting introduced in a follow up bug.

installation of beta wiped out toolbar customization

Not related to this, check bug 1613965.

the "Cc" etc, buttons to create the lines are in an odd place.

What's odd about the positioning? That's quite a standard location similar to other email clients, and is very easy to reach with both mouse and keyboard shortcuts.

Would like the option to just auto add the two lines and hide the buttons.

That's a bit of an edge case that doesn't introduce any real benefit other than removing 1 click to show that line, and it's quiet odd to always need CC and BCC always visible for every email.

entering multiple addresses with return between now works in b2. Did not in b1.
"old" way with single address per line took 1 click to change from "to" to "cc", etc. cut & paste is awkward in comparison.
I have never seen buttons to the far right. Been using mailers 35-40 years. Unix, windows, multiple other systems.

I think I need to chime in here, there is already some push back in support over the pills and the inability to set a default BCC in particular. Many Thunderbird users curate newsletters etc for community organizations, from their church to the local football club. These people want to add all addresses to a BCC line by default for obvious reasons. Many do not send using a To: header at all, until they get SPAM classifications at least.

A couple of support topics
https://support.mozilla.org/en-US/questions/1278219
https://support.mozilla.org/en-US/questions/1278340

An alternative would be to allow an option to designate mailing lists to be set to add to a default send type To: CC: BCC: in the address book. But we have to consider these users, they are not an edge case in my opinion.

(In reply to Matt from comment #229)

Many Thunderbird users curate newsletters etc for community organizations, from their church to the local football club. These people want to add all addresses to a BCC line by default for obvious reasons. Many do not send using a To: header at all, until they get SPAM classifications at least.

Adding all addresses to a BCC line may also give a SPAM classification. Newsletter are best sent using the Mail Merge add-on.

Should Mail Merge functionality be added to Thunderbird core?

Thank you

(In reply to doug2 from comment #228)

Hey doug2, thank you for your feedback, and sorry that you couldn't warm up yet with our new recipient area.

entering multiple addresses with return between now works in b2. Did not in b1.

We're still working on improving this feature, please stay tuned. We do appreciate that it's a change which requires a bit of adjustment from the existing legacy layout. We are striving to deliver a new user experience which we believe will ultimately be significantly better, more versatile, and more efficient than the status quo.

"old" way with single address per line took 1 click to change from "to" to "cc", etc.
cut & paste is awkward in comparison.

Well, it was two clicks, one to open dropdown, next to choose type. And if you wanted to change 50 existing recipients, that was 100 clicks - now that's truly akward if you ask me. Now you can easily change 100 recipients with 3 simple steps: Ctrl+A to select all of same type, Ctrl+X to cut, Ctrl+V to paste in the other row. I find that awesome. As Alessandro said, more methods like drag and drop are coming, and I am working with Alessandro to complete the mouse and keyboard selection experience. We also have bug 1609647 about offering a two-click way of changing recipient type from context menu of each recipient, which may ease the transition for your scenario.

I have never seen buttons to the far right. Been using mailers 35-40 years. Unix, windows, multiple other systems.

Well, for now we've placed them there after careful consideration of pros and cons. Every UI design decision has them. You can now create a CC recipient with a single click on an openly available button, as opposed to carefully picking from a covert dropdown. They are also currently prime members of the tab focus ring, easy to reach with keyboard as Alessandro pointed out. If we put them to the left, others will say, why clutter the prime side with secondary options. Every change is a challenge to some extent, but no change spells doom...

That said, please do not hesitate to file bugs for regressions, or specific proposals for improvement!

(In reply to Óvári from comment #230)

Adding all addresses to a BCC line may also give a SPAM classification. Newsletter are best sent using the Mail Merge add-on.

Despite what you saying being correct, it does not change the usage that people want to make of the program. I have spent the last 10 years trying to educate, but there are millions of them, and one of me. So let's just make a product that the users want To use in the way they want to use it. When it breaks because of spam rules we can explain that to them. It is much harder to explain why we are making it harder for them to conceal their newsletter recipients' addresses. They want to do that for privacy and EU regulations.

It is far easier to tell them spam rules are constantly changing and they got caught when it breaks, because it is not Thunderbird doing it.

Should Mail Merge functionality be added to Thunderbird core?

Certainly, it should be there, it should have been there since about version 2. But then MAPI should have been working for the last decade, but I can still not make it work.

Thunderbird reminds me of Lotus 123. Powerful and bare bones, requiring the user to build the caravan they work from on the chassis supplied by Mozilla. Microsoft always just supplied the lot and found most users used less than 20% of the features, but for the others, it was only a case of discovering them, not having to build their own.

(In reply to Matt from comment #229)

I think I need to chime in here, there is already some push back in support over the pills and the inability to set a default BCC in particular.

Thanks Matt! If you are talking about the per-identity auto-BCC feature from account settings, then the "inability to set a default BCC" is a myth. I've just tried it and it works just like before. So I'm not seeing how the new design affects that. If there's anything which you think is not working, please file a bug and link it to this bug. Or is it about not finding the BCC button in the far-right top corner?

A couple of support topics
https://support.mozilla.org/en-US/questions/1278219

I believe that report has never been accurate, but I have personally seen to it that virtually all of the unexpected jumps have been eliminated. In the new design, you can still type a recipient, press Enter, type another recipient, press Enter, no jumps. Only if you press Enter on an empty input, that will obviously move you to the next addressing row.

https://support.mozilla.org/en-US/questions/1278340

Yes, this one is more serious now, describing the effects of habit formation and feeling initially lost in the new layout. I do believe that the new layout is pretty much self-explaining, but yes, to existing users, there's a bit of a learning curve. We should probably prioritize bug 1609647 to ease the transition. We could also explore if there are any sustainable (or maybe temporary) ways of reacting to clicks on the To label (on the right) in an attempt to change the flavor. Postbox has a context menu to show and hide rows right there. We might even show a context menu on left-click. Or flash our new buttons on the right when you click on the left. Such transition candy might be considered when the feature is more complete.

An alternative would be to allow an option to designate mailing lists to be set to add to a default send type To: CC: BCC: in the address book.

On record as bug 163498. Yes, that could be useful to prevent accidental disclosure of the list. On the other hand, you can just add the list into the correct addressing row, and that should work, and it does per my quick tests.

But we have to consider these users, they are not an edge case in my opinion.

We should always consider our users. Edge case or not is a bit hard to tell, but the transition experience would probably deserve some more attention.

Whiteboard: [patchlove][proof of concept/workaround: addon, see URL][parity-all-other-mailers][parity-postbox][ux-feature-wanted-38] → [parity-all-other-mailers][parity-postbox]
Blocks: 1615839
Blocks: 1615917
Blocks: 1616155
Blocks: 1616514
Blocks: 1616717
No longer blocks: 1616514
No longer blocks: 1616717
No longer blocks: 1616155
Blocks: 136897
See Also: → 1167434
Regressions: 1623285
Duplicate of this bug: 1244494
Regressions: 1627166
Regressions: 1627435
Regressions: 1627451
No longer regressions: 1627451
Regressions: 1629144
Blocks: 1629364
Blocks: 1633555
Blocks: 1637657
Regressions: 1640722
Regressions: 1642279
Whiteboard: [parity-all-other-mailers][parity-postbox] → [parity-all-other-mailers][parity-postbox][relnotetb78]

It's unfortunate that the most relevant UX change of Thunderbird 78 has been associated with the wrong product (MailNews Core) all along without anyone taking notice, and hence many bugs cloned from here (and their subsequent clones) are also in the wrong product. There is very little TB code which still lives in MailNews Core (shared with SeaMonkey), so the correct product is Thunderbird.

Can we make sure that at least it will be included in the release notes for TB78, because users won't read the release notes for TB73beta [1] where this landed? Thanks.

[1] https://www.thunderbird.net/en-US/thunderbird/73.0beta/releasenotes/

Component: Composition → Message Compose Window
Flags: needinfo?(vseerror)
Product: MailNews Core → Thunderbird

re :I do believe that the new layout is pretty much self-explaining

Sorry, but I disagree....No it isn't.
Why is Cc and Bcc associated with the FROM by placing it to the right of 'FROM' ?
It is not logical by any stretch of the imagination. It certainly will be a headache for those who have an eyesight issue and cannot locate because they are not where expected.

You can have the same buttons with same effect in the logical position on left, one above the other, next to the 'TO'.
Then it would be intuitive and users would only need to discover/learn the new functionality.
You will always need to use the TO/Cc anyway, so mouse is naturally focussed at start of TO on the left.
The mouse would only be to the right if needing to change the 'FROM' which is probably less used.

Adding and improving functionality is great and much needed, I like the sound of the proposed new functionality and it's potential, but there is no way it is logical to put buttons for cc, Bcc to appear to be associated with FROM.

re :I have never seen buttons to the far right. Been using mailers 35-40 years. Unix, windows, multiple other systems.
I completely agree with this statement.
I'm not aware of other email clients that would put TO and Bcc functionality buttons off top right of a FROM totally unassociated with the whereabouts of the actual TO,Cc,Bcc.

re :If we put them to the left, others will say, why clutter the prime side with secondary options.
They are cluttered on the right side and in the wrong place. The 'prime' side is adjacent to the TO, cc and Bcc.

Have you considered what happens when you have a longer FROM name and email address, this means in order to see that and the buttons, you will have to have a wider window, but if the same space is used for new functionality as is currently used then a greater width of window is not necessary.

Regarding the use of RED in font:
This is never a good idea and has been a serious problem and flaw in Thunderbird for decades. It is important to stop using red for the sake of 10% of men who suffer from various degrees of colour blindness. When I was learning about design decades ago, this was common knowledge that you did not use red on items like email addresses. Red should only be used if the user has full control over change of colour. I still see people asking for how to stop red font colour in eg: TO fields when typing addresses. Please take this into consideration whilst you are redigning this area.

(In reply to Anje from comment #236)

Hi Anje, thanks for feedback!
It was me who wrote your initial quote, so let me reply. :-)

re :I do believe that the new layout is pretty much self-explaining
Sorry, but I disagree....No it isn't.

If the position of CC and BCC is your only worry, that means that the other 90% of the new layout is self-explaining. Even with CC and BCC I don't think anyone would fail to understand what they will do when you are looking for them...

Why is Cc and Bcc associated with the FROM by placing it to the right of 'FROM' ?

I hear you. There's a separator, so they are not meant to be associated as such. We've tried and discussed many possible positions, and users have requested that we push them more to the middle, so that's where they are now. It's a well-considered compromise position which has several advantages: clean layout, using up less space, allow focus on more important elements, better keyboard access, etc...

Also, there are many other ways of displaying CC/BCC which do NOT require the disclosure buttons on the right:

  • right-click on selected items in To-field, then "Move to CC/BCC" from context menu.
  • From contacts side bar, select recipients and use Add-to-CC/BCC buttons.
  • We'll also implement some variant of bug 1616514, allowing users to start compositions with CC/BCC already open (per identity).

It is not logical by any stretch of the imagination. It certainly will be a headache for those who have an eyesight issue and cannot locate because they are not where expected.

That's a non-argument, honestly. Unexpected location is not an eyesight issue. If you have eyesight issues, we're striving to provide full accessibility for screen readers, but that's all we can do. In fact, by the current position we have optimized the focus ring for the basic case of a simple message with To-recipients so that it's less of a hassle to users with eyesight issues and screenreaders.

You can have the same buttons with same effect in the logical position on left, one above the other, next to the 'TO'.

Where they would use up more space and clutter the area with less used options.

Then it would be intuitive and users would only need to discover/learn the new functionality.

Learning the new position of two buttons isn't very hard imho. We didn't hide them.
In fact, they are now more exposed than before. In the old layout, having to change the type for each and every recipient from a dropdown which is typically hidden was not very intuitive, it's just that we all got used to it.

You will always need to use the TO/Cc anyway, so mouse is naturally focussed at start of TO on the left.
The mouse would only be to the right if needing to change the 'FROM' which is probably less used.

re :I have never seen buttons to the far right. Been using mailers 35-40 years. Unix, windows, multiple other systems.

Well, the Google Web UI has them more far to the right than us, in their default compose UI... So we're NOT the lonely only one out there.

I completely agree with this statement.
I'm not aware of other email clients that would put TO and Bcc functionality buttons off top right of a FROM totally unassociated with the whereabouts of the actual TO,Cc,Bcc.

Hold on Anje, these are just disclosure buttons. After disclosure of CC/BCC rows, labels precede their row.

re :If we put them to the left, others will say, why clutter the prime side with secondary options.
They are cluttered on the right side and in the wrong place. The 'prime' side is adjacent to the TO, cc and Bcc.

Ah well...

Have you considered what happens when you have a longer FROM name and email address,

Yes. I have just tried it on half-screen size and there is no usability issue. Not for the short labels in en-US at least.
That's something to talk about. I like Google's solution where they just use the short English labels CC/BCC even for other localizations like German, to avoid long and clumsy "Kopie / Blindkopie".
But let's not forget that we're a desktop app so far, and screens are typically long horizontally.

this means in order to see that and the buttons, you will have to have a wider window, but if the same space is used for new functionality as is currently used then a greater width of window is not necessary.

I don't think that is true. If I understand you correctly, you'll have two columns for the recipient type labels, so that's using up more horizontal space than now.

Regarding the use of RED in font:
This is never a good idea and has been a serious problem and flaw in Thunderbird for decades. It is important to stop using red for the sake of 10% of men who suffer from various degrees of colour blindness. When I was learning about design decades ago, this was common knowledge that you did not use red on items like email addresses. Red should only be used if the user has full control over change of colour. I still see people asking for how to stop red font colour in eg: TO fields when typing addresses. Please take this into consideration whilst you are redigning this area.

Kindly file a separate bug if you think that's an issue. We're using red to mark erroneous pills, which looks like the only intuitive color for that.
I think it's customizable through adjusting the CSS, probably userChrome.css just like before. Not sure.

Bottom line:
I think compared to the massive overall benefits of the new recipients area (not explained in this comment), getting used to a slightly different position of CC/BCC disclosure buttons is a minor issue of adjusting your muscle memory.