1609977 - Implement tooltips for remove/delete buttons of each recipient input field/ addressing row, and streamline the confirmation alert

Status: RESOLVED FIXED

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

Description

[Thomas D. (:thomas8)]

Updated summary and description to match reality.

New description (from bug 1609974, comment 0):

Implement tooltips for remove/delete buttons of each recipient input field/ addressing row

In absence of a tooltip, it's not clear what the [x] buttons next to our new recipient input fields are going to do - just hide the field, or remove recipients, or remove recipients AND hide the field?

Proposal for tooltip text of [x] buttons (example of CC field):

"Remove all CC recipients"

If we have that tooltip, perhaps we could drop the alert? (not sure)

---

Original summary of this bug: Recipient pill context: Rename "Delete" menu to "Remove recipient" / "Remove N recipients", so that was about a context menu caption and consistency of wording between various parts of the UI. We bypassed that with a one-liner in comment 1.
Starting from comment 3, due to one duplicate sentence in both bug descriptions (sorry), this bug has been morphed to cover what was originally distinctly filed as bug 1609974, adding tooltips on the (x) buttons to remove recipient input rows, and we also ended up improving the alert of that.

---

Original description:

Recipient pill context: Rename "Delete" menu to "Remove recipient" / "Remove N recipients"

"Remove" is more appropriate than "Delete" for the action of removing recipients from a message, and we use "Remove" in the alert, so this should also be changed in pill context menu. Delete has connotations of permanent data deletion, e.g. of a contact, a notion which may well come to mind for the context menu of a recipient pill.

This is 2020, so we should be smarter sometimes.
I propose to make this a dynamic menu caption based on the number of selected recipient pills:

1 recipient selected: "Remove recipient"
> 1 recipients selected: e.g. "Remove 5 recipients"

Count is cross-field.

Note that unless we differentiate the string furthermore based on focus vs. selection, we cannot use "Remove this recipient", because the context menu can be triggered from a focused recipient which isn't selected, and hence will correctly not be deleted.

Comment 1

[Magnus Melin [:mkmelin]]

You can compare these to nicely formatted text and for text Copy/Paste/Cut/Delete are the norm. So I don't agree.

Comment 2

[Thomas D. (:thomas8)]

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

You can compare these to nicely formatted text and for text Copy/Paste/Cut/Delete are the norm. So I don't agree.

Good point, thanks. I don't have strong feelings about this bug.

I guess there are two legitimate perspectives / design concepts for the recipients - text and items - and both of them are present in the current implementation. The newer and more eyecatching concept (wrt TB's history) is items (aka "pills"), and Alessandro once said he's trying to emphasize that. Again, I don't have strong feelings either way, and I'm on record to suggest merging the best of both worlds.

For this bug, if you look at recipients as text, then yes - it should be plain vanilla Cut/Copy/Paste/Delete - that's totally OK, nice and terse, and consistent with the notion of "text".

For others looking at recipients as items (which they also are), the notion is more like |Cut/Copy/Paste selected items|, so "Remove recipient(s)" might match that conccept better. I took a leaf from composition's attachment pane where also have "Remove Attachments" to avoid the impression of deleting the underlying file, and to be clearer about what exactly is going to be removed (note that most other commands do not have the term "Attachments"). Deleting/removing things is always a bit delicate (more so if it can't be undone), so being explicit about the object of deletion/removal might make users feel a little safer... Also in terms of internal ux-consistency, we usually try to use the same terms for the same action. So from an "items" perspective, matching the traditional context menu for text may seem less attractive.

Comment 3

[Alessandro Castellani [:aleca]]

If we're implementing and discussing tooltips for the X icon in this bug, there's no need to have bug 1609977 opened.
It's confusing to me dealing with the same issue on multiple bugs and getting bombarded by messages of similar bugs.

My suggestion is to keep it simple and straightforward.
The X icon will have a tooltip like "Remove address row", not hiding, not clear. That icon removes the address row from the UI so we should say that.

If the address row has any pill, the alert dialog MUST appear as it's a fail-safe for accidental clicks.
We can update the copy in that dialog to say "cleared" instead of "removed" to be less "scary".

The workflow makes sense to me and it's not confusing.
The X remove the row.
If pills are present in the row, show the confirm dialog to clear them.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: 1609977-remove-row-tooltip.patch

I was so cheeky and took the bug so you can work on the harder bugs. ;-)

I changed the tooltip text to "Remove the address row". What do you think?

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9121902 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

Hey, thank you so much for taking care of this.
It's almost perfect but we're missing those rows that are dynamically generated:
https://searchfox.org/comm-central/rev/009c65e87d240a61404a57362bdf7aa514894fd5/mail/base/content/mailWidgets.js#2078

If you can add that, then it's perfect.
Thanks again.

Comment 7

[Thomas D. (:thomas8)]

So now we have 3 action verbs for the same thing: Delete (pill context), Remove (alert title), Clear (alert body)... which looks detrimental to simplicity and consistency. Also note that this bug was filed about changing the menu caption currently called "Delete" in the pill context menu.

I think the best action verb is "Remove", the literal meaning of which is "move out / take out". I have argued against "Delete" which is too strong imo, apart from being inconsistent with "Remove" in the alert.

I'm still not convinced that the alert is needed, but let's skip that discussion for now and make it better wrt best practices (1) and terseness:

Proposal for the alert (I am using the example of BCC for the dynamic strings)

Remove BCC recipients
------------------------------------------------
(?) Remove all recipients of the type "BCC"?

[Remove]  [Cancel]

Proposal for string IDs:
confirmRemoveRecipientFieldTitle
confirmRemoveRecipientFieldBody

I think it's good practice that the tooltip should match other captions, menus, dialog titles etc. as closely as possible, hence:

Proposal for per-field tooltip: (as I suggested in bug 1609974, and I'm not sure why we've moved that discussion here now).

"Remove all BCC recipients" (and by analogy for other recipient types)

It's exactly NOT just about removing the row. What's a row anyway if I have 50+ recipients in 10 rows in my CC recipient field? It's about losing/removing your recipients of a certain type for this composition.

(1) https://uxplanet.org/5-essential-ux-rules-for-dialog-design-4de258c22116

Comment 8

[Alessandro Castellani [:aleca]]

I'm OK with using "Remove" for everything instead of "Delete" or "Clear".

I don't agree in editing the tooltip text of the X icon of the recipient area.
The primary purpose of that icon is to specifically remove the entire recipient row, not to remove the pills.
If the row has pills, we prompt the confirm dialog.

Updating the tooltip to "Remove all *** recipients" is wrong because that's not the primary purpose of that icon.

Comment 9

[Thomas D. (:thomas8)]

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

I'm OK with using "Remove" for everything instead of "Delete" or "Clear".

Thank you. Richard, could you then try updating the alert per my comment 7?

I don't agree in editing the tooltip text of the X icon of the recipient area.
The primary purpose of that icon is to specifically remove the entire recipient row, not to remove the pills.
If the row has pills, we prompt the confirm dialog.

Oh, I see now, good catch. My tooltip proposal didn't take into account that the recipient field might be empty.

I think we need to agree on a fixed term for that input field (and its content) where users can enter recipients of a certain type, also with a view on support and documentation. I propose to use the following terms consistently for all respective strings:

• recipient field
• CC recipient field
• CC recipients
• recipients of the type "CC"

Proposal for consistent terminology

"Add other types of recipient fields" - (new) extraRecipients button tooltip
"Remove the CC recipient field" - (new) (x) button tooltip
"Remove CC recipients" - (current) alert title
"Remove all recipients of the type "CC"" (my alert proposal)

Detailed rationale:

Currently, we're mixing different terms, which is inconsistent and confusing:

• "Other types of addressing fields" (extraRecipients button tooltip; note that there's no action verb)
• "Remove the address row (Richard's proposal for (x) button tooltip)
• "All the addresses in the CC field will be cleared." (Richard's proposal for the alert, now obsolete)

Terms used:

• field vs. row
• addressing vs. address
• address vs. recipient

I prefer "field" because I am struggling with the notion of "row". If I have 50+ recipients, I'll have (say) 10 rows with 5 recipients in one big CC container which as a whole no longer looks like a row, at all. "Row" also doesn't explain the UI function: It's a text input area, an input field for entering recipients. Field also coincides with the technical term (e.g. RFC 4021), which is helpful for clarity and yet well understandable (as in "input field"), not obfuscating.

Between "addressing" / "address" vs. "recipient" I think "recipient" is much clearer in the long run. "Address" is slightly ambiguous again because it also refers to an email address vs. display name. The term "recipient" nicely captures both plain vanilla and fully qualified recipients. It also points out the function in the composition context: It's a message recipient (vs. sender). When we add our "Move to..." context menu (whatever the caption may be) in bug 1609647, in documentation it's clearer to say "change the recipient type" than "change the address type".

Comment 10

[Richard Marti (:Paenglab)]

Attachment: 1609977-remove-row-tooltip.patch

I'm using Fluent to not need to duplicate the "Remove the address field" string in the DTD and the properties files.

I'm using field instead row like Thomas proposed. Using a dynamic tooltip with the field type in it looks too complicated (and honestly I don't know how to do with Fluent) especially for the ones in the xhtml (a special label for every type seems too much for me).

Comment 11

[Thomas D. (:thomas8)]

Comment on attachment 9121986 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

Richard, thanks for picking this up. Can we polish this a bit more along the alert proposal of my comment 7? I understood that Alessandro is fine with that, too.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +484,5 @@
>  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.
>  
>  ## Recipient pills fileds.
>  ## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle): %S will be replaced with the field name.
>  confirmRemoveRecipientRowTitle=Remove %S

The alert title looks incomplete to me. I think we really need to say "Remove %S recipients"

@@ +490,1 @@
>  confirmRemoveRecipientRowBody=All the addresses in the %S field will be removed. Do you want to proceed?

As I tried to explain in my alert proposal of comment 7, I think this alert message is too verbose, and we should follow best practices for confirmation dialogs which I referenced. Just **|Remove all recipients of the type "%s"?|** would suffice imo for the alert body message, and much easier to parse. "Do you want to proceed?" looks really clumsy, outdated, and suggesting a big danger which isn't the case, and it doesn't add any useful information, just clutter. I don't know how hard it is to change the alert's default button caption to become [Remove], but it must be doable and it should be done. OK buttons which don't make their action explicit are also quite outdated.

Comment 12

[Thomas D. (:thomas8)]

Comment on attachment 9121986 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

(In reply to Richard Marti (:Paenglab) from comment #10)
> Created attachment 9121986 [details] [diff] [review]
> Using a dynamic tooltip with the field type in it looks too complicated (and honestly I don't know how to do with Fluent)

That's an implementation-level excuse, so it doesn't count ;-)

> especially for the ones in the xhtml (a special label for every type seems too much for me).

I disagree. We should be as clear as possible about what exactly is going to be removed.

::: mail/components/compose/content/messengercompose.xhtml
@@ +2086,5 @@
>              </hbox>
>  
>              <hbox id="addressRowCc" class="addressingWidgetItem address-row hidden">
>                <hbox class="aw-firstColBox" align="top">
> +                <label data-l10n-id="remove-address-row"

So if we are lazy (and still new to Fluent), we could just have one dedicated tooltip for each flavor, which is a total of only 5 tooltips. So this would become:
<label data-l10n-id="remove-address-row-cc"

Otherwise, I think we need to figure out dynamic strings for Fluent anyway, so now is as good a time as any. It didn't look too hard when I had a cursory look, in fact it's designed for dynamic translations, fixed terms, embedding other strings, and the like.

@@ +2124,5 @@
>              </hbox>
>  
>              <hbox id="addressRowBcc" class="addressingWidgetItem address-row hidden">
>                <hbox class="aw-firstColBox" align="top">
> +                <label data-l10n-id="remove-address-row"

<label data-l10n-id="remove-address-row-bcc" (and by analogy for other fields below, in the quick and lazy variant)

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Addressing widget
> +remove-address-row =
> +    .tooltiptext = Remove the address field

The reference of "the address field" isn't very clear. At the very least, this must become "Remove *this recipient field*", but I'm not happy with this "one-for-all" approach, which is a nightmare also in terms of accessibility. So for ARIA, we'd have to specify anyway which type of recipient field gets removed - so we can just make that visible in the tooltip, too.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: 1609977-remove-row-tooltip.patch

Added individual tooltips for the existing fields. Only the customised field have the generic "Remove the recipient field". This custom fields are only seldom used by users that add this custom fields.

Alessandro, which strings would you like to be changed?

Comment 14

[Alessandro Castellani [:aleca]]

Comment on attachment 9122086 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

Sweet, Fluent makes things way easy.
The wording is good to me, but if it's OK with you, I'll take over this patch and update the strings to change the field types dynamically as I did it in the past in some OTR patches.

Comment 15

[Alessandro Castellani [:aleca]]

Just |Remove all recipients of the type "%s"?| would suffice imo for the alert body message, and much easier to parse. "Do you want to proceed?" looks really clumsy, outdated, and suggesting a big danger which isn't the case, and it doesn't add any useful information, just clutter.

I recommend not changing the message and leaving the final question, maybe updating it as:
"All the addresses in the %S recipient field will be removed. Do you want to proceed?"

The question at the end is important, even if it might feel redundant or outdated, because there's a bit of danger in doing this action.
If the user has lots of addresses in that recipient field, or even a single address manually written from memory and not saved in the address book, and the remove button is accidentally clicked, I want to avoid for the user to quickly dismiss the dialog.

Also, the final question is to offer a clear [Cancel - OK] button choice, which I might agree not the most fancy and modern solution, but it's and easy YES/NO choice that doesn't leave room for confusion.
If we change the button to Remove, then we'll have a [Cancel - Remove] choice, which opens up another layer of confusion and wording issues.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip-aleca.patch

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 9122137 [details] [diff] [review]
1609977-remove-row-tooltip-aleca.patch

Looks good, thanks. This Fluent shenanigan was what I searched and didn't found (the const parts).

We need to change the updated localization entities to be sure the already translated strings get updated by the translators. Unfortunately we have no other process to be sure they will get the changes.

You should update the patch author and the reviewer. And also a more verbose commit message with the additional changes would be good.

r+ with the above addressed.

Comment 18

[Alessandro Castellani [:aleca]]

Will do, thanks.

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

Comment 20

[Richard Marti (:Paenglab)]

Comment on attachment 9122153 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +249,5 @@
>  <!-- Headers -->
>  <!--LOCALIZATION NOTE headersSpaces.style is for aligning  the From:, To: and
>      Subject: rows. It should be larger than the largest Header label  -->
>  <!ENTITY headersSpace2.style "width: 8em;">
> +<!ENTITY extraRecipients.tooltip "Other types of recipient fields">

This needs also a entity change.

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

Ah, thanks, that slipped.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9122159 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

::: mail/base/content/mailWidgets.js
@@ +36,5 @@
>    );
> +  const { Localization } = ChromeUtils.import(
> +    "resource://gre/modules/Localization.jsm"
> +  );
> +  const syncL10n = new Localization(

lets just use l10n as the name

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3532,5 @@
> + * recipient type.
> + */
> +function updateRecipientRows() {
> +  for (let row of document.querySelectorAll(".address-row")) {
> +    if (!row.querySelector(".aw-firstColBox > label")) {

you're querying for this twice, so you could save the result from the first time ans use that below

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +484,5 @@
>  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.
>  
>  ## Recipient pills fileds.
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle2): %S will be replaced with the field name.
> +confirmRemoveRecipientRowTitle2=Remove %S recipients

capitalize Recipients, like normal for dialog titles
But, it's really Addresses

@@ +486,5 @@
>  ## Recipient pills fileds.
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle2): %S will be replaced with the field name.
> +confirmRemoveRecipientRowTitle2=Remove %S recipients
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowBody2): %S will be replaced with the field name.
> +confirmRemoveRecipientRowBody2=All the addresses in the %S recipient field will be removed. Do you want to proceed?

%S field

I agree with Thomas a verb would be better for the button. Instead of OK, Remove

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +249,5 @@
>  <!-- Headers -->
>  <!--LOCALIZATION NOTE headersSpaces.style is for aligning  the From:, To: and
>      Subject: rows. It should be larger than the largest Header label  -->
>  <!ENTITY headersSpace2.style "width: 8em;">
> +<!ENTITY extraRecipients2.tooltip "Other types of recipient fields">

I think this change is wrong: it *is* an addressing field, but not necessarily a recipient field (like Reply-To, and there are a few other we're not currently showing by default)

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +4,5 @@
> +
> +# Addressing widget
> +
> +# Variables:
> +#   $type (String) - the type of the recipient row

addressing row

@@ +5,5 @@
> +# Addressing widget
> +
> +# Variables:
> +#   $type (String) - the type of the recipient row
> +remove-address-row-type = Remove the { $type } recipient field

Remove the { $type } field

Otherwise it's wrong e.g. for Reply-To

Comment 23

[Magnus Melin [:mkmelin]]

Might also consider to have these warnings shown only happen if you manually changed anything (addresses or mail body). If you just open a reply with pre filled fields, you shouldn't get these prompts if you want to remove e.g. Ccs.

Comment 24

[Thomas D. (:thomas8)]

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

Might also consider to have these warnings shown only happen if you manually changed anything (addresses or mail body). If you just open a reply with pre filled fields, you shouldn't get these prompts if you want to remove e.g. Ccs.

I think Magnus is right. Or maybe we can just remove the prompt altogether...

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

If the address row has any pill, the alert dialog MUST appear as it's a fail-safe for accidental clicks.

• Imho, "accidental clicks" on a 16 pixel (X) icon with no attractive click targets nearby are pretty unlikely.
• Tooltip on (x) will now inform users in advance about the button effect, which further eliminates the risk of mistaking it for "hide"
• If our hover effect doesn't communicate clear enough that this is going to permanently remove the entire row - let's improve that, e.g. by highlighting the entire row with a red border when the (x) button is hovered, or adding a temporary strikethrough effect on all pills - you name it.
• If the UI communicates the button's action more clearly, and after user has seen the effect one, two, three times, what's the danger anyway? Users will hardly type new recipients just to delete them; so, as Magnus said, it's mostly about mass-deleting pre-existing recipients from replies, forwards, etc. Removing those from the current message will typically NOT "delete" them in my TB environment, I can always go back to the original message and get them again.

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

The question at the end is important, even if it might feel redundant or outdated, because there's a bit of danger in doing this action.
If the user has lots of addresses in that recipient field, or even a single address manually written from memory and not saved in the address book, and the remove button is accidentally clicked, I want to avoid for the user to quickly dismiss the dialog.

If I understand you correctly, you went for the extra verbose wording to be deliberately more interruptive, almost "double interruptive": 1st interruption: the unexpected prompt itself. 2nd interruption: verbose wording on the prompt. Imho that might be overdoing it for that little "bit of danger" which we can reduce to near-zero as explained above.

So I don't think that the alert is a MUST per se. What I know from best practices is that alerts (confirmation dialogs) should be avoided whereever possible, because their protective effect wears off fast, and after that they end up as an annoying interruption (1). I expect that very fate for our "Remove CC" alert: essentially it just serves to explain what the button itself is currently failing to explain at first glance, but three (x)-clicks later, we can safely assume users to understand the effect, and it becomes an everyday nuisance for a routine task (more so for business users). Or we can keep it and wait until users file bugs...

(1) https://uxplanet.org/5-essential-ux-rules-for-dialog-design-4de258c22116

Comment 25

[Thomas D. (:thomas8)]

Comment on attachment 9122159 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

Thank you for the new patch. I've made my points, reply-to and followup-to might require special treatment as Magnus pointed out. Otherwise this little bug is getting a bit out of hands, consuming more resources than deserved and expected, so maybe you guys can just sort it out in whichever way, and I may or may not have another look at the end result. A bit of chaos is said to be healthy. ;-)

As a side note, we've worked hard on original territory of Bug #1609974 (the tooltip), and the prompt (both not part of the original scope), but the original purpose of this bug (per current summary) is still unaddressed:
> Recipient pill context: Rename "Delete" menu to "Remove recipient" / "Remove recipients" (for details, see my comment 2).

Again, these are all in the trivial range, so I shouldn't care much...

Comment 26

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

I updated the patch, and also implemented a check to not prompt the user when removing an addressing field that has pills created on load from the account settings.
The prompt will come up if the user actively changes something.

Comment 27

[Thomas D. (:thomas8)]

Comment on attachment 9123203 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

I'm not sure if this does what it you want it to do.

Spelling mistake in commit message. And what is the meaning of "Update recipient strings"?? You mean "Update recipient removal alert strings"?

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +759,5 @@
>      [fieldName.value]
>    );
>  
>    let pills = container.querySelectorAll("mail-address-pill");
>    // Ask the user to confirm the removal of all the typed addresses.

Maybe this comment should be more detailed wrt conditions, which would also expose that conditions are fuzzy.

@@ +761,5 @@
>  
>    let pills = container.querySelectorAll("mail-address-pill");
>    // Ask the user to confirm the removal of all the typed addresses.
>    if (
> +    gContentChanged &&

That's a global variable tracking any changes to the message, header or body. So if user updates something in body before removing the pre-existing pills, we'll still get a false positive alert for unmodified pills, isn't it? Maybe this could be fine-tuned here or on followup bug to only alert when recipients have really been changed by user. And what about cases where user has saved draft with newly typed recipients (even not in AB), e.g. using Ctrl+S (or save when closing), which will set gContentChanged to false, so how does that suddenly qualify my newly-typed recipients for confirmation-less deletion? According to the currently proposed logic ("alert on user-typed addresses"), that should alert but will not, right? Has the interaction of gContentChanged and auto-save been examined? (Alternatively, improve the button and it's hover effect to clearly communicate what it does, and just scrap the entire alert...)

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9123203 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

Seems to have bitrotted

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3531,5 @@
>  /**
> + * Update the close label of every recipient row with the properly formatted
> + * recipient type.
> + */
> +function updateRecipientRows() {

It only sets tooltip, so the name could be better. 
updateTooltipsOfAddressingFields?

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +482,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.
>  
>  ## Recipient pills fileds.

lets fix this typo

@@ +486,5 @@
>  ## Recipient pills fileds.
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle2): %S will be replaced with the field name.
> +confirmRemoveRecipientRowTitle2=Remove %S Addresses
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowBody2): %S will be replaced with the field name.
> +confirmRemoveRecipientRowBody2=All the addresses in the %S field will be removed. Do you want to proceed?

this hasn't changed so no need for a new localization key and making localizers translate it again

Comment 29

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

I updated and unbitrotted the patch, but I'm not asking for a full review as I think this is not completed and some further discussion is necessary.

Alternatively, improve the button and it's hover effect to clearly communicate what it does, and just scrap the entire alert...

This has to stay. It doesn't matter how clear and intuitive the tooltip can be, if the user accidentally clicks on the close label of an addressing field with 50+ addresses which can't be recovered, that's gonna be terrible and we're gonna be flooded by complaints.
The alert dialog might be annoying for you, but an extremely important fail-safe for someone else.

Magnus, regarding the alert buttons, I have to use the confirmEx() method in order to be able to control the strings of those buttons, right?
What do you think about "Confirm" and "Cancel"?

I also implemented a class called recipient-edited which gets added when a new pill is created, and the alert dialog gets triggered only if that class is present in the container.
In this way, we won't trigger the alert unless the user actively edited those fields.

Obviously, that class won't be present if the user saves the message as a draft after it was edited. But at that point, if the user accidentally deletes an addressing field, it could be quickly recovered from the draft.

Comment 30

[Alessandro Castellani [:aleca]]

Richard, would you be able to load this patch and build it on macos?
I think I'm having some issues with my SDK as I get this error whenever I open the compose message window:
[fluent] Request for keys failed because no resource bundles got generated.

Probably the new fluent file is not getting compiled correctly? I don't know.
This works without problems on Linux.

Comment 31

[Richard Marti (:Paenglab)]

Works for me on Mac. The remove row button has it's correct tooltip.

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 9124549 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

Yep need to use confirmEx

::: mail/base/content/mailWidgets.js
@@ +36,5 @@
>    );
> +  const { Localization } = ChromeUtils.import(
> +    "resource://gre/modules/Localization.jsm"
> +  );
> +  const l10n = new Localization(

can we keep the variable as a var

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +778,5 @@
>  
>    let pills = container.querySelectorAll("mail-address-pill");
> +  let isEdited = container
> +    .querySelector(".address-container")
> +    .classList.contains("recipient-edited");

maybe addressing-field-edited

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +484,5 @@
>  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.
>  
> +## Recipient pills fields.
> +## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle2): %S will be replaced with the field name.
> +confirmRemoveRecipientRowTitle2=Remove %S Addresses

Maybe the original was better, just Remove %S?

@@ +490,1 @@
>  confirmRemoveRecipientRowBody=All the addresses in the %S field will be removed. Do you want to proceed?

Maybe 
Are you sure you want to remove the %S field and with all the %S addresses?

   [Cancel] [Remove]

Comment 33

[Alessandro Castellani [:aleca]]

Sounds good, thanks for the feedback.

Comment 34

[Thomas D. (:thomas8)]

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

Review of attachment 9124549 [details] [diff] [review]:

+## Recipient pills fields.
+## LOCALIZATION NOTE(confirmRemoveRecipientRowTitle2): %S will be replaced with the field name.
+confirmRemoveRecipientRowTitle2=Remove %S Addresses

Maybe the original was better, just Remove %S?

Can we try to agree about the relationship between the button and the alert, and the exact purpose of the alert?

• Remove button has two functions:
  • Remove this addressing field (easily undoable for the field, so we do NOT warn on empty field)
  • Remove all addresses (if any) in this field (sometimes not undoable, so we warn about addresses)

So the alert is only about the danger of losing addresses, not to confirm the obvious and no-risk intention of hiding the field.
Imo, the title "Remove BCC" is not reflecting that reality, and really misses the point of danger.
Should be: "Remove BCC addresses" (without a question mark, for consistency with other TB confirmation alerts)

confirmRemoveRecipientRowBody=All the addresses in the %S field will be removed. Do you want to proceed?
Maybe
Are you sure you want to remove the %S field and with all the %S addresses?

That's ungrammatical, 1 word too much or 1 word missing (if in doubt, please remove "and").

I suggest: "Remove all %S addresses from this message?"

Rationale:
This warning only appears when there are addresses in the field, so by design, it is NOT a warning about removing the field itself, which isn't harmful in any way. Asking the user if he really wants to remove the field itself is wrong in two ways:

• Of course user wants to remove the field! There's a tooltip now "Remove {$type} field" and user clicked on that button on purpose (I maintain that accidentally clicking this button is impossible; maybe misunderstanding the button effect, yes). Confirming an obvious intention does not make sense. Note that remove already has a notion of deleting. We don't say "hide" on the tooltip.
• There's no danger in removing the field itself (easy to undo). So why are we complicating the question by suggesting that there's danger in the harmless part ("are you really sure that you want to remove this field??")?? The danger is in removing unsaved addresses, only.

So we shouldn't mention the field. Plus:

• Sorry to insist, but the introductory "Are you sure you want..." is not adding any valuable information, it falsely suggests big dangers, and it just distracts from the main message: Remove BCC addresses - or not?
• Long questions are more likely to be ignored, and they will become even more annoying over time. I bet this will end up as a case for "Never ask me this question again"...

So I am still suggesting to keep this alert sweet and simple (KISS):

[Remove BCC Addresses]
Remove all BCC addresses from this message?

[Cancel] [Remove]

Just my 2 cents.

Comment 35

[Alessandro Castellani [:aleca]]

[Remove BCC Addresses]
Remove all BCC addresses from this message?
[Cancel] [Remove]

I agree with Thomas for the alert strings.
Since that alert is only to warn about the removal of currently written addresses, the title and message make more sense in this way.

I would only change the message to use the word field:
Remove all BCC addresses from this field?

Comment 36

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

Comment 37

[Magnus Melin [:mkmelin]]

I think that while we warn about it due to the addresses, it's still odd to tell we're going to do one thing (remove the addresses) and then go and do another thing (remove the field, and implicitly the addresses). But alright, maybe it's better grammar to have the "addresses" added to the end.
Re the "Are you sure..." - well, that makes it a proper sentence, and it's very common to have confirmations worded like this. Maybe

+------ Remove Bcc Addresses ------------------------------------
|  Are you sure you want to remove the Bcc addresses?
|
|                                            [Cancel] [Remove]
+-----------------------------------------------------------------

Comment 38

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

Updated

Comment 39

[Thomas D. (:thomas8)]

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

Created attachment 9124549 [details] [diff] [review]

What do you think about "Confirm" and "Cancel"?

Ideally, users should be able to take their decision by just reading the button caption (maybe combined with alert title), because that's what we're forcing them into: one button must be pressed. "Confirm" is too generic.

I also implemented a class called recipient-edited which gets added when a new pill is created, and the alert dialog gets triggered only if that class is present in the container.
In this way, we won't trigger the alert unless the user actively edited those fields.

Nice improvement. What's your definition of "editing those fields"? Copy and paste of recipients from outside the message is editing, isn't it? (should alert). What is a "new pill"? If I type an address and it autocompletes, does that constitute a new pill? If I paste in a (virgin) pill from another (draft?) message, is that "editing"? If I paste in text or a pill which is already known in my AB, is that "editing"?

Obviously, that class won't be present if the user saves the message as a draft after it was edited. But at that point, if the user accidentally deletes an addressing field, it could be quickly recovered from the draft.

I'm not sure if the typical user would be smart enough to realize that he can go back to the draft folder and the stale draft message might still have that information which is already missing on the actual draft. What if auto-save kicks in (worse if set to shorter intervals for data-preservation)? Old information gone.
And how do you copy recipients from the draft anyway? One by one from message reader's header? Otherwise, for multiple recipients in one go that's not even possible without losing the rest of your changes in body, so you would have to close the composition, answer "save changes?" with NO (OMG, that's scary!), lose all your body edits, and only THEN could you retrieve multiple recipients in one go from the stale draft. (I'll ignore message source because users don't know that and we can't seriously suggest that).

It's not obvious at all that the status of the message should change in any way by saving as a draft. On the contrary, the saved draft should remain in exactly the same status and condition like what is there on the user's screen at the point of saving. We've fixed several similar problems (*) and the only sustainable solution has always been "keep it exactly as it is". Why should saving the draft with my 50 newly-typed addresses suddenly make them less worthy of preservation? That's inconsistent and makes the behaviour appear even more random.

(*): X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0; attachmentreminder=1; deliveryformat=4

Comment 40

[Thomas D. (:thomas8)]

Comment on attachment 9124869 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

Thank you, looking much better... :-)
Maybe make it 'addressing field' in the tooltip?

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +5,5 @@
> +# Addressing widget
> +
> +# Variables:
> +#   $type (String) - the type of the addressing row
> +remove-address-row-type = Remove the { $type } field

Just 'field' sounds a bit odd/technical. Maybe add 'addressing'?

Remove the { $ type } addressing field

Comment 41

[Alessandro Castellani [:aleca]]

"Confirm" is too generic.

Update to:

+------ Remove Bcc Addresses ------------------------------------
| Are you sure you want to remove the Bcc addresses?
|
| [Cancel] [Remove]
+-----------------------------------------------------------------

Nice improvement. What's your definition of "editing those fields"?

Anytime a pill is created, removed, edited, moved. Any action that changes the amount of recipients in that specific field is considered "editing".

We've fixed several similar problems

Do you have any suggestion on how to store the status of the recipient fields in the draft message. I would very much like to keep that status consistent when saving the draft.

Comment 42

[Thomas D. (:thomas8)]

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

Anytime a pill is created, removed, edited, moved. Any action that changes the amount of recipients in that specific field is considered "editing".

That sounds good! :-)

We've fixed several similar problems

Do you have any suggestion on how to store the status of the recipient fields in the draft message. I would very much like to keep that status consistent when saving the draft.

Yes, see my hint in comment 39:

(*): X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0; attachmentreminder=1; deliveryformat=4

So you may want to have something like "recipientsedited=1" in the draft, which is structurally same as "attachmentreminder=1".
The bug for that was bug 919286, but we implemented that in bug 521158, attachment 825270 [details] [diff] [review].

Comment 43

[Magnus Melin [:mkmelin]]

Storing status of recipient modifications sounds like serious over engineering. But for the draft case we could just init it to be set as modified - it's not a very important or common use case.

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 9124869 [details] [diff] [review]
1609977-remove-row-tooltip.patch

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

LGTM. I guess conflicting with the other patch?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3535,5 @@
> +  for (let row of document.querySelectorAll(".address-row")) {
> +    let label = row.querySelector(".aw-firstColBox > label");
> +    if (!label) {
> +      continue;
> +    }

does this ever happen?

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +791,5 @@
> +    .classList.contains("addressing-field-edited");
> +  // Ask the user to confirm the removal of all the typed addresses if the field
> +  // holds addressing pills and has been previously edited.
> +  if (isEdited && pills.length) {
> +    let confirmAlert = Services.prompt.confirmEx(

result perhaps?

Comment 45

[Alessandro Castellani [:aleca]]

LGTM. I guess conflicting with the other patch?

Yeah, as we're dealing with the creation of the same ftl file.
I will land/wait for bug 1613045 to land before updating this patch.

does this ever happen?

It used to before bug 1607526 landed, since the To field didn't use to have the label.
It's not necessary anymore.

Comment 46

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

Unbitrotted and updated.
Waiting for a full try-run before marking it for a check-in.

Comment 47

[Alessandro Castellani [:aleca]]

Try run for this, including bug 1022228: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=554d58b86636677fa21fde633cc70f26af258636

Comment 48

[Thomas D. (:thomas8)]

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

Created attachment 9125120 [details] [diff] [review]
Waiting for a full try-run before marking it for a check-in.

Before checkin, perhaps double-check the commit message, which looks twisted:

Bug 1609977 - Update recipient alert removal strings and add fluent tooltips to recipient close label. r=mkmelin

Proposed correction:
Bug 1609977 - Update recipient removal alert strings and add fluent tooltips to [X] buttons for removing addressing rows. r=mkmelin

Comment 49

[Alessandro Castellani [:aleca]]

Attachment: 1609977-remove-row-tooltip.patch

Thanks for the heads up, commit message updated.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6ff16e8c98c7
Update recipient removal alert strings and add fluent tooltips to [X] buttons for removing addressing rows. r=mkmelin