Bug 1629364 Comment 4 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

(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 <a@b.com> (space in display name, frequent use case)
- "a░b"@c.com (space in 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 exactly what I did. They just happen to touch the same spot.

> 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 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.
(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 <a@b.com> (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 exactly what I did. They just happen to touch the same spot.

> 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 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.
(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 <a@b.com> (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 exactly what I did. They just happen to touch the same spot.

> 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.
(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 <a@b.com> (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.

Back to Bug 1629364 Comment 4