Closed Bug 1645643 Opened 2 months ago Closed 2 months ago

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

Categories

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

All
macOS

Tracking

(thunderbird78? fixed)

RESOLVED FIXED
Thunderbird 79.0
Tracking Status
thunderbird78 ? fixed

People

(Reporter: wsmwk, Assigned: aleca)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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

(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

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

Assignee: nobody → alessandro

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.

Flags: needinfo?(bugzilla2007)

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

Flags: needinfo?(bugzilla2007)
Summary: Deleting pills with BACKSPACE or DEL key error-prone, disfunctional → Deleting pills with BACKSPACE (aka DELETE, ←) key sometimes fails on MAC, depending on prior focus
Flags: needinfo?(vseerror)

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.

Flags: needinfo?(vseerror)
OS: All → macOS
Priority: -- → P1
Severity: -- → S2
Regressed by: 1633555

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.

Attached patch 1645643-delete-pills.diff (obsolete) — Splinter Review

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?

Attachment #9156840 - Flags: review?(richard.marti)
Attachment #9156840 - Flags: feedback?(bugzilla2007)
Status: NEW → ASSIGNED

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

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

Attached patch 1645643-delete-pills.diff (obsolete) — Splinter Review

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

Attachment #9156840 - Attachment is obsolete: true
Attachment #9156840 - Flags: review?(richard.marti)
Attachment #9156840 - Flags: feedback?(bugzilla2007)
Attachment #9156856 - Flags: review?(richard.marti)
Attachment #9156856 - Flags: feedback?(bugzilla2007)
See Also: → 1645916
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.
Attachment #9156856 - Flags: review?(richard.marti) → review+
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
Attachment #9156856 - Flags: approval-comm-beta?

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.

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.

Attachment #9157075 - Flags: review+
Attachment #9157075 - Flags: feedback?(alessandro)
Attachment #9157075 - Flags: feedback?(alessandro) → feedback+
Attachment #9156856 - Attachment is obsolete: true
Attachment #9156856 - Flags: feedback?(bugzilla2007)
Attachment #9156856 - Flags: approval-comm-beta?
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
Attachment #9157075 - Flags: approval-comm-beta?

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 79.0
Comment on attachment 9157075 [details] [diff] [review]
1645643-delete-pills.diff

Approved for beta
Attachment #9157075 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.