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

Status: RESOLVED FIXED

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

Description

[Thomas D. (:thomas8)]

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

Comment 1

[Thomas D. (:thomas8)]

^^

Comment 2

[Alessandro Castellani [:aleca]]

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

Comment 3

[Wayne Mery (:wsmwk)]

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

Comment 4

[Alessandro Castellani [:aleca]]

I'll take this and fix it.

Comment 6

[Alessandro Castellani [:aleca]]

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

Comment 7

[Alessandro Castellani [:aleca]]

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.

Comment 8

[Alessandro Castellani [:aleca]]

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.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: Bug 1729741 - Fix macOS native context menu issue and improve the recipients area blur events. r=mkmelin

Comment 10

[Alessandro Castellani [:aleca]]

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.

Comment 11

[Alessandro Castellani [:aleca]]

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

Comment 12

[Alessandro Castellani [:aleca]]

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.

Comment 13

[Magnus Melin [:mkmelin]]

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?

Comment 14

[Thomas D. (:thomas8)]

Attachment: 1729741_fixPillContextMacOS.diff

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.

Comment 15

[Alessandro Castellani [:aleca]]

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

Comment 16

[Alessandro Castellani [:aleca]]

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

Comment 17

[Alessandro Castellani [:aleca]]

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

Comment 18

[Alessandro Castellani [:aleca]]

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.

Comment 19

[Markus Stange [:mstange]]

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.

Comment 20

[Alessandro Castellani [:aleca]]

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.

Comment 21

[Alessandro Castellani [:aleca]]

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

Comment 22

[Alessandro Castellani [:aleca]]

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

Comment 23

[Wayne Mery (:wsmwk)]

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

Comment 24

[Thomas D. (:thomas8)]

(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

Comment 25

[Carl]

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

Comment 26

[doug2]

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.

Comment 27

[Alessandro Castellani [:aleca]]

Another try-run to fix one last tiny failing test: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=abc2314b259879fe8e323759af899a1290d870f0

Comment 28

[Thomas D. (:thomas8)]

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

Comment 29

[Calum Mackay]

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

Comment 30

[Pulsebot]

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

Comment 31

[Alessandro Castellani [:aleca]]

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.

Comment 32

[Alessandro Castellani [:aleca]]

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.

Comment 33

[Wayne Mery (:wsmwk)]

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

Comment 34

[Rob Lemley [:rjl]]

Thunderbird 91.1.1:
https://hg.mozilla.org/releases/comm-esr91/rev/9626369176a3

Comment 35

[Rob Lemley [:rjl]]

Thunderbird 93.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/e4aa7bc29ea5