Closed Bug 1575046 Opened 5 months ago Closed 5 months ago

Support pasting lists of addresses into mailing list dialogs

Categories

(Thunderbird :: Address Book, enhancement)

enhancement
Not set

Tracking

(thunderbird_esr68+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
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+
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.

Summary: Support pasting lists of addresses into mailing lists → Support pasting lists of addresses into mailing list dialogs

You encouraged a "boy scout" fix for this in bug 1569492. Is it really easy, then by all means, let's do it.

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.

Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #9087161 - Flags: review?(mkmelin+mozilla)
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
Attachment #9087161 - Flags: review?(mkmelin+mozilla)

Thanks for the review. This addresses the comments and also improves a few things.

Attachment #9087161 - Attachment is obsolete: true
Attachment #9088173 - Flags: review?(mkmelin+mozilla)
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)
Attachment #9088173 - Flags: review?(mkmelin+mozilla) → review+

Thanks for review. This addresses the comments except for handling tab key, which is done in a new part2 patch.

Attachment #9088173 - Attachment is obsolete: true

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.

Attachment #9088436 - Flags: review?(mkmelin+mozilla)
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
Attachment #9088436 - Flags: review?(mkmelin+mozilla)

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

  1. a
  2. a, b, c
  3. d

becomes:

  1. a
  2. a
  3. b
  4. c
  5. 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.

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

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.

Did you use an existing list?

Need something to land, so I'm taking part 1.

Keywords: leave-open
Target Milestone: --- → Thunderbird 70.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a017484882ab
Improve adding lists of addresses into mailing list dialogs. r=mkmelin

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.

Attachment #9088436 - Attachment is obsolete: true

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.

Attachment #9091067 - Flags: review?(mkmelin+mozilla)

3 of 3: Just rebased and renamed. Now works due to fix in part2.

Attachment #9090053 - Attachment is obsolete: true
Attachment #9091068 - Flags: review?(mkmelin+mozilla)
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
Attachment #9091067 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #9091068 - Flags: review?(mkmelin+mozilla) → review+

Made that small edit to the doc strings. Thanks for pointing it out.

Attachment #9091068 - Attachment is obsolete: true
Keywords: checkin-needed

(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 on attachment 9091155 [details] [diff] [review]
part3-tab-key-for-address-lists-3.patch

Undesired hunk slipped in. Let's use the original.
Attachment #9091155 - Attachment is obsolete: true
Attachment #9091068 - Attachment is obsolete: false

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 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.
Attachment #9091067 - Flags: approval-comm-esr68?
Attachment #9091067 - Flags: approval-comm-beta+

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Last two parts landed on TB 71.

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

Attachment #9091067 - Flags: approval-comm-esr68? → approval-comm-esr68+

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.

Flags: needinfo?(paul)

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

Flags: needinfo?(paul)

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

Attachment #9092383 - Flags: review?(alessandro)
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 ;-)
Attachment #9092383 - Flags: review?(alessandro) → review+

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 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.
Attachment #9092483 - Flags: feedback?(alessandro)

My build actually works. So let's hear what the new auto-complete experts say ;-)

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 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
Attachment #9092483 - Flags: feedback?(alessandro) → feedback?(paul)

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 ;-)

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.
Attachment #9092483 - Flags: feedback?(paul) → feedback+

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.

Note to self: Remove excess space at line 781.

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.
Attachment #9092483 - Attachment description: 1575046-follow-up.patch → 1575046-follow-up.patch - NOT USED
You need to log in before you can comment on or make changes to this bug.