1645643 - Deleting pills with BACKSPACE (aka DELETE, ←) key sometimes fails on MAC, depending on prior focus

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P1)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of Bug #1602431 +++

78 beta. Mac. I don't often delete pills so I don't know when this changed, assuming it ever worked.

"fail" Reply to a message with two addresses. Click on a pill (either one), hit delete, nothing. And nothing in error console.

"works" Reply to a message with two addresses. Click after last pill, hit delete, last pill is selected, hit delete again, pill is deleted

"works" Reply to a message with two addresses. Click after last pill, click any pill, hit delete, pill is deleted

I don't have add-ons installed that would affect compose.

Comment 1

[Wayne Mery (:wsmwk)]

(I cleaned up the description - there are only three lines of fail + works)

Also, I had situations where pill is selected by click but caret is elsewhere, like subject such that delete key results in deleting characters in subject

Comment 2

[Alessandro Castellani [:aleca]]

This is definitely a regression that happened recently.
I'll investigate and create a patch for it.

Comment 3

[Alessandro Castellani [:aleca]]

This doesn't seem to happen on Linux or Windows, but only on macOS.
Thomas, can you confirm that on Windows everything works as expected.

Comment 4

[Thomas D. (:thomas8)]

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

78 beta. Mac.
"fail" Reply to a message with two addresses. Click on a pill (either one), hit delete, nothing. And nothing in error console.

• Which recipient types are recipients in original message being replied to?
• Are you using Reply-all? (otherwise how did you get two pills in the reply?)
• Are you using MAC key Delete ⌦ or the MAC equivalent of Windows Backspace ← (confusingly traditionally called Delete on MAC)
• Signatures, signature settings?
• Where is cursor/focus before clicking on pills in "fail" case? (Might involve focus/controller issues given "works" cases).

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

This doesn't seem to happen on Linux or Windows, but only on macOS.
Thomas, can you confirm that on Windows everything works as expected.

Deletion works perfectly well on Win10-64, I've just downloaded Beta 78.0b1 (32-bit) to test (does 32 vs. 64 bit matter for this?). However, STR may not be detailed enough to reproduce.

Alex, can you reproduce this bug on MAC? (Not explicitly clear from comment 3)

Comment 5

[Thomas D. (:thomas8)]

^^ comment 4

Comment 6

[Alessandro Castellani [:aleca]]

I found the problem and I was able to consistently recreate the issue.

The issue presents itself when the addressing row has the focus and then a pill is selected.
The focus remains on the addressing row, therefore making the delete useless as the focus needs to move to the pill first.

This method is the culprit and I think I found the bug that regressed it.
https://searchfox.org/comm-central/rev/361d3ff8a878f3ea9d7dbac0f1dce07dffc7d0ed/mail/base/content/mailWidgets.js#2489

This is a macOS issue only.

Comment 7

[Alessandro Castellani [:aleca]]

Found the regression by reapplying this revision: https://hg.mozilla.org/comm-central/rev/06b4fa036ccda170128e73093c437e9d545a9ba7

Apparently, the focus on macOS doesn't change naturally when clicking on a pill element, that's why I added that forced pill.focus().
It was correctly removed by Thomas because the blur() interfered with the common retention of focus when CTRL+Click on a pill to unselect it.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1645643-delete-pills.diff

This fixes the issue.
I'm asking a review to Richard as he can build on macOS.

Thomas, can you confirm this change doesn't affect the work done on the focus ring on Windows?

Comment 9

[Thomas D. (:thomas8)]

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

Thomas, can you confirm this change doesn't affect the work done on the focus ring on Windows?

I don't know how this relates to focus ring, but it does not negatively impact focus upon click or pressing space bar. However, assuming that MAC allows for equivalent of Ctrl+Click, or spacebar to remove focused pill from selected pills, focus must always end up on that pill for this method even when it is no longer selected. So I think you can just do pill.focus always (without wrapping in condition). Regardless, it might trigger all sorts of command updating in compose window, not sure if triggered by just using focus() or actual focus change.
(--> Matrix?)

Comment 10

[Thomas D. (:thomas8)]

And can you please file another bug for MAC that focus must follow mouse clicks without extra code?

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1645643-delete-pills.diff

Patch updated to remove the unnecessary condition.
Also, comment and commit message have been updated.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9156856 [details] [diff] [review]
1645643-delete-pills.diff

Finally it worked for me on Mac. I needed to build with the patch although it is only a JS change. Maybe Mac didn't clear the cache. Tested also on Windows.

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9156856 [details] [diff] [review]
1645643-delete-pills.diff

[Approval Request Comment]
Regression caused by (bug #): bug 1633555
User impact if declined: macOS users will not be able to focus on clicked pills
Testing completed (on c-c, etc.): not yet as C-C is currently closed
Risk to taking this patch (and alternatives if risky): very low and Windows and Linux already move the focus automatically on click

Comment 14

[Thomas D. (:thomas8)]

Just give me some minutes to double-check on something.
Sorry, due to Bug 1643340 I lost some time before being able to get back to this.

Comment 15

[Thomas D. (:thomas8)]

Attachment: 1645643-delete-pills.diff

I suggest some minor modifications/corrections to commit message, function comment, and inline comment. Code is unchanged, so :paenglab's r+ still stands.

As a matter of fact, we are only fixing focus for the click event, due to MAC-only bug 1645916.
We're not fixing anything for the keypress event, for which the pill is already focused (otherwise it couldn't receive the keypress). That we're also (needlessly) re-focusing the pill after keypress is just a side effect of trying to keep our code simple.

Comment 16

[Alessandro Castellani [:aleca]]

Comment on attachment 9157075 [details] [diff] [review]
1645643-delete-pills.diff

[Approval Request Comment]
Regression caused by (bug #): bug 1633555
User impact if declined: macOS users will not be able to focus on clicked pills
Testing completed (on c-c, etc.): not yet as C-C is currently closed
Risk to taking this patch (and alternatives if risky): very low and Windows and Linux already move the focus automatically on click

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d667aaed4e09
[macOS] Force focus when a click event is registered on a recipient pill. Workaround for bug 1645916. r=Paenglab

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9157075 [details] [diff] [review]
1645643-delete-pills.diff

Approved for beta

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 78.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/e431355d0d28