1674177 - Address inputs allow entering recipients with leading comma again

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P3)

Description

[Thomas D. (:thomas8)]

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.

Comment 1

[Thomas D. (:thomas8)]

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

[Alice0775 White]

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

[Magnus Melin [:mkmelin]]

(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

[Magnus Melin [:mkmelin]]

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

[Alessandro Castellani [:aleca]]

I agree with Magnus, leading commas should be prevented without any exception.

Comment 6

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1674177 - Suppress typing a "leading" space or comma in address input and pills. r=aleca,thomasd

Comment 7

[Henry Wilkes (they/them) [:henry-x]]

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 typing oh and auto-completing will give you the leading comma in the resulting pill).

Comment 8

[Thomas D. (:thomas8)]

Alex, does this need a try-run before setting keyword checkin-needed-tb?

Comment 9

[Alessandro Castellani [:aleca]]

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.

Comment 10

[Henry Wilkes (they/them) [:henry-x]]

(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

[Alessandro Castellani [:aleca]]

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.

Comment 12

[Henry Wilkes (they/them) [:henry-x]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fd256326dd81
Suppress typing a "leading" space or comma in address input and pills. r=aleca