1701313 - Send button still remains disabled in spite of valid, non-pillified recipient address(es) for many text input methods. Should also prevent sending to autocomplete artifact "x >> max"@bar.com (privacy bug 632127)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1679848 +++

We're still failing to enable the send button for many input methods of a valid, non-pillified email address because of using the wrong event (keyup instead of input) and some other flaws.

Steps to reproduce:

Create a new message and provide a single RFC 822 email address in any addressing field (To/CC/BCC) using any of the following methods:

• type single (sic!) character which autocompletes directly inline (without >>): [e-mail]
• drag [e-mail] from body text to input
• copy [e-mail] from somewhere and paste it to input via context menu (mouse-only)
• or use context menu key, cursor down to Paste, press Enter (keyboard-only)

Reverse case scenario, Send button still enabled when it should not be:

• have one valid email pill and type a character to trigger an indirect inline autocomplete (with >>), do not confirm in any way, just press Send (Bug 632127)
  [e-mail] x>> [e-mail] (use x as nickname to force top match)

Actual results:

• For all of the above input methods, Send button remains disabled in spite of single, valid, non-pillified address in input
• For the reverse case: message gets sent to "x >> max"@bar.com, which is a valid address, but not your intended recipient, which is a privacy violation which Thunderbird could easily avoid (ux-error-prevention).

Expected results:

• Send button should be consistently enabled if there's a single, valid, non-pillified email address, regardless of the input method
• Send button should be disabled to prevent sending to non-pillified autocomplete artifact address "x >> max"@bar.com, for ux-error-prevention and privacy protection.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1701313_fixSendButtonLockStatus.diff

Prevent sending to autocomplete artifact address (bug 632127) and fix Send button enabling for many input methods of non-pillified address.

• Fix bug 632127 by disabling Send button as long as non-pillified autocomplete
  artifacts like x >> [e-mail] are present in an address input (privacy,
  ux-error-prevention).
• Listen to input event instead of keyup to trigger onRecipientsChanged()
  and updateSendLock(), to enable Send button for valid, non-pillified email
  address(es) regardless of text input method (e.g. drag and drop, paste via
  mouse, inline autocomplete from single typed character etc.).
• Related code cleanup.

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9211910 [details] [diff] [review]
1701313_fixSendButtonLockStatus.diff

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

Good improvements, thanks.

Quick question adjacent to this patch, have we ever considered disabling the Send button if there's at least one invalid address in the recipient fields?
Is that something that could be useful to avoid accidental submissions in case the user misses the red pill in a long scrolled list of addresses?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4960,5 @@
> +        ? isValidNewsAddress(inputValueTrim)
> +        : isValidAddress(inputValueTrim)
> +    ) {
> +      gSendLocked = false;
> +      continue;

No need to continue here since there's no code running after this condition.

Comment 3

[Thomas D. (:thomas8)]

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

Review of attachment 9211910 [details] [diff] [review]:
Good improvements, thanks.

Quick question adjacent to this patch, have we ever considered disabling the
Send button if there's at least one invalid address in the recipient fields?

Yeah, I was thinking about that...

Is that something that could be useful to avoid accidental submissions in
case the user misses the red pill in a long scrolled list of addresses?

Per current design, accidental send submissions with red pill will be blocked by invalid address alert, which is kinda helpful because it also mentions the first offending pill.

We could explore disabling the send button instead (in a new bug), but then we should probably have some other hint pointing out the presence of invalid pill(s).

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4960,5 @@

•    ? isValidNewsAddress(inputValueTrim)
  

•    : isValidAddress(inputValueTrim)
  

• ) {

•  gSendLocked = false;
  

•  continue;
  

No need to continue here since there's no code running after this condition.

[Edit] That's true, I will fix it.

continue is required because... even if we find a single valid pill e.g. in To (which enables Send per current logic as it does not check for subsequent invalid pills), we still want to check for autocomplete artifacts having >> in CC and any other rows (which will disable Send per this bug, to prevent sending the artifact as a valid address).

Comment 4

[Thomas D. (:thomas8)]

Attachment: 1701313_fixSendButtonLockStatus.diff

Alright! Nitfixed removal of 'continue' and added a comment.

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9212358 [details] [diff] [review]
1701313_fixSendButtonLockStatus.diff

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

Awesome, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/86b172990379
Prevent sending to autocomplete artifact address (bug 632127) and fix Send button enabling for many input methods of non-pillified address. r=aleca