Prevent whitespace-only value problem in recipient inputs: invisible space prevents pill navigation
Categories
(Thunderbird :: Message Compose Window, enhancement)
Tracking
(Not tracked)
People
(Reporter: thomas8, Assigned: thomas8)
References
Details
(Keywords: ux-error-prevention)
Attachments
(1 file, 3 obsolete files)
4.51 KB,
patch
|
aleca
:
review+
|
Details | Diff | Splinter Review |
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
- compose, type an address followed by comma and space into any recipient input:
|foo@bar.com, | (with an initial intention of adding another address) - note an error in the first address, should be "fox@bar.com"
- cursor left (repeatedly) to select
foo@bar.com
pill for editing
Actual result
- comma auto-creates pill (ok), invisible space (░) remains in input box, followed by cursor:
foo@bar.com
░| - pressing cursor left to select first pill for editing fails (due to the invisible space, i.e. without any obvious reason):
foo@bar.com
|░ - 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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Comment 2•5 years ago
|
||
Comment 3•5 years ago
|
||
Assignee | ||
Comment 4•5 years ago
•
|
||
(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.
Assignee | ||
Comment 5•5 years ago
|
||
(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•5 years ago
|
||
Can you rebase the patch with the latest merge?
Marginal things changed slightly, like comments messages and some eslint fixes.
Comment 7•5 years ago
|
||
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 hidden (obsolete) |
Assignee | ||
Comment 9•5 years ago
•
|
||
(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
withkeydown
oronbeforeinput
.
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/
Assignee | ||
Comment 10•5 years ago
|
||
(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.
Assignee | ||
Comment 11•5 years ago
|
||
(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.
Assignee | ||
Comment 12•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #6)
Can you rebase the patch with the latest merge?
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•4 years ago
•
|
||
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. ;-)
Assignee | ||
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Comment 16•4 years ago
|
||
Just a tiny linting error, everything else looks good.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 17•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b7282a2b7a3d
Handle whitespace and respective UX issues in recipient inputs. r=aleca
Assignee | ||
Comment 18•4 years ago
|
||
(In reply to Pulsebot from comment #17)
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b7282a2b7a3d
Handle whitespace and respective UX issues in recipient inputs. r=aleca
Awesome. Thank you!
Assignee | ||
Updated•4 years ago
|
Description
•