There are focus issues which impede/confuse the workflow when deleting multiple recipients, depending on direction. Essentially, it only works somewhat "correctly" when using backspace to delete "backwards" (going leftwards/upwards to the first recipient).

Note: We have folded bug 1530254 (navigate recipients with cursor down/up) into this bug 1527547 because they complement each other well and are in the same corner of the code. We've also added auto-select of recipients on focus to create a hassle-free, efficient UX both mouse and keyboard. This bug provides a precursory "pill-style" experience in the current release version as of TB 68, even before the advent of real recipient pills in bug 440377.

STR

1. Compose message with three recipients: R1, R2, R3. Ensure this scenario before going into any of the alternative steps below.
2. 1. Click recipient delete button on R2 and don't move your pointer after that.
   2. Place cursor in front of R2 and use DEL button on (windows) keyboard to delete "forward" (to the right of cursor).
   3. For comparison: Place cursor after R2 and use BACKSPACE button on (windows) keyboard to delete "backward" (to the left of cursor).

Actual result

2. 1. Mouse pointer is over the delete button of recipient R3, but keyboard focus has been moved to the end of R1, which causes R1 to be highlighted, which is unexpected, irritating and useless because clicking again will delete R3, not R1. Iow, using the recipient delete button will delete "forwards" up to the last recipient, but focus jumps "backwards" each time. This will certainly also cause havoc to screen readers.

   2. Using DEL, having deleted R2 rightwards, focus suddenly jumps leftwards to the end of R1 and gets stuck there. This is unexpected, useless, and inconsistent because when using Backspace (leftwards deletion), we allow holding the key to delete multiple recipients, so why is it different here?

   3. Using BACKSPACE, having deleted R2 leftwards, focus correctly shifts leftwards to the end of R1 and user can continue deleting leftwards, which is predictable and expected as far as direction of deleting is concerned (although it's also somewhat error-prone if you hold a little too long).

Expected result

2. 1. Using recipient delete button is a "forward"/rightwards action, so the next recipient (R3) which is going to be deleted with the next click in same position should also be focused and highlighted. Not sure about the exact cursor position, at the end of R3 would feel right to me (alternatively, at the start of R3, or R3 highlighted).

   2. Using DEL button is a a "forward"/rightwards action, so focus should seamlessly shift forwards/rightwards to the start of the next recipient (R3) and allow deleting that (reverse case of 2.iii.)).

This fixes comment 0 nicely, and another bug where placing the cursor e.g. in the middle of an input, then deleting a neighbor forwards/backwards will pointlessly remember the last cursor position/selection when that other field gets focus again.

I reduced the sibling.sibling thing by using parent.parent, which is safer, and I moved that query-selector into the .js code, hence radically simplifying the onclick attribute in XUL, which gets duplicated for every recipient so it should be as simple as possible for better performance.

One known glitch: When deleting with mouse causes scrollbar to disappear, recipients shift under the mouse so that the previous recipient will be hovered, but the next recipient is focused. It does work perfectly when header is big enough to avoid scrollbar, then the next item to be deleted by click will be under the mouse and focused.

I provided for deleteFlavor "deleteClick" (currently unused) so that it'll be easier to fix the remaining glitch as we won't have to break up the logic again.

Jörg, how can we ensure that this gets the missing review timely to avoid bitrot? I think the code is pretty straightforward, nothing obscure here

Ping him personally?

Let's get this right, right here. This fixes and improves the overall UX of navigating, selecting, and deleting recipients.
Now with auto-select, cursor down/up navigation (Bug 1530254), convenient and concise deletion of recipients via keyboard, and error-prevention for inline deletion.

Aceman, could you please review this soonish?
Richard, requesting ui-r because of some new tweaks here.

Try this after applying the patch:

1. Add at least 3 recipients:
   John <john@asdf.com>
   Recipient 2 <second@foo.com>
   Jane <jane@bar.com>
   (Cursor is now in the empty To: field after Jane.)
2. Cursor up (or left-click) to select "Recipient 2". Wow, that was easy!
3. Press DEL (or BACKSPACE) twice to delete "Recipient 2" and the empty row, and continue hitting the same key intermittently (tap-tap, tap-tap) to delete more recipients (or hold to delete them all).
4. If for whatever reason you prefer to delete "Jane <...>" character by character, place cursor at start or end position and hold a deletion key until she's gone: we'll automatically stop you there for a second. Press a deletion key again to delete the empty row.
5. Never try this in the current release version...

Fixes and improvements in detail:

• ux-efficiency, ux-control, ux-consistency: Cursor down/up are now enabled to navigate the list of recipients vertically, with convenient auto-selection (see below). Forget about clumsy and flimsy double-TAB, and enjoy the new feeling of control with ease (Bug 1530254). Of course we don't interfere with your autocomplete navigation.
• ux-efficiency: Recipients are now auto-selected no matter how you touch them (keyboard/mouse). It's much more likely that user wants to act on the complete address |John Doe <john@asdf.com>| than to change something inside the address.
• ux-consistency: When using DEL key to delete recipient rows, we're now observing direction (forwards/downwards first).
• ux-error-prevention: When holding DEL or BACKSPACE to delete letterwise within a single recipient text (inline deletion), we no longer eat into the next or previous recipient, because it's nonsense and error-prone to delete more than 1 recipient letter by letter. But when you start deleting on an empty row (i.e. let go of the keys after inline deletion), we'll still do row deletion as expected, in the correct direction.
• replaced deprecated event.keyCode notation with simpler and more transparent event.key
• removed awKeydown() function and which could never occur because we catch the only two keys of that function here in awRecipientKeyDown(...) and explicitly prevent awKeydown via event.stopPropagation (see comment in old code). Maybe we'd want to check if this was used by Addons, but it's unlikely and they definitely shouldn't.

(In reply to Thomas D. (currently busy elsewhere) from comment #5)

3. Press DEL (or BACKSPACE) twice to delete "Recipient 2" and the empty row, and continue pressing the same key intermittently (tap-tap, tap-tap) to delete more recipients (or hold to delete them all).

Sorry, if I have not understood how it is functioning correctly.

Is there an Undo function for the deletion? Otherwise, holding the DEL key to delete multiple recipients seem prone to unwanted deletion. Maybe stick to tab-tab?

(In reply to xracoonx from comment #6)

(In reply to Thomas D. (currently busy elsewhere) from comment #5)

3. Press DEL (or BACKSPACE) twice to delete "Recipient 2" and the empty row, and continue pressing the same key intermittently (tap-tap, tap-tap) to delete more recipients (or hold to delete them all).

Sorry, if I have not understood how it is functioning correctly.

Have you tried it??

Is there an Undo function for the deletion?

No Undo (before or after this patch); out of scope for this bug.

Otherwise, holding the DEL key to delete multiple recipients seem prone to unwanted deletion.

Using DEL on a recipient key WAS prone to unwanted deletion of other recipients before my patch, I actually made it safer: When you delete a recipient character by character (inline deletion), even holding down your favorite deletion key, TB will now stop when you have deleted that recipient. But starting from an already selected recipient, or from an empty row, you can now also do controlled deletion of several recipients going forward, press DEL key twice for each recipient: First keypress deletes the entire recipient, second keypress deletes the blank row and selects the next recipient. If you deliberately keep holding the DEL key, it'll be "fast-track deletion of multiple recipients", which is no different from holding BACKSPACE in the current version. Apart from direction, there's no difference between DEL and BACKSPACE, so they should behave the same way.

Maybe stick to tab-tab?

Definitely not, but you can still use tab-tab for navigation if you so wish.

(In reply to Thomas D. (currently busy elsewhere) from comment #7)

(In reply to xracoonx from comment #6)

Sorry, if I have not understood how it is functioning correctly.

Have you tried it??

No. But I would. Is a binary available?

(In reply to xracoonx from comment #9)

Have you tried it??
No. But I would. Is a binary available?

Not yet. We can ask for a try build maybe after getting some more feedback/reviews.
Otherwise let's try to stay focused here.

Some nits fixed, better comments, and a new focus function.
Richard, behaviour hasn't changed over the last patch.

I suggest to rip out the existing focus function because it adds a lot of unnecessary complexity for nothing. Two parameters for focusing one row. Bloated inside. For maximum confusion, the caller in the delete function even (sometimes!) required to use two different row inputs to ultimately focus/ensureVisible the same row. And the ensureIndexIsVisible in the old focus function doesn't change anything, probably inbuilt now. I added some console.logs and TODOs to illustrate these problems.

(In reply to Thomas D. (currently busy elsewhere) from comment #11)

I suggest to rip out the existing focus function because it adds a lot of unnecessary complexity for nothing. Two parameters for focusing one row. Bloated inside. For maximum confusion, the caller in the delete function even (sometimes!) required to use two different row inputs to ultimately focus/ensureVisible the same row. And the ensureIndexIsVisible in the old focus function doesn't change anything, probably inbuilt now. I added some console.logs and TODOs to illustrate these problems.

Here's my proposal for a new focus function which can accept either a row number OR the input textbox element as an argument. So callers can pass in whatever is more easy for them. Row number is handy for many cases, and less heavy than passing in the entire element. However sometimes, if rows get added or removed (as in awDeleteHit()), it might be difficult and/or less performant to determine the row number to be focused in advance. So in that case they can just pass the input element itself without bothering to figure out the row number which it will have after the shuffle.

function awSetFocus(rowOrInput) {
  setTimeout(_awSetFocus, 0, rowOrInput);
}

function _awSetFocus(rowOrInput) {
  if (typeof rowOrInput === "number")
    awGetInputElement(row).focus();
  else
    rowOrInput.focus();
}

(In reply to Thomas D. (currently busy elsewhere) from comment #12)

Here's my proposal for a new focus function which can accept either a row
number OR the input textbox element as an argument.

Let's not. I think allowing mixed parameter types is bad practise in general. There are cases where it's useful, but this isn't one of them.

(In reply to Magnus Melin [:mkmelin] from comment #14)

(In reply to Thomas D. (currently busy elsewhere) from comment #12)

Here's my proposal for a new focus function which can accept either a row
number OR the input textbox element as an argument.

Let's not. I think allowing mixed parameter types is bad practise in general. There are cases where it's useful, but this isn't one of them.

Thanks, that's right. I'll do the focus stuff cleanup in bug 1528840 (see current patch attachment 9047092 [details] [diff] [review]).

Update as we've landed the new, simplified focus function in bug 1528840 (attachment 9047329 [details] [diff] [review] / https://hg.mozilla.org/comm-central/rev/b4b9a40aa811).

Aceman, pls double-check. If we're good, pls do try-run. I want to land this first before venturing into more cleanup exercises in bug 1528840. Tia.

• This patch addresses all of Aceman's review in Comment 16. I'll add some comments wrt minor deviations.
• I fixed another nit where pressing Alt+Backspace to undo inline-deleting a recipient text would wrongly delete the row.
• Comments added and improved
• Various nitfixes
• FTR: This patch removes awKeyDown() after thorough testing: Normally it does nothing as it gets explicitly bypassed in awRecipientKeyDown(); what it did otherwise (delete "random" recipients when pressing DEL/BACKSPACE with focus ring on recipient type picker) was undiscoverable, unpredictable, and bugged.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d5909d56bcf3d4a4ebcebf1302034817764266af

Looks like one test failure: test-send-button.js::test_send_enabled_manual_address

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

New patch with test update courtesy of Aceman (thanks a lot!), and double quotes nitfix.

Aceman, Richard, no changes, but please add your review flags again.

Ready for landing :-)

Has r=Aceman from comment 19, and ui-r=Paenglab from comment 13 (please allow them to set their flags again on the patch).

Try run is green wrt our patch (fails only in unrelated test testTaskView.js).

(In reply to :aceman from comment #22)

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

(In reply to Richard Marti (:Paenglab) from comment #25)

Comment on attachment 9048869 [details] [diff] [review]
When no change, then you don't need to ask and you can set flags yourself.
Not tested as you wrote there was no change.

I know, but I don't like setting other people's flags, and especially on the final patch, imo it's more informative and looks better to see the actual reviewers at a glance. Thanks hey!

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/392d6ce8f05d
Fix and improve UX of navigating, selecting and deleting recipients via keyboard. ui-r=Paenglab, r=aceman

There is a length restriction of the commit message of about 40cm on my screen. You can capture your memoires below the first line ;-)

(In reply to Pulsebot from comment #27)

Pushed by mozilla@jorgk.com:

Thanks, that was fast!

At least a small test of the added feature, at the spot in the test that detected it :)

Hmm, failed on Mac here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=af8e5451b93e732765f7a1346944992a61feccae&selectedJob=232880745

I ran it locally on Windows before landing and I'm sure you ran it on Linux.

Yes, OS X must behave differently again :)
The click into recipient in the test is ignored, because cursor is already there. So let's re-focus it manually.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=27e4d4eb3fee5bc0811ebb04fa9a405a8930e74a

https://hg.mozilla.org/comm-central/rev/e915e56ae4360435030adfa96b8c9eb9758b783f
fix test-send-button.js on Mac. r=jorgk

Note that this bug has provided a precursory "pill-style" experience for the current release version as of TB 68, even before the advent of "real" recipient pills in bug 440377.