Closed Bug 1602431 Opened 4 months ago Closed 2 months ago

Deleting pills with BACKSPACE or DEL key error-prone, disfunctional

Categories

(MailNews Core :: Composition, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 74.0

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

References

Details

(4 keywords)

Attachments

(1 file, 10 obsolete files)

16.75 KB, patch
aleca
: review+
aleca
: ui-review+
Details | Diff | Splinter Review

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)

Version: unspecified → Trunk

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.

See Also: → 1603563
See Also: → 1603728
Attached patch 1602431-removePills-focus.patch (obsolete) — Splinter Review

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.

Assignee: nobody → bugzilla2007
Attachment #9116267 - Flags: feedback?(richard.marti)
Attachment #9116267 - Flags: feedback?(alessandro)
Status: NEW → ASSIGNED

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)...
Comment on attachment 9116267 [details] [diff] [review]
1602431-removePills-focus.patch

Looks good. Not every edge case tested but what I tested looked logical.
Attachment #9116267 - Flags: feedback?(richard.marti) → feedback+
Comment on attachment 9116267 [details] [diff] [review]
1602431-removePills-focus.patch

Review of attachment 9116267 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this patch.
The approach looks good at first glance, but currently missing the handling of click events.
We're basically working on the same thing between this bug and bug 1603563, which covers the click events.

I'll grab your code and implement it in my patch to cover the edge cases I missed.
Let's continue on bug 1603563 to not duplicate our effort.
Attachment #9116267 - Flags: feedback?(alessandro) → feedback+
Comment on attachment 9116267 [details] [diff] [review]
1602431-removePills-focus.patch

Obsoleted by bug 1601740.
Attachment #9116267 - Attachment is obsolete: true

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.

Attachment #9121409 - Flags: ui-review?(alessandro)
Attachment #9121409 - Flags: review?(mkmelin+mozilla)
Duplicate of this bug: 1603563
Attachment #9121409 - Attachment is patch: true

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!
Comment on attachment 9121409 [details] [diff] [review]
1602431-removePills-focusSelect.patch (incl. full fix for bug 1603563)

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

From a UI point of view this works as expected, thanks for taking care of this.
I left a little comment for a portion of code, but of course let's wait for Magnus' review.

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]**>`

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

This is a bit verbose and can be simplified by removing the `else` statements.
```
if (siblingsType == "next") {
  // Check for next siblings.
  let element = this.nextElementSibling;
  while (element) {
    if (!element.hasAttribute("selected")) {
      return element
    }
    element = element.nextElementSibling;
  }
}
	
// Check for previous siblings.
let element = this.previousElementSibling;
while (element) {
  if (!element.hasAttribute("selected")) {
    return element
  }
  element = element.previousElementSibling;
}
	
return null;
```
Attachment #9121409 - Flags: ui-review?(alessandro) → ui-review+

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.

Flags: needinfo?(alessandro)
Duplicate of this bug: 1603728

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

Flags: needinfo?(alessandro)

Address Alessandro's review, comment 14.

Attachment #9121409 - Attachment is obsolete: true
Attachment #9121409 - Flags: review?(mkmelin+mozilla)
Attachment #9122277 - Flags: ui-review+
Attachment #9122277 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9122277 [details] [diff] [review]
1602431-removePills-focusSelect.patch (incl. fixes for bug 1603563, bug 1603728)

Sorry, I think [x] patch used to auto-check itself, but no more...
Attachment #9122277 - Attachment is patch: true

Bug 1603166 currently removes originalInput, which affects this bug.

Depends on: 1603166
Comment on attachment 9122277 [details] [diff] [review]
1602431-removePills-focusSelect.patch (incl. fixes for bug 1603563, bug 1603728)

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

Working pretty good. 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. Incidentally this is also how Gmail works

::: mail/base/content/mailWidgets.js
@@ +2031,5 @@
> +     * Get the nearest sibling pill which is not selected.
> +     *
> +     * @param {string} siblingsType (optional) direction of iterating siblings
> +     *   "next" (default): iterate next siblings
> +     *   "previous": iterate previous siblings

* @param {("next"|'"previous")}[siblingsType="next"] - direction of iterating siblings

.. is the way to document optional parameters and the default value

@@ +2043,5 @@
> +          if (!element.hasAttribute("selected")) {
> +            return element
> +          }
> +          element = element.nextElementSibling;
> +        }

there's a missing return null here

@@ +2250,4 @@
>      }
>  
>      /**
> +     * Handle keypress event on a pill of MailRecipientsArea.

a pill in the mail-recipients-area.

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

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

@@ +2517,5 @@
> +
> +    /**
> +     * Check if any pill in the addressing area is selected.
> +     *
> +     * @return {Boolean} true if any pill is selected,

boolean.
I don't think we need to describe the false case

@@ +2521,5 @@
> +     * @return {Boolean} true if any pill is selected,
> +     *                   false otherwise.
> +     */
> +    hasSelectedPills() {
> +      return Boolean(this.querySelector("mail-address-pill[selected]"));

shouldn't use Boolean like this, since the parameter is not a boolean
just use double not (!!) instead
Attachment #9122277 - Flags: review?(mkmelin+mozilla) → feedback+
Depends on: 1610883

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

Attachment #9122277 - Attachment is obsolete: true
Attachment #9122500 - Flags: ui-review+
Attachment #9122500 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9122500 [details] [diff] [review]
1602431-removePills-focusSelect.patch (incl. fixes for bug 1603563, bug 1603728)

Need to fix some more comment nits.
Attachment #9122500 - Attachment is obsolete: true
Attachment #9122500 - Flags: review?(mkmelin+mozilla)

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

Attachment #9122757 - Flags: ui-review+
Attachment #9122757 - Flags: review?(mkmelin+mozilla)
Attachment #9122757 - Attachment is patch: true

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.

Comment on attachment 9122757 [details] [diff] [review]
1602431-removePills-focusSelect.patch (incl. fixes for bug 1603563, bug 1603728)

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 wonder if it wouldn't be more simple and clearer to just have a different method for backspace deletion?

::: mail/base/content/mailWidgets.js
@@ +2093,4 @@
>       * Create a new recipient row container with the input autocomplete.
>       *
>       * @param {Array} recipient - The unique identifier of the email header.
> +     * @return {XULElement} - The newly created recipient row.

We can probably leave these at {Element} - since sooner or later they will be

@@ +2416,5 @@
> +     * @param {XULElement} pill - The mail-address-pill element where Event fired.
> +     * @param {("next"|"previous")} [focusType="next"] - How to move focus after
> +     *   removing pills: try to focus one of the next siblings (for DEL etc.)
> +     *   or one of the previous siblings (for BACKSPACE).
> +     * @param {boolean} [select = false] - After deletion, whether to select the

[select=false]

@@ +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

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

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

Attachment #9122757 - Attachment is obsolete: true
Attachment #9122975 - Flags: ui-review+
Attachment #9122975 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9122975 [details] [diff] [review]
1602431-removePills-focusSelect.patch

Need to update patch now that bug 1603166 has landed.
Attachment #9122975 - Flags: review?(mkmelin+mozilla)

Updated after bug 1603166.

Attachment #9122975 - Attachment is obsolete: true
Attachment #9123098 - Flags: ui-review+
Attachment #9123098 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9123098 [details] [diff] [review]
1602431-removePills-focusSelect.patch

Review of attachment 9123098 [details] [diff] [review]:
-----------------------------------------------------------------

I still have the feeling  removeSelectedPills does too many conflated things. But at least it works.

Re the encoding: please make sure to save the patch file as UTF-8. Looks like Firefox thinks it's not that atm.
Attachment #9123098 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 74.0

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

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

Flags: needinfo?(bugzilla2007)

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

Flags: needinfo?(bugzilla2007)

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.

Attachment #9123221 - Flags: feedback?(alessandro)
Attachment #9123221 - Flags: feedback?(acelists)

One nitfix for my testfix attempt.

Caveat: Please See comment 46 for details.

Attachment #9123222 - Flags: feedback?(alessandro)
Attachment #9123222 - Flags: feedback?(acelists)
Attachment #9123221 - Attachment is obsolete: true
Attachment #9123221 - Flags: feedback?(alessandro)
Attachment #9123221 - Flags: feedback?(acelists)
Comment on attachment 9123222 [details] [diff] [review]
1602431-removePills-testfix.patch

Review of attachment 9123222 [details] [diff] [review]:
-----------------------------------------------------------------

Multiple tests are still failing.
I'll take care of this, don't worry.
Attachment #9123222 - Flags: feedback?(alessandro)
Attachment #9123222 - Flags: feedback?(acelists)
Attachment #9123222 - Flags: feedback-

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

Attachment #9123098 - Attachment is obsolete: true
Attachment #9123222 - Attachment is obsolete: true
Attachment #9123246 - Flags: ui-review+
Attachment #9123246 - Flags: review+
Attachment #9123246 - Attachment is patch: true
Comment on attachment 9123246 [details] [diff] [review]
1602431-removePills-focusSelect-FIXED_TESTS.patch

Review of attachment 9123246 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/test/browser/shared-modules/ComposeHelpers.jsm
@@ +344,5 @@
>  function clear_recipient(aController) {
>    for (let pill of aController.window.document.querySelectorAll(
>      "mail-address-pill"
>    )) {
> +    pill.toggleAttribute("selected");

, true
Comment on attachment 9123246 [details] [diff] [review]
1602431-removePills-focusSelect-FIXED_TESTS.patch

Review of attachment 9123246 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for looking into this! I have one comment nit and some questions on approach, for all the reasons cited in comment 46, and trying to understand where my approach went wrong...

::: mail/test/browser/shared-modules/ComposeHelpers.jsm
@@ +340,1 @@
>   * Remove the recipient by typing backspaces.

Comment needs update: Remove all recipients.

@@ +340,5 @@
>   * Remove the recipient by typing backspaces.
>   *
>   * @param aController    Compose window controller.
>   */
>  function clear_recipient(aController) {

As we're removing *all* recipients, I think this function should be renamed here and everywhere: clear_recipients(...)

@@ +345,5 @@
>    for (let pill of aController.window.document.querySelectorAll(
>      "mail-address-pill"
>    )) {
> +    pill.toggleAttribute("selected");
> +    aController.e("recipientsContainer").removeSelectedPills(pill);

You are very lucky that this works because of my intelligent defaults... ;-)

But what is the advantage of going through the inherent complexity of .removeSelectedPills() for every single pill, every time we just want to clear all recipients in a tabula rasa approach?
pill.remove() as in my proposal looks faster and sufficient for this purpose. We're not testing delete functionality here, and we shouldn't, because this function runs a dozen times (so if delete functionality would ever fail, this function would also fail a dozen times).

The only advantage of .removeSelectedPills() is that it auto-sets the focus somewhere (in the rowInput of the last available row for our case here, I believe; which isn't very useful for subsequent tests). So that's why in my fix, I'm setting focus manually and predictably to the rowInput of the *first* available row. Any disadvantage to my approach? (I'd love to learn...)

I'd also like to know why my testfix fails (again, for learning purposes...), because imo there's essentially no difference - or maybe it fails on setting focus? Note that I don't need to select because I'm using pill.remove() directly.

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.

Flags: needinfo?(alessandro)
Flags: needinfo?(alessandro)

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.

Attachment #9123246 - Attachment is obsolete: true
Attachment #9123372 - Flags: ui-review+
Attachment #9123372 - Flags: review+

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

Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED
Comment on attachment 9123372 [details] [diff] [review]
1602431-removePills-focusSelect.patch

[Approval Request Comment]
Regression caused by (bug #): 440377
User impact if declined: 

- same degree of nuisance as bug 1603166, which has approval-comm-beta+
- prevent datalossy deletion of unselected focus pill
- prevent datalossy deletion of unexpected pills in the wrong direction (Backspace / Delete)
- prevent loss of focus when deleting pills
- prevent jumping focus which prevents verifying correct deletion
- enable predictable consecutive, controlled deletion of single pills with BACKSPACE and DELETE keys
- enable immediate and easy cut/copy/paste by selecting pills on plain navigation, e.g. via cursor keys

Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): none
Attachment #9123372 - Flags: approval-comm-beta?
Comment on attachment 9123372 [details] [diff] [review]
1602431-removePills-focusSelect.patch

No more betas this cycle.
Attachment #9123372 - Flags: approval-comm-beta?
You need to log in before you can comment on or make changes to this bug.