Bugzilla

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

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

So I suggest to land this as-is.
Objections?

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

~~`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).

So I suggest to land this as-is.
Objections?~~

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