1629364 - Prevent whitespace-only value problem in recipient inputs: invisible space prevents pill navigation

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

Whitespace-only values in composition's recipient inputs are invalid, useless, and undesired. They occur easily especially when entering comma-separated email addresses. They are invisible and hence cunningly prevent keyboard navigation into adjacent pills. Instead of adding more hooks and checks to work around or fix them later in unrelated events, we can just prevent them from occuring.

STR

1. compose, type an address followed by comma and space into any recipient input:
   |[e-mail], | (with an initial intention of adding another address)
2. note an error in the first address, should be "[e-mail]"
3. cursor left (repeatedly) to select [e-mail] pill for editing

Actual result

• comma auto-creates pill (ok), invisible space (░) remains in input box, followed by cursor:
  [e-mail] ░|
• pressing cursor left to select first pill for editing fails (due to the invisible space, i.e. without any obvious reason):
  [e-mail] |░
• confusingly, we sanitize at some random time after the fact, but only for certain scenarios (Enter)

Expected result

• Noting that...
  • an email address cannot begin with spaces unless quoted
  • nobody starts an email address with spaces by putting spaces first
  • there are several other ways to remain with spaces-only in the input
  • there are many ways to unfocus an input (enter, tab, click elsewhere)
  • whitespace-only in the input is neither useful nor valid unless with quotes or other characters
• ... we should just prevent whitespace-only values from occuring, which is much easier, safer, and better ux
• I.e. for the STR above, eliminate the space after the comma as soon as the first pill is created, and allow immediate cursor-left navigation to correct the first pill.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1629364_sanitizeEmptyRecipientInput.diff (applies on top of bug 1629144, attachment 9139908)

Simple fix. As we now prevent whitespace-only input.value from occuring, we no longer have to check for that nor sanitize elsewhere, whereby existing sanitation was incomplete/random anyway.

Comment 2

[Thomas D. (:thomas8)]

Comment on attachment 9140029 [details] [diff] [review]
1629364_sanitizeEmptyRecipientInput.diff (applies on top of bug 1629144, attachment 9139908 [details] [diff] [review])

Please note that the patch here applies on top of bug 1629144, attachment 9139908 [details] [diff] [review].

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9140029 [details] [diff] [review]
1629364_sanitizeEmptyRecipientInput.diff (applies on top of bug 1629144, attachment 9139908 [details] [diff] [review])

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

I think we should leverage the method we already have in place instead o creating a new one.
As you correctly said, having a blank space when writing an email is pretty disruptive, so we could prevent it altogether.

What id we update the switch inside the recipientKeyPress() event to disable typing a blank space at all?
```
case " ":
      event.preventDefault();
      break;
```

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +575,1 @@
>          SetFocusOnNextAvailableElement(element);

You shouldn't add the code of another patch to your new patch.
If the two patches are independent, be sure to remove the previous one before working on a new one.
If the new patch is dependent on the previous one, you should create a new patch on top of the previous one, but avoiding implementing the same code.

@@ +592,5 @@
> + *
> + * @param {Event} event - The DOM input event.
> + * @param {HTMLElement} element - The element that triggered the input event.
> + */
> +function recipientInput(element) {

I'm not sure we should add a whole other method and listener to an already pretty filled input field.

Comment 4

[Thomas D. (:thomas8)]

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

Note: In the following, space character represented by ░.

Comment on attachment 9140029 [details] [diff] [review]
1629364_sanitizeEmptyRecipientInput.diff (applies on top of bug 1629144,
attachment 9139908 [details] [diff] [review])

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

I think we should leverage the method we already have in place instead o
creating a new one.

keypress event isn't right for this (using keypress is actually deprecated, especially for cases involving text input*). There are dozens of keypresses which do not change the input, like all movement keys. So we'd fire input.value sanitation more often than needed. As I tried to hint at the top of comment 0, we'd have to cover a lot of different keypress "hooks" (e.g. tab, Ctrl+V, Ctrl+X, DEL, backspace etc.), and that would still not cover all possible ways of changing input, which includes drag and drop, paste from context menu via mouse, etc. Pls note that per current summary of this bug, technically I'm not preventing whitespace input per se (which we cannot, see below), but I'm only sanitizing all cases of (input.value == whitespace-only). E.g., you can type "John Doe░" in To-field, then decide you want John in CC, accidentally cut only "John Doe" and remain with invisible "░" in the input. Then start cursing Thunderbird because cursor-left will no longer select the last pill.

As you correctly said, having a blank space when writing an email is pretty
disruptive, so we could prevent it altogether.

My patch prevents exactly and only what we can safely prevent, at exactly the right spot (input event), namely that input.value (in its entirety) must not be whitespace-only.

What id we update the switch inside the recipientKeyPress() event to disable
typing a blank space at all?

case " ":
      event.preventDefault();
      break;

That would not work at all because it would prevent entering valid email addresses like the following:

• John░Doe <[e-mail]> (space in display name, frequent use case)
• "a░b"@c.com (space in quoted local part of email address; discouraged, but valid.)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +575,1 @@

     SetFocusOnNextAvailableElement(element);

You shouldn't add the code of another patch to your new patch.

Well, that's not what I did. They just happen to touch the same spot sequentially.

If the two patches are independent, be sure to remove the previous one
before working on a new one.

Yeah, maybe they are independent in spite of touching the same spot, should have done that probably. My bad, sorry for that.
It might become a bit of a merry-go-round though, because then I'd have to unbitrot the first patch again.

If the new patch is dependent on the previous one, you should create a new
patch on top of the previous one, but avoiding implementing the same code.

That's exactly what I did: new patch on top of the previous one, not implementing the same code, but subsequent patch happens to change different things in the same lines. I don't think we should merge them because they do different things, and we should keep separate things separate also if we ever want to refactor the whitespace-sanitation in some other way and avoid breaking unrelated things.

@@ +592,5 @@

• • @param {Event} event - The DOM input event.
• • @param {HTMLElement} element - The element that triggered the input event.
• */
  +function recipientInput(element) {

I'm not sure we should add a whole other method and listener to an already
pretty filled input field.

Well, I'm all for streamlining code, but otoh, counting methods and listeners shouldn't stop us from doing the correct thing. Doing this in keypress can't cover all cases, and would just add input.value validation clutter to keypress, and/or fire where it's not needed (see start of this comment).

Btw, this patch does not yet cover all cases of possible whitespace-sanitation, but it makes it very easy to add more as we'll have a dedicated, non-deprecated listener with its own method, recipientInput().

*: https://developer.mozilla.org/en-US/docs/Web/API/Element/keypress_event
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent

When you need to handle text input, use the input event instead.

Comment 5

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #4)

(In reply to Alessandro Castellani (:aleca) from comment #3)
That's exactly what I did: new patch on top of the previous one, not implementing the same code, but subsequent patch happens to change different things in the same lines. I don't think we should merge them because they do different things, and we should keep separate things separate also if we ever want to refactor the whitespace-sanitation in some other way and avoid breaking unrelated things.

Fwiw, and as anticipated, the first patch has landed in bug 1629144, so the patch here (attachment 9140029 [details] [diff] [review]) now applies seamlessly.

Comment 6

[Alessandro Castellani [:aleca]]

Can you rebase the patch with the latest merge?
Marginal things changed slightly, like comments messages and some eslint fixes.

Comment 7

[Alessandro Castellani [:aleca]]

I was looking at this again and I was wondering if you considered using the onblur attribute to strip away leftover whitespaces.
Also, we should definitely open a bug to replace the keypress with keydown or onbeforeinput.

Comment 8

[Alessandro Castellani [:aleca]]

I noticed another behaviour that we took care in the past, and I don't know if it could be related to this.
Now, if I type address and hit Enter to accept the autocomplete suggestion, the cursor goes directly on the Subject line, instead of staying on the input field.

Comment 9

[Thomas D. (:thomas8)]

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

Can you rebase the patch with the latest merge?

Ok, will do!

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

I was looking at this again and I was wondering if you considered using the onblur attribute to strip away leftover whitespaces.

I did and discarded the idea. onblur is too late, as we seem to agree that we want to prevent scenarios of (input.value==whitespace-only) to even occur and remain in the UI for some time. Before blur, as long as the whitespace-only input.value still hangs around, keyboard-navigation from input to last pill via cursor is blocked, which is a highly irritating/annoying UI state as the trouble-causing whitespace is invisible.

Also, we should definitely open a bug to replace the keypress with keydown or onbeforeinput.

Generally yes, although I don't see keypress disappearing any time soon, so maybe it's not urgent. I doubt that onbeforeinput will be helpful in our case. As for keydown/keyup, iirc, tab key might cause problems, as keydown and keyup used to fire on different elements when [shift+]tab changes focus, and keypress didn't suffer from that. Alas, it's deprecated...

If you find yourself wondering (like me) wrt the WHY of keypress deprecation:
https://www.mutuallyhuman.com/blog/keydown-is-the-only-keyboard-event-we-need/

Comment 10

[Thomas D. (:thomas8)]

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

I noticed another behaviour that we took care in the past, and I don't know if it could be related to this.
Now, if I type address and hit Enter to accept the autocomplete suggestion, the cursor goes directly on the Subject line, instead of staying on the input field.

I'd be surprised if this is caused here, but we should check that out.

Comment 11

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #10)

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

I noticed another behaviour that we took care in the past, and I don't know if it could be related to this.
Now, if I type address and hit Enter to accept the autocomplete suggestion, the cursor goes directly on the Subject line, instead of staying on the input field.

I'd be surprised if this is caused here, but we should check that out.

...the regression wasn't caused by this bug but rather by the changes done in the toolkit autocomplete in bug 1627520. See TB bug 1630765.

Comment 12

[Thomas D. (:thomas8)]

Attachment: 1629364_sanitizeEmptyRecipientInput.diff

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

Can you rebase the patch with the latest merge?

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9141516 [details] [diff] [review]
1629364_sanitizeEmptyRecipientInput.diff

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

Thanks for updating this.
After further consideration, I still think it's best to not implement another event handler in the addressing fields.

If we analyze the problem, the only real issue here is if the user starts typing a blank space as a first character, which as you correctly said is wrong and unusual.

We can prevent that by updating the `handleKeyPress()` method with:
```
case " ":
      // Prevent the typing of a blank space as a first character.
      if (!element.value.trim()) {
        event.preventDefault();
      }
      break;
```

We are already firing this method at every keypress and covering various scenarios, so adding another condition to the switch method seems the most straightforward solution in order to prevent this problem for all the addressing fields.

The other problem we could have is that if the user manages somehow to add a blank space in the field.
We can further fix this potential issue by clearing the the input value `onblur` and `onfocus` since we already have those event firing anyway.

Comment 14

[Thomas D. (:thomas8)]

Attachment: 1629364_sanitizeEmptyRecipientInput.reloaded.diff

Alright. Here's whitespace handling reloaded. Starting from Alex' comment 13, I added some tweaks to further improve the UX.

• Per Alex comment 13, prevent the most likely way of whitespace-only input (pressing spacebar), and deal with any remaining whitespace-only on blur. However, main problem of this bug (invisible whitespace preventing keyboard navigation from input to pills) can still occur (before blur), as there's a number of other ways which can result in whitespace-only to remain in the input, e.g. deleting/dragging/cutting visible input text, but not all of the leading or trailing whitespace.
• The tweak: Let's address the key problem of this bug directly - whitespace-only value in a recipient input should NOT per se block keyboard navigation into pills (backspace, cursor-left, home). We just have to put the brakes on repeated keypresses, and make sure cursor is on the left side of the input, so as to avoid accidents for backspace and unexpected jumps. So we allow controlled moving of focus, and onblur immediately deals with the rest. It just works :-)
• Whilst we are here, streamline resetAddressContainer() a bit, and trim invalid input text on blur if it hasn't been converted to a pill yet.

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

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

Thank you. This review was a bit of a nut to crack, but ultimately helpful. ;-)

Comment 15

[Alessandro Castellani [:aleca]]

Comment on attachment 9144817 [details] [diff] [review]
1629364_sanitizeEmptyRecipientInput.reloaded.diff

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

LGTM, thanks for taking care of this. r+

I launched a try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=42660e6748caf4056291e9b5a09a108658be8c13
I'll mark this for check in after a green run.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1629364_sanitizeEmptyRecipientInput.diff

Just a tiny linting error, everything else looks good.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b7282a2b7a3d
Handle whitespace and respective UX issues in recipient inputs. r=aleca

Comment 18

[Thomas D. (:thomas8)]

(In reply to Pulsebot from comment #17)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b7282a2b7a3d
Handle whitespace and respective UX issues in recipient inputs. r=aleca

Awesome. Thank you!