Closed Bug 1674054 Opened 2 years ago Closed 2 years ago

All non-pillified recipients (pasted, typed) silently discarded when clicking [Send] button directly

Categories

(Thunderbird :: Message Compose Window, defect, P1)

Tracking

(thunderbird_esr78+ fixed, thunderbird84 affected)

RESOLVED FIXED
85 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird84 --- affected

People

(Reporter: josh, Assigned: aleca)

References

Details

(Keywords: dataloss, ux-error-prevention)

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

  1. Compose email body + subject
  2. Enter (or paste) e-mail address in To field. Do not press Space, or Comma, etc, to complete the Pill.
  3. With the mouse cursor, click "Send".

Actual results:

  1. Will receive a message: "No recipients were specified. Please enter a recipient or newsgroup in the addressing area."
  2. After dismissing the e-mail address Pill is completed.
  3. You can send now.

(Alternatively if you have a correct address in the To field already, or you're entering an address into CC etc, then you do not get the warning message at step #4. Instead the extra addresses are lost and the email is sent with an incomplete list of recipients).

Expected results:

The e-mail pill should be completed or treated as text and sent.

Wow! Thank you Josh for reporting this.

This is highly error-prone and datalossy, regardless of the likelihood.

STR:

  • Have just 1 validated pill (e.g. auto-Bcc)
  • paste a csv list of 50 recipients, without confirming them or clicking anywhere else
  • click Send immediately

Actual result:

  • upon sending, the whole set of 50 recipients is silently discarded!

Aleca, can you look into this?

Proposed fix:

  • Imho, Thunderbird should just disable the Send button as long as there are unfinished pills, because it's also error-prone if we do autocomplete-on-send and just pick the currently best autocomplete result or convert csv-lists to pills whilst the user isn't watching and has no way of double-checking because the message will be already out. We should expect users to confirm their recipients, at least by clicking somewhere in the message.
  • Personally, I find even autocomplete-on-blur quite questionable - what if another app steals focus whilst I'm in the middle of typing a recipient? Maybe we should only autocomplete for Enter, Tab, cursor-right (Bug 1672916).

Josh, I am unable to reproduce your first scenario - if there's no finished pill, send button should be disabled. Can you provide more precise STR for that case? Does it involve newsgroups or active addons? Every single detail matters so that we can reproduce.

Severity: -- → S2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(josh)
Flags: needinfo?(alessandro)
Priority: -- → P1
Summary: Email pill does not complete before clicking on Send → All non-pillified recipients (pasted, typed) silently discarded when clicking [Send] button directly

Technically, I'm actually surprised that clicking [Send] does not trigger blur on the address input, assuming that we pillify on blur... but it wouldn't be a very good/safe solution anyway as outlined above.

Josh, I am unable to reproduce your first scenario - if there's no finished pill, send button should be disabled. Can you provide more precise STR for that case? Does it involve newsgroups or active addons? Every single detail matters so that we can reproduce.

  • OpenSUSE Tumbleweed
  • Thunderbird 78.3.2 (64-bit)
  • KDE Plasma
  • Extensions: CardBook, Enigmail, Sieve, ThreadVis. (No extensions relating to Composition AFAIK).

The send button is never disabled for me (I didn't even realise it should be). When I open the compose window I can immediately press the Send button and receive the error about not having any recipients.

My opinion is that auto-complete should not happen on blur/de-focus. Instead if I'm partially through typing an address, it should just attempt to send with whatever that may be. For example, "person@example.com" should be sent exactly as that, not tried to complete to "Mr. Person <person@example.com.au>" as I may have been specifically sending something to the "example.com" domain (not .au).

Flags: needinfo?(josh)

Technically, I'm actually surprised that clicking [Send] does not trigger blur on the address input

Indeed, this is very strange and it should happen.

Imho, Thunderbird should just disable the Send button as long as there are unfinished pills...

Mh, I think we could implement that, but maybe we should also add a tooltip explanation when hovering over the disabled Send button. Something like "unfinished typed recipients" or something like that.
This might also be a bit confusing for users so we need to be careful.

Personally, I find even autocomplete-on-blur quite questionable - what if another app steals focus whilst I'm in the middle of typing a recipient? Maybe we should only autocomplete for Enter, Tab, cursor-right

We should keep autocomplete-on-blur as it's part of the natural progression for users that only use the mouse (eg. my wife never uses tab to move the cursor to another field, which drives me crazy, but that's another story).

Flags: needinfo?(alessandro)

(In reply to josh from comment #3)

Josh, I am unable to reproduce your first scenario - if there's no finished pill, send button should be disabled. Can you provide more precise STR for that case? Does it involve newsgroups or active addons? Every single detail matters so that we can reproduce.

  • OpenSUSE Tumbleweed

Did u download TB as distro package or directly from thunderbird.net?

  • Thunderbird 78.3.2 (64-bit)
  • KDE Plasma
  • Extensions: CardBook, Enigmail, Sieve, ThreadVis. (No extensions relating to Composition AFAIK).

At least Enigmail and CardBook might certainly relate to composition.

The send button is never disabled for me (I didn't even realise it should be). When I open the compose window I can immediately press the Send button and receive the error about not having any recipients.

Is the send button still always enabled after "Help > restart with addons disabled" ?

My opinion is that auto-complete should not happen on blur/de-focus. Instead if I'm partially through typing an address, it should just attempt to send with whatever that may be. For example, "person@example.com" should be sent exactly as that, not tried to complete to "Mr. Person <person@example.com.au>" as I may have been specifically sending something to the "example.com" domain (not .au).

Good points! You could file a bug or RFE if you're unhappy with the current behaviour.
Fyi, the current way of dealing with entering example.com (not in your AB) vs. example.com.au (in your AB) is that you are expected to manually remove the autocomplete suggestion using DEL or backspace before proceeding - which sort of reverts the burden into autcomplete inconvenience as you have already entered a complete and correct address...

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

[Thomas:] Personally, I find even autocomplete-on-blur quite questionable - what if another app steals focus whilst I'm in the middle of typing a recipient? Maybe we should only autocomplete for Enter, Tab, cursor-right

We should keep autocomplete-on-blur as it's part of the natural progression for users that only use the mouse (eg. my wife never uses tab to move the cursor to another field, which drives me crazy, but that's another story).

The question is, when your wife needs to enter person@example.com (not in AB) manually with person@example.com.au already in your AB, with inline autocomplete triggering, how do you know which address she really wants? All those guessing games without explicit user confirmation are actually a high risk of causing privacy violations via unintended addresses. Even <tab> to autocomplete is an abuse, because <tab> should be respected as a pure navigation key instead of changing my input. Firefox location bar certainly had their reasons to limit autocomplete to explicit confirmation with Enter - nothing else - take it or leave it! Tab from location bar will navigate into autocomplete suggestions and place them into location bar, but you still need to press Enter to accept the proposal.

Iow, stopping to type your recipient halfway and then expecting TB to autocomplete anyway without any explicit confirmation like Enter is a wrong and error-prone expectation.

(In reply to Thomas D. (:thomas8) from comment #5)
^^ ni?reporter

Flags: needinfo?(josh)

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

(In reply to josh from comment #3)

Josh, I am unable to reproduce your first scenario - if there's no finished pill, send button should be disabled. Can you provide more precise STR for that case? Does it involve newsgroups or active addons? Every single detail matters so that we can reproduce.

  • OpenSUSE Tumbleweed

Did u download TB as distro package or directly from thunderbird.net?

It is installed from system packages:

Repository : openSUSE-Tumbleweed-Oss
Name : MozillaThunderbird
Version : 78.4.0-1.1

  • Thunderbird 78.3.2 (64-bit)
  • KDE Plasma
  • Extensions: CardBook, Enigmail, Sieve, ThreadVis. (No extensions relating to Composition AFAIK).

At least Enigmail and CardBook might certainly relate to composition.

Indeed you are right (see below).

The send button is never disabled for me (I didn't even realise it should be). When I open the compose window I can immediately press the Send button and receive the error about not having any recipients.

Is the send button still always enabled after "Help > restart with addons disabled" ?

Starting in safe-mode has the Send button disabled in the composition window.

Restarting Thunderbird and then disabling CardBook appears to also have the Send button disabled. In other words, I think CardBook may be the offending plugin. https://addons.thunderbird.net/en-US/thunderbird/addon/cardbook (I am running the latest 52.6).

The bug still exists if I have a valid email address in the To field, I can have incomplete/partial addresses in CC or elsewhere which are subsequently lost on pressing Send.

My opinion is that auto-complete should not happen on blur/de-focus. Instead if I'm partially through typing an address, it should just attempt to send with whatever that may be. For example, "person@example.com" should be sent exactly as that, not tried to complete to "Mr. Person <person@example.com.au>" as I may have been specifically sending something to the "example.com" domain (not .au).

Good points! You could file a bug or RFE if you're unhappy with the current behaviour.

There appears to be some good points/discussions on this topic in this thread. I'll leave it for somebody else to open.

Flags: needinfo?(josh)
Duplicate of this bug: 1675780
See Also: → 1679848

Are we doing something to get this bug with substantial covert dataloss potential out of the way? See my STR in comment 1.

Flags: needinfo?(mkmelin+mozilla)

I'll try to take care of this bug later this week.
I agree that the Send button should be disabled if we have non-pillified values in the addressing rows.

Assignee: nobody → alessandro

Great!

Flags: needinfo?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attached patch 1674054-send-disable.diff (obsolete) — Splinter Review

This patch disables the send button on typing, running the check only once if the button is currently enabled.

It seems that I'm not able to disable the Ctrl+Enter shortcut, which sends the message regardless by creating pills of whatever string is present in the field.
I thought the keyboard shortcut would be disabled alongside the send button.
Am I missing something?

Attachment #9190706 - Flags: review?(mkmelin+mozilla)
Attachment #9190706 - Flags: feedback?(bugzilla2007)
Comment on attachment 9190706 [details] [diff] [review]
1674054-send-disable.diff

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

I think we need to figure out why it doesn't pillify in time onblur for these cases, which is likely the right solution. The patch looks like it wouldn't work for paste either.
Attachment #9190706 - Flags: review?(mkmelin+mozilla) → review-

Mhh...so here maybe the solution is to intercept the send action and go through these checks:

  • Pillify all the leftover strings in the input fields.
  • Check if any of those pill is invalid (prompt the user about it and interrupt the send).
  • Send.

Does this approach sound good?

Attached patch 1674054-pill-before-send.diff (obsolete) — Splinter Review

With this patch, all the leftover strings are pillified before sending.
The sending stops if there's an invalid address in the newly generated pills.

I didn't tackle the paste-and-convert-to-pill approach, which I'd like to handle in a dedicated bug as I think it needs a little bit more UX thinking to prevent unexpected behaviour.

Attachment #9190706 - Attachment is obsolete: true
Attachment #9190706 - Flags: feedback?(bugzilla2007)
Attachment #9190891 - Flags: review?(mkmelin+mozilla)
Attachment #9190891 - Flags: feedback?(bugzilla2007)
Blocks: 1679848
Comment on attachment 9190891 [details] [diff] [review]
1674054-pill-before-send.diff

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

LGTM, r=mkmelin

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4838,5 @@
> +      `input[is="autocomplete-input"][recipienttype]`
> +    );
> +    // If we find a leftover string in the input field, create a pill. If the
> +    // newly created pill is not a valid address, the sending will stop.
> +    if (input.value.trim().length) {

nit: don't need the .length as this is not an array
Attachment #9190891 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9190891 - Attachment is obsolete: true
Attachment #9190891 - Flags: feedback?(bugzilla2007)
Attachment #9191141 - Flags: review+
Target Milestone: --- → 85 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4859c8cc0e87
Convert leftover strings inside recipient inputs into pills before sending. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
See Also: → 1683064

Comment on attachment 9191141 [details] [diff] [review]
1674054-pill-before-send.diff

[Approval Request Comment]
Regression caused by (bug #): pills
User impact if declined: sending may leave out an address under certain conditions
Testing completed (on c-c, etc.): on c-c and beta
Risk to taking this patch (and alternatives if risky): not risky

Attachment #9191141 - Flags: approval-comm-esr78?

Comment on attachment 9191141 [details] [diff] [review]
1674054-pill-before-send.diff

[Triage Comment]
Approved for esr78

Attachment #9191141 - Flags: approval-comm-esr78? → approval-comm-esr78+
Duplicate of this bug: 1684390
Regressions: 1688336
Regressions: 1696789
Blocks: 1701313
You need to log in before you can comment on or make changes to this bug.