Address inputs allow entering recipients with leading comma again
Categories
(Thunderbird :: Message Compose Window, defect, P3)
Tracking
(thunderbird_esr78 wontfix)
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | wontfix |
People
(Reporter: thomas8, Assigned: henry-x)
References
(Regression)
Details
(Keywords: regression, ux-error-prevention, ux-error-recovery)
Attachments
(1 file)
Bug 1674177 - Suppress typing a "leading" space or comma in address input and pills. r=aleca,thomasd
48 bytes,
text/x-phabricator-request
|
Details | Review |
Alex and I had explicitly prevented starting a new recipient in address input by typing a comma, which can easily create wrong recipients where the comma becomes part of the new pill. Unfortunately, this has regressed (Alice, you may, if you want ;-)).
STR
- compose new msg
- in To-field, type ","
Actual result
- comma appears in address input
Expected
- Nothing should change. Leading (sic!) comma should be suppressed, i.e. it should not even appear in the input, as if you had never pressed the key.
Reporter | ||
Comment 1•4 years ago
|
||
Note: Normally we do not require quotes for display names containing comma, but for the case of this bug, if you really need your email address or display name to start with a comma (extremly odd, but valid), well - you'd have to apply the quotes manually in that exceptional case.
Comment 2•4 years ago
|
||
Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=1c9f4fdf777217c65e8760abbc9d5ef6215927b9&tochange=306ac4b77c679440f825d487a9147544591db953
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1effb11e8e178a9b1f25ba171f4d52f857ebb713&tochange=5bfe22b4bd0d64b9d105b66b3ed0fcee10d70e93
Comment 3•4 years ago
|
||
(In reply to Thomas D. (:thomas8) from comment #1)
If you really need your email address or display name to start with a comma (extremely odd, but valid), well - you'd have to apply the quotes manually in that exceptional case.
That's not really the case. A quote would (should!) be literally a quote.
Comment 4•4 years ago
|
||
Regressed by https://hg.mozilla.org/comm-central/rev/1d6c19070bff
I guess it should really just test if the comma is the first char in the value? if (/^,/.test(element.value))
and prevent it if that's the case.
Comment 5•4 years ago
|
||
I agree with Magnus, leading commas should be prevented without any exception.
Assignee | ||
Comment 6•4 years ago
|
||
Assignee | ||
Comment 7•4 years ago
|
||
So the current fix is very basic. It just suppresses a typed leading ,
(so if the address is otherwise empty, typing ,
will do nothing).
A leading comma can still be used as part of the address by inserted through:
- pasting in the
,
- editing the pill after it is created
- through an existing address that is autocompleted (e.g., if in the address book you have
,oh <example@org>
then typingoh
and auto-completing will give you the leading comma in the resulting pill).
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Reporter | ||
Comment 8•4 years ago
|
||
Alex, does this need a try-run before setting keyword checkin-needed-tb
?
Comment 9•4 years ago
|
||
I think we should, at least running the mochitest to be sure we didn't affect any area of the composition.
We don't have any test covering this scenario, tho, and I don't if Henry wants to add one in the patch.
What do you think Henry? I'm okay with landing this and adding a small test in a follow up bug.
Assignee | ||
Comment 10•4 years ago
|
||
(In reply to Alessandro Castellani [:aleca] from comment #9)
I think we should, at least running the mochitest to be sure we didn't affect any area of the composition.
We don't have any test covering this scenario, tho, and I don't if Henry wants to add one in the patch.
What do you think Henry? I'm okay with landing this and adding a small test in a follow up bug.
I think I ran this on try for an earlier version, but not the latest. So I just started a new job here https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ff119eee13aa6e4e1a0bdf86c9918be36cdacbf5
Regarding a test for this scenario. I could write a new test, but it would be the first test I've added, rather than just modified. So I'm not familiar with what methods or libraries are available or recommend. But I need to learn at some point, so if you want me to add a mochitest, could you point me to an existing up to date one for guidance? Also, is there some file I need add the test's path to, or will it be automatically included?
Comment 11•4 years ago
|
||
Writing tests at first can be a bit overwhelming and annoying, so I'm happy to guide you as much as possible.
We already have a couple of test files that handle the generation and handling of pills.
browser_addressWidgets.js: https://searchfox.org/comm-central/rev/e80e0b49d038b6398deaaaa0d599a3cb5c0a9457/mail/test/browser/composition/browser_addressWidgets.js
browser_replyAddresses.js: https://searchfox.org/comm-central/rev/e80e0b49d038b6398deaaaa0d599a3cb5c0a9457/mail/test/browser/composition/browser_replyAddresses.js
If you'll ever need to create a new file, use the same browser_
prefix and put it in the correct location.
As you can see both these files are in the test/browser/composition/
path.
After adding the file, you need to update the INI file in the same drectory to include this new test file: https://searchfox.org/comm-central/source/mail/test/browser/composition/browser-shared.ini
For this, I think we can use the browser_addressWidgets.js
as it's fairly small and it comes already with a setupModule that generates a test account to start creating fake compose emails.
We have a small guide here: https://developer.thunderbird.net/thunderbird-development/testing/writing-mochitest-tests
Up to you if you want to add the test in this patch or create a follow up.
Whatever you feel is more comfortable.
Assignee | ||
Comment 12•4 years ago
|
||
(In reply to Alessandro Castellani [:aleca] from comment #11)
Up to you if you want to add the test in this patch or create a follow up.
Whatever you feel is more comfortable.
Thanks for the info :)
I think I will have a go at writing a test for this.
Assignee | ||
Updated•4 years ago
|
Comment 13•4 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/fd256326dd81
Suppress typing a "leading" space or comma in address input and pills. r=aleca
Updated•4 years ago
|
Updated•4 years ago
|
Description
•