Closed Bug 1729741 Opened 1 year ago Closed 1 year ago

[macOS] Right-click on a recipient pill fails to select it/clears selection with widget.macos.native-context-menus set to true

Categories

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

Thunderbird 93
Unspecified
macOS

Tracking

(thunderbird_esr91+ fixed, thunderbird93? fixed, thunderbird94+)

RESOLVED FIXED
94 Branch
Tracking Status
thunderbird_esr91 + fixed
thunderbird93 ? fixed
thunderbird94 + ---

People

(Reporter: thomas8, Assigned: aleca)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Thanks to Wayne who found this on TB 93 (on Matrix today).
Maybe regressed. Aleca reports it also affects 91.1.
Makes contextual actions on a single recipient pill with mouse a no-op, which is a pretty pretty significant and annoying everyday loss of functionality. Should fix this asap.

Aleca, could you have a look?

STR

  1. On Mac Big Sur, right-click on a recipient pill in composition's addressing area
  2. Check if pill is selected
  3. Choose Cut from context menu

Actual

  • On Mac Big Sur / TB 93, right-click no longer selects the pill
  • pill not cut; contextual actions all fail (except Edit Address)

Expected

  • Right-click on a pill must select it first (as it does on Windows)
  • pill should be cut away; contextual actions from a plain vanilla right-click on a pill should all work

^^

Flags: needinfo?(alessandro)

Confirmed.
This also affects 91.1 on macOS.
Bumping up the severity and priority as this issue is pretty disruptive.

Severity: S3 → S2
Flags: needinfo?(alessandro)
Priority: P2 → P1

It also fails for me with 92.0b5. So I don't have a regression range.

I'll take this and fix it.

Assignee: nobody → alessandro
  • 89b3 works.
  • 89b4 has a partial regression (right click doesn't select but if the pill is selected, the right click doesn't provoke a deselection).
  • 90b1 doesn't work entirely and the issue remained identical as of now.
Regressed by: 1700679
Summary: On Mac Big Sur, right-click on a recipient pill fails to select it, which renders contextual actions like Cut/Copy/Delete no-op → [macOS] Right-click on a recipient pill fails to select it/clears selection with widget.macos.native-context-menus set to true

Found the issue in this code: https://searchfox.org/comm-central/rev/0fc487b31cc488a9fa042a41d16ba6d96a842a18/mail/base/content/widgets/mailWidgets.js#2494-2496

Apparently, native macos context menus trigger a focusout event.
This doesn't happen on Windows or Linux.

Coming up with a fix.

I'm having issues in figuring out how to solve this. These are my findings:

  • The focusout listener attached to the entire recipients area doesn't play well with native macos context menus as they steal the focus and causes the pill deselection code to run.
  • The fact that the event listener is attached to the entire recipient area and we let it bubble up to all its child elements is not ideal as it doesn't leave us much flexibility to handle these edge cases.

Possible solutions/approaches to fix this:

  • Force the native macos context menus to not steal the focus if the original target is a pill (not sure if it's doable and if it could affect the keyboard navigation of the context menu items).
  • Drop the focusout and handle blur events to the elements we need to listen to (recipients area, input fields, pills) to avoid bubbling and preventing deselection with more fine tuning.

Maybe a smarter approach that is escaping me right now.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(bugzilla2007)

I think I fixed all the scenarios with these changes.
I need to test this patch on Linux and Windows to be sure I didn't cause any regression.
If this approach looks correct, I'll add a bunch of tests to cover every scenario for all platforms.

Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)

And of course, this patch breaks things on Windows but works perfectly on macOS.

The problem in general is the focusout event listener which gets triggered with the new macOS native context menus.
The focusout event is used to properly deselect pills when the focus leaves the recipients area (eg. moves to the contacts sidebar, or any other element).

Another possible solution would be to not use the focusout event but add a onclick event to the compose window in order to deselect all selected pills only on specific occasions. This would allow us to ignore the lost focus when opening the context menu.
I'm not sure how hacky something like that might be.

Flags: needinfo?(mkmelin+mozilla)

Using onclick instead seems it could be trouble, but would need some experimentation...

Could we perhaps find a way to ignore focusout if we know it's coming from the context menu?

Flags: needinfo?(mkmelin+mozilla)
Attached patch 1729741_fixPillContextMacOS.diff (obsolete) — Splinter Review

This might be enough to fix it. Can't test on Mac.
I'm assuming that it's the popup which gets the focus when context menu triggers focusout event on Mac. Otherwise just substitute the context menu ID with whichever element gets focus.

Flags: needinfo?(bugzilla2007)
Attachment #9240272 - Flags: review?(alessandro)
Comment on attachment 9240272 [details] [diff] [review]
1729741_fixPillContextMacOS.diff

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

Thanks for the suggestion but unfortunately this doesn't work on macOS (it was the first thing I tried).
With the native context menus, the `event.relatedTarget` is `null`, but not just for the context menu.
It returns null also when the focus is moved tot he message body, or the contact sidebar, or any other element not in the message header, so we can't use that as a condition.

Another problem that is not covered in this patch but I did in mine, is that right clicking on a non-selected pill, doesn't select it (only on macOS as usual).
Attachment #9240272 - Flags: review?(alessandro) → review-

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

Using onclick instead seems it could be trouble, but would need some experimentation...

I think I have a "not so dirty" patch with this approach.

Could we perhaps find a way to ignore focusout if we know it's coming from the context menu?

I tried, but on macOS the focusout event triggered by native context menus doesn't come with any unique pointer indicating the origin of it.

Pinging Markus to see if he has any insight on this and if it might be a toolkit issue with the native context menu.

Flags: needinfo?(mstange.moz)

I updated the patch on Phabricator with the onclick approach.
That fixes everything on macOS, but I need to test things properly on Windows and Linux. I uploaded it for easy cross platform fetching and updating.

Attachment #9240272 - Attachment is obsolete: true

Interesting, I wasn't aware of the focus phenomenon with native context menus. Could you file a bug about that? I'm not sure what causes the focus change here, maybe the native window itself gets an "unfocused" notification from macOS.

Flags: needinfo?(mstange.moz)

Updated patch on phabricator now works on all platforms.
Here's an overview of what I did:

  • Remove the focusout event listener from the entire recipient area (this removes also the issue of detecting if the user is changing to another window)
  • Handle the selection and deselection of pills through the pill's onclick event.
  • Handle the deselection of single pills through the pill's onblur event.
  • Handle the deselection of multiple pills through the window's onclick event, preventing deselection if the click happens on a pill, or an element we want to ignore (right click, menubar, toolbarbutton).

Having a click event attached to the whole window is not optimal, but maybe this could be a good enough solution to fix this problem before 91 gets widely adopted, meanwhile we investigate the cocoa toolkit issue.

Try run launched with newly implemented tests to cover all the scenarios listed above and some context menu actions: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=14e4646ec1486d2df4136028c7fac1a22767450c

And of course synthesizeMouseAtCenter doesn't work for context menu items on macOS.
Test updated and launched another try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9937e41e993bdc051e7346eeb70aa3fde3776bd5

Blocks: tb91found

tested comment 22 try run on Mac. looks good. Though probable that I didn't cover all the bases.

One unexpected thing to note - I somehow caused the To: field contents to be selected when I did command+A or some variation

Calum, dossy, possible for you to test Mac? https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/BsPTuFvBRr2y4dtXGr_BcA/runs/0/artifacts/public/build/target.dmg

Doug, Walt, possible for you to test Windows? 64bit https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/O1mlezSTQjyaF22LmZ4WOA/runs/0/artifacts/public/build/install/sea/target.installer.exe

try build is based on v94 nightly, so you may want a new profile, unless you are already running nightly

Flags: needinfo?(wls220spring)
Flags: needinfo?(dossy)
Flags: needinfo?(dmccammishjr)
Flags: needinfo?(calum.mackay)

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

One unexpected thing to note - I somehow caused the To: field contents to be selected when I did command+A or some variation

That's a feature :-)

  • First Ctrl/Cmd+A selects all pills in current recipient field (with focus on a pill or empty input; obviously plain text in input will absorb this).
  • Another Ctrl/Cmd+A selects all pills in all recipient fields. Very convenient for removing all recipients.

It's also documented:
https://support.mozilla.org/en-US/kb/addressing-email#w_selecting-several-or-all-recipient-items

(In reply to Thomas D. (:thomas8) from comment #24)

  • Another Ctrl/Cmd+A selects all pills in all recipient fields. Very convenient for removing all recipients.

But this also selects the 'Reply-To' pill and you don't really want to remove this one??

Tested Win 10 64 bit TBird 93.0b1 Right click on pill brings up menu with "Edit Address" etc. Tried To, CC, BCC and Reply-To
Appears to be ok. Well done.

Target Milestone: --- → 94 Branch

(In reply to Carl from comment #25)

(In reply to Thomas D. (:thomas8) from comment #24)

  • Another Ctrl/Cmd+A selects all pills in all recipient fields. Very convenient for removing all recipients.
    But this also selects the 'Reply-To' pill and you don't really want to remove this one??

Interesting point, Carl, but we can't discuss that here. You could file an RFE. There are pros and cons to excepting Reply-To (and Auto-CC/BCC) from Select-All shortcut.

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

Calum, dossy, possible for you to test Mac? https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/BsPTuFvBRr2y4dtXGr_BcA/runs/0/artifacts/public/build/target.dmg

Looks good to me on MacOS 10.15.7 (19H1323), so far.

Thanks for fixing this one.

Flags: needinfo?(calum.mackay)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/6fec250b1d73
Fix macOS native context menu issue and improve the recipients area blur events. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Flags: needinfo?(wls220spring)

Comment on attachment 9240190 [details]
Bug 1729741 - Fix macOS native context menu issue and improve the recipients area blur events. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): bug 1700679
User impact if declined: Impossible to use the right click context menu on recipient pills with the native macos widgets.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): medium, as it drastically changes the triggers of the context menus for all platforms, but we should be safe as new tests have been implemented to cover the entire workflow.

Attachment #9240190 - Flags: approval-comm-beta?

Comment on attachment 9240190 [details]
Bug 1729741 - Fix macOS native context menu issue and improve the recipients area blur events. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): bug 1700679
User impact if declined: Impossible to use the right click context menu on recipient pills with the native macos widgets.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): medium, as it drastically changes the triggers of the context menus for all platforms, but we should be safe as new tests have been implemented to cover the entire workflow.

Attachment #9240190 - Flags: approval-comm-esr91?

Comment on attachment 9240190 [details]
Bug 1729741 - Fix macOS native context menu issue and improve the recipients area blur events. r=mkmelin

[Triage Comment]
Approved for beta

[Triage Comment]
Approved for esr91

Flags: needinfo?(dossy)
Flags: needinfo?(dmccammishjr)
Attachment #9240190 - Flags: approval-comm-esr91?
Attachment #9240190 - Flags: approval-comm-esr91+
Attachment #9240190 - Flags: approval-comm-beta?
Attachment #9240190 - Flags: approval-comm-beta+
Regressions: 1762895
You need to log in before you can comment on or make changes to this bug.