I am looking at a recent iteration of bug 440377: 73.0a1 (2019-12-11) (64-bit).*
Build Id: 20191211185751

STR (reproducible, reduced 2019-12-12)

1. create at least one pill in To-field (not reproducible without): (to-foo)
2. create another pill in CC-field: (cc-bar)

Actual result

• focus (cursor) jumps back to the end of TO field (but we just created a pill in CC field)

Expected result

• focus (cursor) remains in CC field

*: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e8fc86894b3288d4360b195fe7a952aec10302b9

I also had some red and incomplete/erroneous recipients in there

Now reproducible for me (updated STR on the bug). I tried to update to the latest trybuild from bug 440377, comment 200, but it's still showing 2019-12-10 (maybe international time differences?). How can I positively verify if the TB version running is matching a certain treeherder try build? I couldn't find the build ID on the treeherder page. Is it there?

Aceman, does this reproduce for you?

Note: While testing for this bug, I also succeeded several times to get into the following runtime condition (possibly involving shortage of available RAM):

STR (different issue)
(not reproducible)

1. type John -> autocomplete John Doe <john@asdf.com>
2. type "Jane" very fast (autocomplete too slow to trigger), press Enter (where Jane is known in AB as "Jane Doe <Jane@asdf.com>")

Actual

• Jane does not autocomplete NOR convert into a pill, stubbornly remains plain text.
• all but impossible to get another autocompletion for Jane or even in that field - pretty stuck.

Expected

• autocomplete and convert Jane to pill

Still reproduces on the latest try run. STR updated/reduced. Reproduces either way, you can also start with one pill in CC and then after adding the first pill in TO, focus will jump back to CC.

My gut feeling keeps telling me that this bug really sucks (also for testing purposes), because it disturbs even the most simple scenarios of adding more than one CC recipient. Any chance that this could get some priority attention?

I'll work on this.

You were right Thomas, this was a pretty nasty issue related tot he underlying architecture of creating a pill.

Long story short, the originalInput used to check which input field the pill was originated from wasn't properly updated and it remained constant to the first pill created. So if you create a pill on CC, the focus was always going back to CC.

I removed that attribute, and I'm using a method to walk up the DOM inside the CE and always get the correct input field.

Here's a try run to see if something broke: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f6738f575fe2164cbd9c8216d70bafe936e9e184

Would it be possible to retain this property (e.g. as rowInput)

I prefer to have method like focusOnRowInput that finds the proper field without relying on stored objects or IDs.
This is gonna be helpful once the drag and drop for pills is implemented, since we won't have to worry about updating that attribute when we move pills around.

Can this ever happen? A pill without address-container?

It's very unlikely, but I experienced it once that's why I put it in.
I can probably remove it since the issue I experience was most likely my fault.

Skip focused pill which is part of selection for deletion? Looks wrong to me.

I need to keep the currently focused pill to move the focus on the input field before removing it. If you notice there's a pill.remove() after the for loop.
I see bug 1602431 touches this, so depending on which lands first, we'll update things accordingly.

I'll wait for Magnus' review before updating the method name.

(In reply to Alessandro Castellani (:aleca) from comment #9)

Thanks for rapid reply :-)

Would it be possible to retain this property (e.g. as rowInput)
I prefer to have method like focusOnRowInput that finds the proper field without relying on stored objects or IDs.

Yes, that's why I proposed to make pill.rowInput a dynamic property without stored objects or IDs.
You can still have your method if you think pill.focusRowInput() is better than pill.rowInput.focus(). I think I like the latter slightly better; looks a bit more natural and generic to me.

This is gonna be helpful once the drag and drop for pills is implemented, since we won't have to worry about updating that attribute when we move pills around.

If the property updates itself whenever requested, there won't be any worries regardless of moving pills around. Otoh, as I said, I believe that having the property is useful, and not having it can cause less elegant code, for focus (as in bug 1602431) and other purposes.
What about this?

class MailAddressPill extends MozXULElement {
    // dynamic all-purpose property
    get rowInput() {
      return this.closest(".address-container")
                 .querySelector(`input[is="autocomplete-input"][recipienttype]`);
    }

    // convenience function
    focusRowInput() {
      this.rowInput.focus();
    }
}

Can this ever happen? A pill without address-container?

It's very unlikely, but I experienced it once that's why I put it in.
I can probably remove it since the issue I experience was most likely my fault.

We try to avoid such safety checks for things which must never happen. It's extra code not doing anything. Better to see the error and fix it.

Skip focused pill which is part of selection for deletion? Looks wrong to me.
I need to keep the currently focused pill to move the focus on the input field before removing it. If you notice there's a pill.remove() after the for loop.

Oh, now I get the idea. Unfortunately, this doesn't look right to me.

• I am not aware of any technical reason why we couldn't remove the focused element from the dom tree? I don't see any error in console nor any other disadvantage, as long as we restore focus afterwards. There's even blur() method for elements which actively removes focus into nowhere.
• The conditional is inside the selection loop, number of selected items potentially unlimited, so the micro-performance cost of this should be avoided if it's not absolutely needed, apart from adding unnecessary code complexity.
• Even if you actually wanted to move focus before deleting the focused pill, what's stopping you from moving focus to the rowInput before iterating items for deletion?
• pill.remove without checking if it's selected is still wrong anyway, for all cases of what I call "satellite focus" on a pill outside the selection.

Finally, since I am rewriting this entire removePills() function anyway in bug 1602431, your changes won't last... ;-)

I see bug 1602431 touches this, so depending on which lands first, we'll update things accordingly.

Yes. Much appreciated. I am happy about our cooperation in correcting and improving the pills experience. :-)

Some clean up and improvements based on Thomas' comments.

(In reply to Thomas D. from comment #10)

Skip focused pill which is part of selection for deletion? Looks wrong to me.
I need to keep the currently focused pill to move the focus on the input field before removing it. If you notice there's a pill.remove() after the for loop.

Oh, now I get the idea.

Or so I thought. It was there for a flash second, but didn't make it into my comment 10. Nevertheless, the comment still stands. Here's to clarify (sorry for the length, I'm also still learning from my reviews, so I'm trying to wrap my head around the coding details and their pros and cons):

The old patch attachment 9121927 [details] [diff] [review] wanted to skip the focused pill from deletion (by checking a potentially unlimited number of selected pills if they happen to be the focus pill), so as to use the pill later for pill.focusRowInput, and only after that, delete the focus pill which was explicitly spared from deletion before (which is quite confusing already...).

• I have mentioned some of the disadvantages/errors of doing that in comment 10.
• I'd agree that running methods from a removed pill is undesired and fragile (although it can actually work, as it did for my previous patch attachment 9122500 [details] [diff] [review] in bug 1602431, because JS passes the pill into the function by value, so removing the DOM pill doesn't kill the parameter pill).

Here's another twist corroborating my gut feeling why using generic pill.rowInput.focus() may be better than having a proprietary method pill.focusRowInput():
I am assuming that having a dynamic (self-updating) property pill.rowInput is possible (see my sample code in comment 10).
So with that, it's easy to avoid the costly and odd "skip-focus-pill" check inside the selected pills iteration:

// Store the focus target from self-updating pill property (internally using .closest(...)).
let rowInput = pill.rowInput; 
// Delete selected pills, potentially including pill (the focus pill)
for (let item of this.getAllSelectedPills()) {
  item.remove()
}
// Use generic focus() method on the stored focus target.
rowInput.focus()

Sweet and simple. Also for my patch in bug 1602431, which I have already updated to avoid using the dead pill.

Bottom line: having a dynamic property pill.rowInput is much more flexible than not having it, because once you get hold of the element, you can do anything with it at any later time, even after the pill is already gone. Otoh, for a method like pill.focusRowInput(), you always require the pill to still exist, although the method itself has nothing much to do with that pill, and you can't do anything else with rowInput because it's not accessible. Also in terms of modularity, I find it a bit surprising that a pill should have it's own proprietary method to act on some other element when the generic method of that element doesn't make it any harder to use.

Midair-collision, but I'll just post this comment anyway, haven't checked the new stuff above yet, forgive me if there's anything odd.

Fixed!

(In reply to Thomas D. from comment #16)

No, we cannot delete a focused pill which isn't part of the selection.

I understand that you want to delete focused but not selected pills because navigation keys currently only move focus without selecting, which isn't helpful, either. I have fixed the respective focus and selection issues in bug 1602431.

I won't change it because that's an overlapping of bugs and patches.
You have a patch taking care of this in bug 1602431, so I shouldn't touch my patch to implement portions of paradigms decided in your patch.

Also, the patch was reviewed and approved and the behaviour stays consistent with what we currently have.
You're fixing it in your patch, great, but let's not overlap objectives here.

Unused method removed and try-run completed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aa5fcd23a85ad06f787b1357aab57649a1c22028

Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/75ad64af9fe2
Fix focus ring moving to the wrong addressing input field after a new pill is created. r=mkmelin

(In reply to Alessandro Castellani (:aleca) from comment #18)

I won't change it because that's an overlapping of bugs and patches. [snip]
You're fixing it in your patch, great, but let's not overlap objectives here.

Fair enough, no problem, sorry. I think the part that was itching me here is the explicit comment which you added, and which could be understood to justify and manifest the incorrect status quo:

// Manually remove the pill in case it wasn't part of the selected array.

Otherwise I'm happy that we have fixed this bug nicely... :-)

Thunderbird 73.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/a58b5ec61207844b3f2728c87b9d0896990de3d0

Perhaps a different bug - I'm on beta and so don't whether this patch helps my issue ...
My issue doesn't require CC as a step, but maybe it makes it easier to reproduce?

1. complete an address in To
2. complete an address in Cc
3. (cursor is now in To) type a single letter such that autocomplete doesn't kick in

Result: tab and enter keys have no effect

If #3 doesn't reproduce, backspace over what you typed in To, and try again a couple times until autocomplete doesn't give choices

I'm not able to reproduce this, sorry.
Which beta are you using?

My cursor stays on the proper input field, and the autocomplete kicks in even with a single letter.
Richard, are you experiencing any of these issues?

(In reply to Wayne Mery (:wsmwk) from comment #23)

Result: tab and enter keys have no effect

Sounds similar to my comment 3.

(In reply to Thomas D. from comment #25)

(In reply to Wayne Mery (:wsmwk) from comment #23)

Result: tab and enter keys have no effect

Sounds similar to my comment 3.

It does indeed. Are you able to reproduce it with the fix from this bug?

My cursor stays on the proper input field, and the autocomplete kicks in even with a single letter.

I'm using 73.0b1. Happens about 50% of the time - sometimes takes multiple attempts, with backspaces

I tried it on Daily and Beta and can't reproduce it. Wayne, do you have an extension that could interfere?

(In reply to Wayne Mery (:wsmwk) from comment #26)

I'm using 73.0b1. Happens about 50% of the time - sometimes takes multiple attempts, with backspaces

Ah, this fix landed on 73.0b2

(In reply to Wayne Mery (:wsmwk) from comment #26)

It does indeed. Are you able to reproduce it with the fix from this bug?

I haven't seen the problem of comment 3 recently. Autocompleting after first character isn't a problem for me.
I like the current Dailies because they come with all those nifty improvements... :-)

I tried a dozen different ways and was not able to reproduce with 73.0b2