Support pasting lists of addresses into mailing list dialogs
Categories
(Thunderbird :: Address Book, enhancement)
Tracking
(thunderbird_esr68+ fixed, thunderbird70 fixed, thunderbird71 fixed)
People
(Reporter: pmorris, Assigned: pmorris)
References
Details
Attachments
(7 files, 5 obsolete files)
4.55 KB,
patch
|
Details | Diff | Splinter Review | |
1.95 KB,
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr68+
|
Details | Diff | Splinter Review |
4.80 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
4.81 KB,
patch
|
Details | Diff | Splinter Review | |
6.25 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
4.66 KB,
patch
|
pmorris
:
feedback+
|
Details | Diff | Splinter Review |
4.29 MB,
video/webm
|
Details |
Support pasting in a list of comma-separated email addresses into a mailing list via the address book, like we do elsewhere (in the compose window and in the calendar invite attendees dialog).
On current trunk when you paste in a list of email addresses and hit "Enter" or "Tab" the addresses all stay on the line where you pasted them, rather than being split into one address per line.
Then when you click the "OK" button some of the addresses are captured but not always all of them.
This is for both the "create a new mailing list" dialog and the "edit existing mailing list" dialog.
Discovered while review patches for bug 1569492.
Assignee | ||
Updated•5 years ago
|
Comment 1•5 years ago
|
||
You encouraged a "boy scout" fix for this in bug 1569492. Is it really easy, then by all means, let's do it.
Assignee | ||
Comment 2•5 years ago
|
||
This does the trick. Open the address book, create or edit a mailing list, and paste in a list of addresses like this: one@one.com, two@two.com, three@three.com, four@four.com
then hit return. The addresses are split out into one row each.
I borrowed some logic from calendar's invite attendees dialog:
https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#1280
Might be worth making an address input parsing function as a common utility that can be used in both places.
Comment 3•5 years ago
|
||
Comment on attachment 9087161 [details] [diff] [review] mailing-list-address-lists-0.patch Review of attachment 9087161 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +775,5 @@ > } > > /** > + * Takes a string, like those entered into a mailing list dialog address field, > + * and returns an array of address entries. I don't think you should be doing it like this. Isn't MailServices.headerParser.extractHeaderAddressMailboxes the thing to use? @@ +782,5 @@ > + * list of addresses. > + * @return {string[]} Array of address entries. > + */ > +function awAbParseAddresses(addressString) { > + const parseHeaderValue = (msgIAddressObject) => { think I'd prefer let for this @@ +783,5 @@ > + * @return {string[]} Array of address entries. > + */ > +function awAbParseAddresses(addressString) { > + const parseHeaderValue = (msgIAddressObject) => { > + const name = msgIAddressObject.name; and this @@ +817,1 @@ > if (element.value != "") { please fix this to just check if (element.value) @@ +835,5 @@ > + } > + > + row = originalRow; > + for (let address of addresses) { > + inputElement = awGetInputElement(row) || awAppendNewRow(false); awAppendNewRow is sometimes null it seems
Assignee | ||
Comment 4•5 years ago
|
||
Thanks for the review. This addresses the comments and also improves a few things.
Comment 5•5 years ago
|
||
Comment on attachment 9088173 [details] [diff] [review] mailing-list-address-lists-1.patch Review of attachment 9088173 [details] [diff] [review]: ----------------------------------------------------------------- Seems good, thx! r=mkmelin ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +782,5 @@ > * @param element The element that triggered the keypress event > */ > function awAbRecipientKeyPress(event, element) { > // Only add new row when enter was hit (not for tab/autocomplete select). > if (event.key == "Enter") { looks like you should handle tab as well @@ +791,3 @@ > event.preventDefault(); > + > + const originalRow = awGetRowByInputElement(element); let @@ +801,5 @@ > + row = originalRow + 1; > + inputElement = awGetInputElement(row); > + > + while (inputElement) { > + if (inputElement.value != "") { if (inputElement.value)
Assignee | ||
Comment 6•5 years ago
|
||
Thanks for review. This addresses the comments except for handling tab key, which is done in a new part2 patch.
Assignee | ||
Comment 7•5 years ago
|
||
Handles tab key press when entering a list of comma-separated addresses. Addresses are split into their own rows and focus moves to the "Cancel" button without adding an empty row at the end of the address rows.
Comment 8•5 years ago
|
||
Comment on attachment 9088436 [details] [diff] [review] part2-tab-key-for-address-lists-0.patch Review of attachment 9088436 [details] [diff] [review]: ----------------------------------------------------------------- I notice that if you have an existing list from which you take some out, and try to paste - then it doesn't expand properly. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +808,2 @@ > > + while (inputElement) { while (inputElement = awGetInputElement(row)) { ? @@ +815,1 @@ > inputElement = awGetInputElement(row); and remove this
Assignee | ||
Comment 9•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #8)
I notice that if you have an existing list from which you take some out, and
try to paste - then it doesn't expand properly.
Can you give some more details on what you see not working properly? I just double-checked and it works for me. The addresses are split into one row each, filling up the empty rows, and bumping any following addresses down below.
So this:
- a
- a, b, c
- d
becomes:
- a
- a
- b
- c
- d
while (inputElement) {
while (inputElement = awGetInputElement(row)) { ?
I'm hesitant to do it this way because it looks so close to:
while (inputElement == awGetInputElement(row)) {
You have to look twice to see what's going on. Not sure it's an improvement for easy reading/understanding. Or maybe I just need to get used to this idiom of using an assignment as an expression.
Comment 10•5 years ago
|
||
Whenever the list has "erased lines", pasting something into those (and then Enter or Tab) won't expand the entries. Expansion only works for the last row. I think it saves the list correctly though (?).
Assignee | ||
Comment 11•5 years ago
|
||
Hm, that's not what I'm seeing. I can erase the contents of several rows, paste in a list on one of those erased rows, hit enter or tab, and they are split up into multiple rows as expected.
Comment 12•5 years ago
|
||
Did you use an existing list?
Comment 13•5 years ago
|
||
Need something to land, so I'm taking part 1.
Comment 14•5 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/a017484882ab Improve adding lists of addresses into mailing list dialogs. r=mkmelin
Assignee | ||
Comment 15•5 years ago
|
||
New patch 2 rebased with Prettier auto-formatting.
And I'm seeing this now with a previously saved mailing list. The problem is the element.value
(near line 934) has stale data. For some reason that property has the single address that was saved and not what you just typed into that row. Will circle back and look closer after doing some other work.
Assignee | ||
Comment 16•5 years ago
•
|
||
part2 of (now) 3: (part1 has already landed)
Digging deeper into this I uncovered another bug. This fixes it, and that allows my "paste address lists" changes to work as expected, even for existing/saved mailing lists. From the commit message of this patch:
Bug 1575046 - Allow users to edit existing mailing lists
If a user opened a saved mailing list (from the address book
dialog) and tried to edit or delete one of the addresses,
then clicked the OK button, the changes were not saved.
Perhaps this was caused by de-XBL changes? The fix was to
not set the 'textbox.value' property (as introduced by
the fix for bug 37435), since that was preventing that
property from updating to what the user typed in.
I tested to see if the problem from bug 37435 returned
after this change and was not able to reproduce it.
All message recipients were sent the message as
expected.
Assignee | ||
Comment 17•5 years ago
|
||
3 of 3: Just rebased and renamed. Now works due to fix in part2.
Comment 18•5 years ago
|
||
Comment on attachment 9091067 [details] [diff] [review] part2-fix-editing-address-lists-0.patch Review of attachment 9091067 [details] [diff] [review]: ----------------------------------------------------------------- Good find! r=mkmelin
Comment 19•5 years ago
|
||
Comment on attachment 9091068 [details] [diff] [review] part3-tab-key-for-address-lists-2.patch Review of attachment 9091068 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +916,3 @@ > * > + * @param {KeyboardEvent} event The DOM keypress event. > + * @param {Element} element The element that triggered the keypress event. While you're at it, please for these add a - before the comment, like jsdoc recommends. @param {Element} element - The element that triggered the keypress event.
Updated•5 years ago
|
Assignee | ||
Comment 20•5 years ago
|
||
Made that small edit to the doc strings. Thanks for pointing it out.
Assignee | ||
Updated•5 years ago
|
Comment 21•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #19)
While you're at it, please for these add a - before the comment, like jsdoc
recommends.@param {Element} element - The element that triggered the keypress event.
That would be the first dash we have in the system. Maybe it's for another bug to redo them consistently.
Comment 22•5 years ago
|
||
Comment on attachment 9091155 [details] [diff] [review] part3-tab-key-for-address-lists-3.patch Undesired hunk slipped in. Let's use the original.
Updated•5 years ago
|
Assignee | ||
Comment 23•5 years ago
•
|
||
Fix a glitch with the previous patch that snuck in when doing commit --amend
. This is the one to check in.
Edit: Fine with me to land the one without the "-". Thanks for catching that, jorgk.
Comment 24•5 years ago
|
||
Comment on attachment 9091067 [details] [diff] [review] part2-fix-editing-address-lists-0.patch This approval is for parts 2 and 3 for beta since part 1 is already in TB 70. Paul wanted to check out the ESR 68 version next week. I suggested to get a build from treeherder, unpack omni.ja and patch specific files.
Comment 25•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3358ca95426c
Allow users to edit existing mailing lists. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a904da1f13fe
Improve tab key behavior when entering address lists. r=mkmelin
Comment 26•5 years ago
|
||
Last two parts landed on TB 71.
Comment 27•5 years ago
|
||
TB 70 beta 1:
https://hg.mozilla.org/releases/comm-beta/rev/aef81082399d8a099f4715bc9120fd2ac6764f3a
https://hg.mozilla.org/releases/comm-beta/rev/1ed138b39b4fceb2c8de1757d8ccb35b22a6cd1d
Comment 28•5 years ago
|
||
(In reply to Pulsebot from comment #25)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3358ca95426c
Allow users to edit existing mailing lists. r=mkmelin
Yes, editing mailing lists is broken in TB 68, so we need to fix it.
Updated•5 years ago
|
Comment 29•5 years ago
|
||
We have a problem here. Part 1 doesn't apply since bug 1569492 made changes on trunk/70 which aren't in 68, particularly here:
https://hg.mozilla.org/comm-central/rev/1f2b56d3ba480f761ed138e2f4660e3ab9ea6350#l6.20
So at least part 1 needs to be rebased, and we might as well do this after making c-esr68 "prettier".
Hopefully parts 2 and 3 will then apply without a problem.
Assignee | ||
Comment 30•5 years ago
|
||
Looks like getting part 1 applied on esr68 will require making some of the changes from bug 1569492, namely the conversion of awRecipientTextCommand
to awAbRecipientKeyPress
. That looks fairly straightforward at a quick glance. Another option would be to just not back-port these patches (clearly nicer for users if we did).
Assignee | ||
Comment 31•5 years ago
•
|
||
Since I was looking at this, I went ahead and made a "part0" patch for TB 68 to prepare the way for the other patches. It's just the needed parts from bug 1569492.
aleca - flagging you to look over these changes and make sure I'm not missing anything since you worked on bug 1569492. No rush, as the plan is to land it after the Prettier formatting of TB 68. (I think these changes are already in Prettier style.)
Comment 32•5 years ago
|
||
Comment on attachment 9092383 [details] [diff] [review] part0-prep-esr68-0.patch Looks OK to me. Backports to TB 68 are always tricky/risky, so the best approach it so land it and try it, like I always do. With this part first, parts 1-3 apply cleanly with a minute tweak to part 3. Better to land it all now before the prettifying ;-)
Comment 33•5 years ago
|
||
TB 68.1.1 or TB 68.2:
https://hg.mozilla.org/releases/comm-esr68/rev/1f74906fee2f0980ec552146596f5bee261fb4d2
https://hg.mozilla.org/releases/comm-esr68/rev/ffe571b2644faf2086a661696dedb42a6b2385ea
https://hg.mozilla.org/releases/comm-esr68/rev/13b18028d225a6d82bff99852e0f2147449dae56
https://hg.mozilla.org/releases/comm-esr68/rev/bfd115c413ffa678e313c3ce33c44bbc7f28c7b3
Comment 34•5 years ago
|
||
That part 0 broke auto-complete click, see bug 1580950.
I guess I can just "back out" that part in the XUL files and put awRecipientTextCommand() back.
Comment 35•5 years ago
|
||
I'll see what that does.
Comment 36•5 years ago
|
||
Comment 37•5 years ago
|
||
Comment on attachment 9092483 [details] [diff] [review] 1575046-follow-up.patch - NOT USED This puts back what you took out. Or do you have a better idea? Some fix for bug 1580950.
Comment 38•5 years ago
|
||
My build actually works. So let's hear what the new auto-complete experts say ;-)
Comment 39•5 years ago
|
||
Wait, this works for me as expected without Jorg's rollback patch.
I'm attaching a video to show you, it's TB 68 with a recent pull, so all the Paul's patches in this bug are applied.
- Hitting Enter on an autocomplete entry works
- Mouse clicking on an autocomplete entry works
Am I doing something wrong?
Comment 40•5 years ago
|
||
Comment 41•5 years ago
•
|
||
Comment on attachment 9092483 [details] [diff] [review] 1575046-follow-up.patch - NOT USED Review of attachment 9092483 [details] [diff] [review]: ----------------------------------------------------------------- I think it's fine to implement back the `ontextentered` attribute, but I'm not sure if it's fine to leave the "Enter" event handled also by the `onkeypress` for the address book. Wouldn't this cause a double trigger? Bouncing the feedback request to Paul
Comment 42•5 years ago
|
||
Am I doing something wrong?
Yes, I think so. You're hitting enter after the click onto a suggestion without noticing that that shouldn't be necessary.
I'm not sure if it's fine to leave the "Enter" event handled also by the
onkeypress
for the address book. Wouldn't this cause a double trigger?
That's what I thought, but it works ;-)
Assignee | ||
Comment 43•5 years ago
|
||
Comment on attachment 9092483 [details] [diff] [review] 1575046-follow-up.patch - NOT USED Review of attachment 9092483 [details] [diff] [review]: ----------------------------------------------------------------- I'd have the same question about the doubled handling of the return key, but if it works and since this is just a fix for 68, why not.
Comment 44•5 years ago
|
||
I can't build TB 68.1.1 right now due to bug 1580940. Let's see whether Alex has a better idea in bug 1580950.
Comment 45•5 years ago
|
||
Note to self: Remove excess space at line 781.
Comment 46•5 years ago
|
||
Comment on attachment 9092483 [details] [diff] [review] 1575046-follow-up.patch - NOT USED Not used. I decided to go with the fix from bug 1580950.
Description
•