I am looking at the latest available iteration of bug 440377 (try build 73.0a1 (2019-12-06) (64-bit).*

This regresses bug 1527547, where I have just made this work correctly and predictably, pill-style.

STR1 (Backspace)

1. add 4 recipients: (pill1) (pill2) (pill3) (pill4)
2. focus and select pill3
3. press backspace 3 times, expecting to delete pill3, pill2, pill1 backwards

Actual result

• 1st backspace deletes pill2 (ok), but cursor jumps away, now after pill 4 (wrong)
• 2nd backspace focuses and selects pill4 (wrong: should delete pill2)
• 3rd backspace deletes pill4 (wrong: should delete pill1)
• This is unexpected and error-prone, because focus and the expected direction of backspace-deleting are ignored.

Expected result:

• Should delete pill3, pill2, and pill1 (in that order, and no other unrelated pills)
• i.e. after each backspace-deletion, should focus and select the previous pill (not sure about RTL languages)

STR2 (DEL)

1. add 4 recipients: (pill1) (pill2) (pill3) (pill4)
2. focus and select pill2
3. press DEL 3 times, expecting to delete pill2, pill3, and pill4

Actual result

• 1st DEL deletes pill2 (ok), but cursor jumps away, now after pill 4 (wrong)
• 2nd and subsequent DEL key presses do nothing, stuck after pill4 (wrong)
• This is disfunctional, because deleting in the expected direction fails.

Expected result

• Should delete pill2, pill3, and pill 4 (in that order)
• i.e. after each DEL-deletion, should focus and select the next pill

STR (NN)

• dozens of variations and edge cases depending on the particulars of selection and focus, see comment 5. When focused pills get deleted, we must ensure that focus never gets lost and is smartly repositioned.

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

(please set version to "trunk" for issues that don't affect releases)

Holding backspace or DEL to delete multiple recipients faster also fails as it gets stuck at the cursor after last pill, after deleting only one pill. We only want to brake when changing from characterwise deletion into pill-wise deletion.

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

(please set version to "trunk" for issues that don't affect releases)

ok

Bug 1603563 is mostly a duplicate of this, with the extra aspect of mouse deletion using (x) on pills.

Current patch attachment 9115847 [details] [diff] [review] in bug 1603563 isn't quite complete...
This patch here isn't either, but I'm hoping to have covered the following:

Removing recipient pills with DEL or BACKSPACE

• all cases and edge cases I could think of
• deleting single and multiple selections
  • enable fast-track, single-key-press consecutive deletions of single pills
  • focus-only after deleting multiple selections of pills
• deleting contiguous and non-contiguous selections
• deleting selections where focus is on unselected pill (using Ctrl), combined with any of the above (always keep existing user-focus on unselected pill; cf. Windows Explorer)
• deleting first and last pill (which in multiple selections can prevent focusing the expected sibling, but we still have to put focus somewhere)
• deleting focused pill - smart focus determination
• starting keyboard deletion in either direction on any focused, but unselected pill (now helpfully selects)
• unmodified navigation with cursor (now selects target pill, focus-only is not practical)
• and the ultimate unthinkable edge case, cross-recipient field multiple non-contiguous selections (yes, you can actually Ctrl+mouse-select random recipients from To, CC, BCC etc. at the same time, and delete them all at once using keyboard, and we still want to get the focus right)...

So this patch overrides the mailwidget.js part of patch attachment 9115847 [details] [diff] [review].

Disclaimer: I have not fixed deletion via any other methods (context menu, Ctrl+X, main menu, mouse-click on pill's (x)). I hacked the latter to at least stop deleting selections, which was error-prone and highly confusing (like when you click the (x) on an unselected pill and all selected pills go away - no, we can't do that!)

Please try the above scenarios and let me know what you think.

I'm sure there must be neater ways of writing this patch, but be aware that moving things around will probably break some edge case behaviour...

Last, but not least, don't forget to try...

• ...the ultimate unthinkable edge case: cross-recipient-field multiple non-contiguous selections (yes, you can actually Ctrl+mouse-select random recipients from To, CC, BCC etc. at the same time, and delete them all at once using keyboard, and we still want to get the focus right)...

Let's fix this properly wrt focus and selection in various scenarios.

• Completely revised code (esp. after bug 1601740).
• Extended to cover the little remainder of bug 1603563, which needs the same algorithm, so it just had to be wired up. Now covers all currently available methods for removing pills:
  • BACKSPACE, DEL
  • Delete (pill context)
  • Cut (pill context, or Ctrl+X)

Features:

• Correct, safe and intuitive selection and deletion with BACKSPACE, DEL (try it!)
  • auto-stop for rapid repeated deletions observing direction (e.g. select pill 3 of 5, hold Backspace -> deletion stops with focus on pill 4)
• Smart focus and selection handling when removing recipients - no more loss of focus.
  • Preserve "satellite focus" on unselected pill (select pill 4 out of 5, Ctrl+cursor left (x2) to focus pill 2 -> focus remains on pill 2)
  • Refrain from deleting focused pills which are not selected ("satellite focus")
  • Otherwise, keep focus as close as possible to the previously focused pill, observing direction (first select pill 2 out of 5, then Ctrl+select pill 4, press Backspace -> focus on pill 3)
• Handle all known scenarios and edge cases (kindly let me know if I missed something):
  • deleting first or last pill (observing direction)
  • deleting focused pill (smart focus move)
  • deleting multi-selection (DEL / BACKSPACE: no subsequent auto-selection for better ux-feedback)
  • contiguous and non-contiguous selections, also with "satellite focus" (e.g., you can select pill 1 and 5 out of 6, and have focus on pill 3)
  • cross-recpient-fields selections (some pills in To AND some pills in CC selected)
• Unmodified navigation (cursor left/right) now conveniently selects the target pill to allow immediate action (edit, delete, cut, copy); cf. Windows explorer behaviour

Note: There are some unrelated, pre-existing bugs / missing features:

• Bug 1602397: E.g. some Shift-selections (mouse or keyboard) fail
• Main menu actions not hooked up (cut/copy/paste/delete...); I had a brief look and it seems non-trivial

I also fixed some minor glitches which had escaped review, e.g. JavaDoc's @return (without s please), and a wrong description on handleKeyPress() of MailRecipientsArea.

I can't seem to be able to import your patch:

abort: decoding near 'Thomas Dllmann <b': 'utf8' codec can't decode byte 0xfc in position 8: invalid start byte!

Just save the attachment "Save Link As ..." and add it to the series file manually, or whatever import you use. Change the ü to ue, we agreed on that previously. The file is most likely in windows-1252 not UTF-8.

Is this taking care also of bug 1603728? Can we close that as a duplicate and keep everything related to delete, backspace, and focus moving into once bug?

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

Review of attachment 9121409 [details] [diff] [review]:

Thank you! :-)

Also, and I don't know if it's a local issue, I can't load your patch due to
the special character in the changeset header
# User Thomas Düllmann <bugzilla2007[snip]>

Oh yes, sorry, old settings which somehow resurfaced.

> ::: mail/base/content/mailWidgets.js
> @@ +2057,5 @@
> > +          }
> > +        }
> > +      }
> > +      return null;
> > +    }

This is a bit verbose and can be simplified by removing the else
statements.

Awesome, thanks. I'll post a new patch.

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

Comment on attachment 9121409 [details] [diff] [review]
# User Thomas Düllmann <bugzilla2007**[snip]**>

Alessandro, kindly edit/truncate my email address in your bug 1602431 comment 14 to prevent spam. Thanks.

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

Is this taking care also of bug 1603728?

Yes, with the benefit of hindsight, that has been an easy collateral fix here.

Can we close that as a duplicate and keep everything related to delete, backspace, and focus moving into once bug?

Done, thanks.

Address Alessandro's review, comment 14.

Bug 1603166 currently removes originalInput, which affects this bug.

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

Review of attachment 9122277 [details] [diff] [review]:

Working pretty good.

Thank you :-))

I think for the case where you delete the last pill
using delete (del forwards), you should not select a pill, but that should
get you to the cursor.

Yes, the correct behaviour got lost in the last iteration because of the missing return null below.

• @param {("next"|'"previous")}[siblingsType="next"] - direction of
  iterating siblings
  .. is the way to document optional parameters and the default value

Thanks. Could you point me to the documentation for the JavaDocs Syntax as we use it?
I was actually looking for documentation, but the official one (1) doesn't seem to cover the details, and our style seems to be different.

there's a missing return null here

Oops! Got lost in the last iteration; this causes the wrong focus when deleting the last pill with DEL.

+ * @param {XULElement} pill the mail-address-pill element where Event fired

@param {Element} pill - The mail-address-pill element where Event fired.

There are different styles in our code, but most do not have the hyphen, and in official JavaDoc, it's spaces.
I also thought we had once agreed that if the explanatory phrase isn't a sentence, small initial and no period would be grammatically correct. Anyway, I'll just rework the comments as you wish.

+ hasSelectedPills() {
+ return Boolean(this.querySelector("mail-address-pill[selected]"));

shouldn't use Boolean like this, since the parameter is not a boolean

Objection, your honor. I'm using Boolean as a function, not as a constructor.
The Boolean() function evaluates any expression (incl. truthy/falsy) as a parameter and returns a primitive boolean.
Which is exactly what we want here:
Boolean (pill-element) == true
Boolean (null) == false

just use double not (!!) instead

I find Boolean(...) function much more readable and explicit than the double not !!(...), and I checked for best practices and they seem to agree:

http://speakingjs.com/es5/ch10.html

"... prefer Boolean(expression), because it is more descriptive"

https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Boolean

to perform this task [convert a non-boolean value to a boolean value], ... use Boolean as a function, or a double NOT operator:
var x = Boolean(expression); // use this...
var x = !!(expression); // ...or this

https://www.w3schools.com/js/js_booleans.asp

You can use the Boolean() function to find out if an expression (or a variable) is true

(1) https://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#tag

Fixed one nit and the JavaDoc comments per review of comment 24, see my comment 25.

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

Thanks. Could you point me to the documentation for the JavaDocs Syntax as we use it?

See https://jsdoc.app/tags-param.html - it's actually JSDoc, which is very similar to JavaDoc.

There are different styles in our code, but most do not have the hyphen, and in official JavaDoc, it's spaces.

The guidelines are not strict about it, but I'd think with hypen it's usually more readable.

1602431-removePills-focusSelect.patch (incl. fixes for bug 1603563, bug 1603728)

Corrected and improved some more comments.

Fixed one nit per Alessandro's bug 1603166 comment 9: Not advisable to remove focused pill if we still want to use pill properties/methods like pill.originalInput later. Although surprisingly it was working perfectly in spite of that flaw, argument passed by value so the pill argument doesn't go away, and even originalInput always got the correct thing from that dead value-pill (why?).

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

See https://jsdoc.app/tags-param.html - it's actually JSDoc, which is very similar to JavaDoc.

Thanks a lot for this helpful link. Can it also be found in TB development documentation?

Undo of pills on deletion produce wrong results.

Example :
5 address in field. Delete one address.
On undo, again all 5 address in field instead of deleted one address. So total address (4+5=9)
same repeat again. keep increasing no of address.

(In reply to Jaise from comment #30)

Undo of pills on deletion produce wrong results.

Hi Jaise, thank you for alerting us to the undo problem.

We're currently evaluating our options wrt undo. It's not easy technically, and it's not prioritized atm because we still have other, more relevant issues wrt pills. Fyi, it's currently not possible to undo pills, and it's unlikely to be implemented soon. Please note that what you're seeing is an automatical undo feature which is inbuilt with the text input box, so it's purely about any text which you ever entered before it became pills. I appreciate that the resulting user experience is poor, and sorry for that, but we can't change that right now.

This bug has a more narrow scope which does not include anything about undo afasics, although it may slightly "improve" your scenario because after this bug, when you delete with Backspace, focus will usually remain on pills, where there's no undo anyway. So please file your issue as a new bug with precise steps to reproduce (e.g. how exactly did you create your 5 recipients), actual result, and expected result. Tia.

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

Thank you for fast review! :-)

Review of attachment 9122757 [details] [diff] [review]:

Could you adjust your editor so the patches can be imported the normal way?
UnicodeDecodeError: 'utf8' codec can't decode byte 0xfc in position 36:
invalid start byte

I think you accidentally used a very old version of this patch (patch file name hasn't changed since): the last and only patch here with "Thomas Düllmann" as patch author is attachment 9121409 [details] [diff] [review] (patch version 2 out of 5), which had "ü" in position 36. Alessandro mentioned this in Comment 13 and I have fixed it immediately, so the current patch cannot be affected.

I wonder if it wouldn't be more simple and clearer to just have a different
method for backspace deletion?

Hmmm. I don't think so:
28 lines of code in the entire function (of which 5 lines are closing brackets only, so it's 23 lines with textual code).
2 lines only (of 28) specific to backspace deletion:

      if (!focusElement && focusType == "previous") {
        focusElement = addressContainer.querySelector("mail-address-pill");

26 lines hence shared code for all sorts of deletion methods (see commit message):
...removing recipient pills with DEL, BACKSPACE, context delete, or cut
Breaking up this common routine would create lots of duplicated code of similar complexity.

@@ +2422,5 @@

  */
+    removeSelectedPills(pill, focusType = "next", select = false) {
+      // We'll look hard for an appropriate element to focus after the removal.
+      let focusElement = null;
+      let addressContainer = pill.closest(".address-container");

just inline this in the one place you use it

Doesn't work: focus pill needed to get addressContainer, but where we need the addressContainer, focus pill may be deleted. So even though the dead parameter pill will survive, the DOM context is lost, and inlining as you suggest causes an error:

TypeError: can't access property "querySelector", pill.closest(...) is null mailWidgets.js:2453:29

@@ +2438,5 @@

       }
 
+      // Remove selected pills.
+      let selectedPills = this.getAllSelectedPills();
+      let selectedPillsCount = selectedPills.length;

you can just refer to selectedPills.length later and avoid this tmp.
the list is not live

Yeah, this works, length of the non-live array of selected pills which are already removed from DOM.

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

I think you accidentally used a very old version of this patch (patch file name hasn't changed since): the last and only patch here with "Thomas

Nope, it's something with the current one too. I don't know what.

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

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

I think you accidentally used a very old version of this patch (patch file name hasn't changed since): the last and only patch here with "Thomas

Nope, it's something with the current one too. I don't know what.

Current patch is attachment 9122757 [details] [diff] [review]. I downloaded that from here again and double-checked the Encoding with Notepad++: UTF-8 with Unix-LFs.
Your error says: UnicodeDecodeError: 'utf8' codec can't decode byte 0xfc in position 36
If that's position 36 of the entire patch, there's "u" (without umlauts), and Notepad++ isn't showing any other extraneous or mischievous character at that position.

Address nits of review comment 32.

Updated after bug 1603166.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/3568d9ea04a6
Fix and handle focus and selection when removing recipient pills with DEL, BACKSPACE, context delete, or cut. r=mkmelin, ui-r=aleca

This is causing a bunch of oranges, so will have to back it out.
TEST-UNEXPECTED-FAIL | comm/mail/test/browser/composition/browser_sendButton.js | false == true - JS frame :: chrome://mochitests/content/browser/comm/mail/test/browser/composition/browser_sendButton.js :: check_send_commands_state :: line 73

I think at least part of the problem is that clear_recipient doesn't do what it should now.

backout: https://hg.mozilla.org/comm-central/rev/a4e036ddf0334578d6330d8cd6e31e93a46de8fa

Thomas, are you able to run tests locally?
Was a try-run ever launched with this?

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

Thomas, are you able to run tests locally?

I have never been into tests, so I don't know the requirements. Do we have documentation on how to set up a testing environment or whatever it takes to run tests locally?

Was a try-run ever launched with this?

Unfortunately not. Good idea for next time. I've tested the new behaviour myself, but try-run would be useful for testing the tests. They are failing because of intentionally changed behaviour, I don't think we broke anything here.

Here's the doc on how to run tests: https://developer.thunderbird.net/testing/running-tests

Whenever we change the behaviour of a section covered by tests, we need to be sure the tests get updated to cover the new behaviour.
If you manage to fix those failures locally, upload a new patch and I'll take care of launching a try-run to test it on all OSs.

Based on Magnus comment 41, I've attempted an untested fix of the clear_recipient function, which may be sufficient to fix the tests (or maybe not).
The testfix patch is complementary to the main patch.

Alessandro, Aceman, may you pls have a look and do whatever you deem fit.

I have deliberately NOT used user accessible methods like "Backspace" or "Delete" to clear the recipients, for two reasons:

• this is just a helper method to clear out all recipients (as opposed to testing deletion), so it should be fast and efficient, and not depend on the inbuilt delicate complexities of deletion functionality which this bug implements.
• testing deletion (incl. focus/selection) after this bug needs it's own dedicated tests, which is another can of worms.

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

Here's the doc on how to run tests: https://developer.thunderbird.net/testing/running-tests

Thanks you for the link. I think I'll be happy to go into that if I get paid by the project... ;-)

Whenever we change the behaviour of a section covered by tests, we need to be sure the tests get updated to cover the new behaviour.
If you manage to fix those failures locally, upload a new patch and I'll take care of launching a try-run to test it on all OSs.

Thank you. If you think that this patch can fix it, you could do a tryrun with both patches applied.

One nitfix for my testfix attempt.

Caveat: Please See comment 46 for details.

This is fixed, at least locally.
The problem was that the pills created in the tests needed to be selected in order to be deleted, as that's what this patch introduces.
I also fixed some minor linting issues.

Here's a try run to see if I missed something else: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0650432613a07d8a3a97427e85169ea55ef6213e

You're missing the point of the test.

We're not only testing the removal of the pills, but we're also testing that the Send button in the compose windows gets disabled since we don't have any valid address to send the email to.

The tests were failing because the simple pill.remove() doesn't trigger the onRecipientsChanged() method which updates the Send button accordingly.
During tests we're creating 1 or 2 pills per addressing field, nothing more, and calling the removeSelectedPills() method is important for the following reasons:

• We run that method multiple times to be sure it doesn't fail or cause unexpected results.
• It's a complex method and we should run it during tests in order to have a feedback in case a future patch changes it and the test suddenly fail. Simply removing the pill wouldn't force the test to pass through that method, so we wouldn't have any coverage.
• Since the onRecipientsChanged() method is called at the end of the removeSelectedPills() method, we need to run it to be sure we reach that point without unexpected failures.

And also, you said it yourself here:

this function runs a dozen times (so if delete functionality would ever fail, this function would also fail a dozen times).

That's exactly why we have to include it in the tests.

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

The tests were failing because the simple pill.remove() doesn't trigger the onRecipientsChanged() method which updates the Send button accordingly.

Thank you very much for the detailed explanation Alessandro, that was the missing link on my side!

I'm surprised to see "checkin-needed-tb" because Magnus' review comment 50 requests a code change (adding 'true' parameter to pill.toggleAttribute("selected"); , which makes it a lot safer to use); my review comment 51 requests to fix a wrong comment in the test, and to rename one function to be more transparent about its purpose.
If you intend to fix those yourself during landing, please leave a note on the bug. Tia.

Ah man, so sorry about that and thanks for the heads up.
I completely misread the comments.
I'll fix everything and upload a new patch.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/cbfeac2dfd57
Fix and handle focus and selection when removing recipient pills with DEL, BACKSPACE, context delete, or cut. r=mkmelin, ui-r=aleca