1785851 - Ensure recipients are always pillified before a message is saved or sent

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Alessandro Castellani [:aleca]]

Reporting user's feedback from bug 1779785 comment 12

Edit an existing pill and click the menu items "File -> Save", "File -> Save As -> Draft", "File -> Save As -> Template" or the toolbar button "Save"
-> The pill is not updated and the created message still contains the old value of the pill

Edit an existing pill and click any other menu item or toolbar button
-> The pill is not updated

Edit an existing pill and click "File -> Close" or the window decoration "X" / "Close"
-> The pill is not updated and the compose window is closed without asking the user whether to save the changes

The mentioned cases pretty much apply similarly to a new entry in the message header fields, which is not yet pillified.

Case (2) is also impacting add-ons in particular: When an add-on calls the API function messenger.compose.getComposeDetails() as part of a registered listener to either the API function messenger.menus.onClicked() or messenger.composeAction.onClicked(), then the returned details contain still the old values, because a new entry was not yet pillified and the edited pill was not yet updated.

When an entry in the message header fields is not yet pillified or an existing pill is edited, I think the change should be applied as soon as the user hits "Enter" or "Tab" or uses the mouse to navigate out of the field.

At the moment the keyboard navigation is working as expected, but the mouse navigation is still lacking in this regard. When you click the subject or body afterwards, then both a new entry is pillified and an edited pill is updated. But when clicking on the menubar or toolbar, neighter change is applied, i.e. a new entry is not yet pillified and an edited pill is not yet updated.

Comment 1

[Alessandro Castellani [:aleca]]

Edit an existing pill and click the menu items "File -> Save", "File -> Save As -> Draft", "File -> Save As -> Template" or the toolbar button "Save"
-> The pill is not updated and the created message still contains the old value of the pill

This is definitely an issue and we need to pillify any address before saving the draft, but only if the action is triggered by the user and not on auto save.

Edit an existing pill and click "File -> Close" or the window decoration "X" / "Close"
-> The pill is not updated and the compose window is closed without asking the user whether to save the changes

This should trigger the "Unsaved changes dialog", doesn't it?
We shouldn't automatically pillify a string if the user is specifically closing the compose window, and if we warn them with a confirmation dialog about unsaved changes, we shouldn't do anything and users decide what to do.

For the other reported issues, there are some historical reasons why pills are not automatically created in all of the occasions.

• If the address is not present, no autocomplete is possible, and the user interacts with a menu item

We can't force the pill creation since we can't safely assume the user is not selecting "Edit > Paste", or trying to open the contacts list to check for another address, and so on.

Comment 2

[Alexander Bergmann]

(In reply to Alessandro Castellani [:aleca] from comment #1)

Edit an existing pill and click the menu items "File -> Save", "File -> Save As -> Draft", "File -> Save As -> Template" or the toolbar button "Save"
-> The pill is not updated and the created message still contains the old value of the pill

This is definitely an issue and we need to pillify any address before saving the draft, but only if the action is triggered by the user and not on auto save.

Yes, for saving drafts and templates(!) Thunderbird should definitely pillify and update pills accordingly, so the created messages contain the new values. You are right, that autosaving a message should not interrupt the user while creating a new pill or editing an existing pill.

Edit an existing pill and click "File -> Close" or the window decoration "X" / "Close"
-> The pill is not updated and the compose window is closed without asking the user whether to save the changes

This should trigger the "Unsaved changes dialog", doesn't it?

Yes, it SHOULD trigger the confirmation dialog, but it DOESN'T do this at the moment!

You can try the following: Save a message as a draft and close the compose window. Then open the saved draft and only edit the pill - without closing it manually. If you then directly try to close the compose window - either via the menu item "File -> Close" or via window decoration "X", the confirmation dialog is not shown.

Only editing a pill in a draft message without any other change does not trigger the confirmation dialog on its own. From a user perspective this might come in as the very same surprise as with the cases regarding send or save.

We shouldn't automatically pillify a string if the user is specifically closing the compose window, and if we warn them with a confirmation dialog about unsaved changes, we shouldn't do anything and users decide what to do.

Yes, but the confirmation dialog is not shown in this specific case - see above.

For the other reported issues, there are some historical reasons why pills are not automatically created in all of the occasions.

• If the address is not present, no autocomplete is possible, and the user interacts with a menu item

We can't force the pill creation since we can't safely assume the user is not selecting "Edit > Paste", or trying to open the contacts list to check for another address, and so on.

Understood... Cut, Copy & Paste, etc. are indeed problematic then. I didn't think of them.

But - maybe - pillifying the input and updating the pills could always be done, when the user switches focus via mouse into another message header field (From / To / Cc / Bcc / ReplyTo / Custom Headers), the subject or the body? At the moment specifically pillifying is not always done in this case.

I think from a user perspective it would be more appropriate to first pillify the input in all of these cases and then show the warning regarding the invalid email address.

Comment 3

[Alexander Bergmann]

As you know I am also the developer of the add-on Mail Merge and - maybe - the following use-case for my add-on can be considered as well:

Since the pillification was introduced back in Thunderbird 78, I get frequent support requests, why Mail Merge is showing them a "No recipients were specified." warning, altough they already entered something like "{{FirstName}} {{LastName}} <{{PrimaryEmail}}>" or simply "{{Mail}}" in the To field.

It always turned out they did not hit "Enter" or "Tab" manually for Thunderbird to pillify the input. Instead they just clicked the subject or the body - or directly the toolbar button "Mail Merge" in case they filled out the To field last.

Over the years I tried to help my users with a prominent warning regarding variables in the To field on the homepage and in the changelog. Nevertheless this is still an issue after all those years!

So when I read the original bug report and experimented with the fix in Daily, I had the idea this use-case could simply be solved as well by pillifying and updating pills whenever a menu item or toolbar button is clicked - or when messenger.compose.getComposeDetails() is called. But you - and John - now rejected this general approach.

Which makes me wonder, what the best solution for an add-on like mine is?

I have now discovered I can call both pillifyRecipients() and updatePillsInEdit() from within the "experiment", which would solve this problem for my add-on for the moment. But it would be reintroduced, as soon as the "experiment" is no longer available for add-ons.

So I would be very glad to have a solution within Thunderbird itself. Either by pillifying the input and updating the pills whenever a menu item or toolbar button of an add-on is clicked - although this probably could lead to the same problems for some add-ons like in the Cut, Copy & Paste cases - or by forcing an update via a MailExtension API.

Or do you - or John - have a better idea?

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: Bug 1785851 - Ensure all pills are pillified before a message is saved or sent. r=darktrojan

Comment 5

[Alessandro Castellani [:aleca]]

Thomas, would you be able to test this once it's on daily?
It affects the timing of saving drafts and templates, and even if we're pretty covered with tests in those scenarios, I'm a bit worried I might have introduced some obscure edge cases due to the async nature of updating opened pills before save.
I'd feel more comfortable after another pair of eyes and some thorough manual testing.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b66e88bf7a9f
Ensure all pills are pillified before a message is saved or sent. r=darktrojan

Comment 7

[Alexander Bergmann]

@Allessandro:

I have just tested the patch on Daily (2022-08-25) and it indeed works now also for "Save as Draft" and "Save as Template".

When closing the compose window the confirmation dialog regarding unsaved changes is shown - which is definitely an improvement. But the edited pills are not actually saved, when the user confirms the changes via "Save". The saved message still contains the old values.

(In reply to Alexander Bergmann from comment #2)

But - maybe - pillifying the input and updating the pills could always be done, when the user switches focus via mouse into another message header field (From / To / Cc / Bcc / ReplyTo / Custom Headers), the subject or the body? At the moment specifically pillifying is not always done in this case.

I think from a user perspective it would be more appropriate to first pillify the input in all of these cases and then show the warning regarding the invalid email address.

Do you think you can also cover this corner case in a follow-up patch?

At the moment, when you - for example - add a simple string like "ABC" into the To field and then hit "Enter" or "Tab", the string is pillified and a warning "ABC is not a valid email address" is shown.

However, when you add "ABC" into the To field and then use the mouse to switch to another field, e.g. Cc / Bcc / ReplyTo / Custom Headers / Subject / Body / Attachments, then the string is not pillified. I think it would be better to also pillify the string and show the warning in this case.

In comment #1 you expressed some concern to always pillify the input, because the user might use some special menubar or toolbar elements. However, when the user switches input into one of the mentioned fields, then I think these concerns do not apply and we can indeed assume the user is finished with adding or editing the input.

As I described in comment #3 this unfortunate behavior of not pillifing simple strings is causing the users of my add-on Mail Merge some problems. So at least pillifying the input when switching between the mentioned fields should ease the problem. (My add-on heavily relies on the use of variables like {{PrimaryEmail}}, which are "only" simple strings and not contacts or email addresses.)

Comment 8

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani [:aleca] from comment #5)

Thomas, would you be able to test this once it's on daily?

I've tested this and found bug 1787958. Otherwise this seems to work as designed.
I'm not sure how to explicitly test timing issues or add-on usage of this.

(In reply to Alessandro Castellani [:aleca] from comment #0)

Edit an existing pill and click the menu items "File -> Save", "File -> Save As -> Draft", "File -> Save As -> Template" or the toolbar button "Save"
-> The pill is not updated and the created message still contains the old value of the pill

This is fixed.

Edit an existing pill and click any other menu item or toolbar button
-> The pill is not updated

Even after the patch, we don't pillify when clicking main menus or toolbar buttons like Spelling, which is good imo.

• We can't pillify on main menu clicks because we need to allow e.g. Edit > Paste.
• Not sure if or why other button clicks on toolbar should pillify (they don't).
• Clicking "inside" the message territory (recipient area, subject, body etc.) will pillify (probably ok).

Edit an existing pill and click "File -> Close" or the window decoration "X" / "Close"
-> The pill is not updated and the compose window is closed without asking the user whether to save the changes

This is fixed.

The mentioned cases pretty much apply similarly to a new entry in the message header fields, which is not yet pillified.

Yes.

Case (2) is also impacting add-ons in particular: ...

Cases aren't numbered - which one is case (2)?
Is there an add-on to test this and what would be the steps?

Comment 9

[Alexander Bergmann]

(In reply to Thomas D. (:thomas8) from comment #8)

Edit an existing pill and click "File -> Close" or the window decoration "X" / "Close"
-> The pill is not updated and the compose window is closed without asking the user whether to save the changes

This is fixed.

Are you sure this is fixed? As I wrote in comment #7 the confirmation dialog regarding unsaved changes is indeed shown now, but the changes are not applied to the saved message. I have just verified this on Daily 106.0a1 (2022-09-13) and it is still the case.

Case (2) is also impacting add-ons in particular: ...

Cases aren't numbered - which one is case (2)?

In my original report in bug 1779785 comment 12 the cases were numbered. The numbering got apparently lost when Alessandro created this new bug.