Closed Bug 1863798 Opened 8 months ago Closed 2 months ago

Clicking on new mail notification on Windows doesn't bring the application to the foreground

Categories

(Thunderbird :: General, defect, P2)

Thunderbird 120
Unspecified
Windows

Tracking

(thunderbird_esr115 wontfix, thunderbird127 verified)

VERIFIED FIXED
128 Branch
Tracking Status
thunderbird_esr115 --- wontfix
thunderbird127 --- verified

People

(Reporter: doelli, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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

Steps to reproduce:

  1. Make sure you have both notifications and system notifications turned on.
  2. Wait for a new mail.
  3. Notification pops up in the Notifications section in Windows 11.
  4. Click on the notification itself.

Actual results:

Nothing happens.

Expected results:

Clicking on notification should open up Thunderbird and the mail that we've been notified about.

TB 120 Beta: After setting alerts.useSystemBackend to true (see bug 1862407), I see this behaviour:

  1. It takes about 10 seconds for TB to become active.
  2. A minimised window is restored, but a non-minimised window in the background is not brought to the foreground.
Summary: Clicking on new mail notification on Windows does nothing → Clicking on new mail notification on Windows does nothing in TB 120 beta (hence not a dupe of bug 1838139)

We looked at the two issues a bit. The delayed activation is quite a mystery, we filed this upstream ticket for it.

The code in ToastNotificationHandler::OnActivate() here
https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/widget/windows/ToastNotificationHandler.cpp#873
doesn't work for Thunderbird. It can easily be fixed with this patch:
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1863798-toast-notification-foreground-mail-client-m-c.patch
since GetMostRecentBrowserWindow() also works for Thunderbird. However, observed behaviour is that the window is still not brought to the foreground, even minimised windows are not reliably restored, sometimes only their taskbar button flashes.

That's particularly surprising since this code
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/nsMessengerWinIntegration.cpp#79,166
works reliably.

There is a hint in this article (quote):
SetForegroundWindow() is the correct way to change the foreground window, but the thread that calls SetForegroundWindow() has to meet certain criteria for it to "work". If it does not meet the criteria, the window's taskbar button is flashed instead. This is by design. This is to protect the user from applications stealing focus and you should respect that.

Looks like the Toast activation used a different thread than the system tray activation in nsMessengerWinIntegration.cpp.

Strangely the activation calls this:
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/MailNotificationManager.jsm#99
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mail/modules/MailUtils.jsm#359
And .showWindow() calls this:
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/nsMessengerWinIntegration.cpp#230
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/nsMessengerWinIntegration.cpp#79
which is the code that works for system tray enabling.

Makoto san, do you have any insights?

Flags: needinfo?(m_kato)
See Also: → 1838139

Nick, you fixed bug 1795974. Can you help with this bug here? As per comment #0 there are two problems:

  1. It takes about 10 seconds for before ToastNotificationHandler::OnActivate() gets called
  2. SetForegroundWindow() here and here doesn't have the permissions to bring the Mozilla app window to the foreground, likely because the process handling the click didn't grant that privilege.

Questions:

  1. Any insights re. the activation delay?
  2. Does the SetForegroundWindow() at ToastNotificationHandler.cpp#879 work in FF or does it fail silently?
  3. In ToastNotification.cpp#609 (from bug 1795974) you allude to (quote) "Send our window's PID to the notification server", but is that actually done? The PID is a local variable, there is no call to AllowSetForegroundWindow() we can see anywhere. Besides, ToastNotification::SignalComNotificationHandled() is only called as part of ToastNotification::HandleWindowsTag().
Flags: needinfo?(nrishel)

For the record, on codeproject.com we got given this advice:

You can pass the PID to the WinToastHandler constructor before you call showToast(). It is up to you to add the PID member to WinToastHandler class.

std::shared_ptr<IWinToastHandler> handler(new WinToastHandler(this));
if (WinToast::instance()->showToast(templ, handler) == -1L)
{...}

If we interpret this correctly, the Mozilla app PID should be retrieved here and added to the Toast handler:
https://searchfox.org/mozilla-central/rev/27ddf0064aa821af8238a99621df79fb1b301c7b/toolkit/mozapps/defaultagent/Notification.cpp#511
Then upon activation in toastActivated() in the handler a call to AllowSetForegroundWindow() should be made with that PID assuming that the handler runs in the context of the notification server. This way the notification server grants the privilege to the Mozilla app. We'll try this soon.

Flags: needinfo?(m_kato)

We'll try this soon.

No dice at all, this isn't even being compiled for TB:
https://searchfox.org/mozilla-central/rev/47e291435c52512f0c664a89abaf1dcab01c2f69/toolkit/mozapps/defaultagent/Notification.cpp#513

Side note: The "old method" (alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled false) worked in 102 and brought the window to the foreground, in 115 even that doesn't work any more.

I'm not sure I have the full context here but if TB is using WinToast for notifications then you're outside of the context of Gecko's native nsIAlert notifications. In general we're on track to phase out our use of WinToast, which is exclusively used in the Default Agent which was previously a standalone exe.

There's WIP documentation explaining the not-intuitive control flow of notifications on Windows. The jist is that handling for notification callbacks is split across two executable: a DLL Surrogate using our NotificationServer COM DLL and Firefox's in-memory notification callbacks. This split is necessary as the COM surrogate will reinvoke Firefox when it is closed before a user interacts with the notification, when alerts.useSystemBackend.windows.notificationserver.enabled is false the DLL Surrogate early exits and the application won't be restarted.

The DLL Surrogate also needs to pass Foreground permissions to Gecko, whether these permissions can be passed along is heuristic in nature and seems to rely on the receiving process having an event loop/window. This presents an issue when interacting with the Remote Server which will instead pass along the command line to a running Gecko instance and exit, because it breaks the heuristic chain in some way. To deal with this we have the final running Gecko instance which will handle the notification communicate back to the DLL Surrogate via a pipe it's PID, which the notification server then grants foreground permission to.

A note for anyone testing, SetForegroundWindow frustratingly always succeeds when run under a debugger. Also an application which doesn't have foreground permissions can not grant or set foreground permissions, so passing around the PID at time of notification creation won't work if we want to e.g. set a relaunched gecko instance to the foreground.

With that context out of the way:

  1. Any insights re. the activation delay?
  2. Does the SetForegroundWindow() at ToastNotificationHandler.cpp#879 work in FF or does it fail silently?
    It works, see above.

If the notification server is enabled via alerts.useSystemBackend.windows.notificationserver.enabled, the notification server has a 10 second timeout before it exits the notification handler so it can pass foreground permissions to the receiving gecko process. If enabled but the PID is never passed back, we'd expect this hitch.

  1. In ToastNotification.cpp#609 (from bug 1795974) you allude to (quote) "Send our window's PID to the notification server", but is that actually done? The PID is a local variable, there is no call to AllowSetForegroundWindow() we can see anywhere. Besides, ToastNotification::SignalComNotificationHandled() is only called as part of ToastNotification::HandleWindowsTag().

When the notification server is enabled it always creates a new gecko process, which results in a new command line being passed either because a new gecko process was created or because the remote server passes it into an existing process. Therefore SignalComNotificationHandled will always be called (unless if the remote server is disabled but we don't support that use case).

Flags: needinfo?(nrishel)

Maybe it's best to take a step back and look at the context.

TB is using nsIAlert here:
https://searchfox.org/comm-central/rev/f9b5e9d86828d56563cf6dff61ca001230931769/mailnews/base/src/MailNotificationManager.jsm#341-367
if the user has chosed to use "system alerts". If not, a XUL/XHTML window is shown.

We believe there is no requirement to launch TB when the user closed TB and hasn't clicked onto the notification. On the other hand, the delay of 10 seconds is not useful since the user is used to clicking onto the notification and getting their message shown.

The observed behaviour for windows system notifications/alerts is this:
In TB 102 everything works.
In TB 115 the notification server is not included, missing back port of bug 1838139 and bug 1848232 to ESR 115.
TB has set alerts.useSystemBackend to false by default, but alerts.useSystemBackend.windows.notificationserver.enabled to true which leads to total malfunction (bug 1858603, bug 1840123).
With alerts.useSystemBackend.windows.notificationserver.enabled also set to false manually, 115 works as 102, so the activation via the notification-click is immediate, minimised windows are restored, but non-minimised windows aren't raised due to a failing SetForegroundWindow(). We've experimented putting the 102 code in ToastNotification[Handler].{cpp|h} into the 115 tree and that in fact restores 102 behaviour. So there must be one change between 102 and 115 which damages the SetForegroundWindow().

In TB's current trunk the notification server is included, so there are two options:
alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at false. That gives
115 behaviour, working, but no SetForegroundWindow().

alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at true. That gives
the behaviour reported in comment #0: working with 10 sec delay, but also no SetForegroundWindow().

So where does that leave TB? As the 10 second delay is not desired and there is no requirement to relaunch, should TB pursue alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at false?

If so, do you have any hint why in this combination the SetForegroundWindow() still fails?

BTW, yes, the debugging quirk of SetForegroundWindow() succeeding during debugging has been documented here (quote):
Either the foreground process or the calling process is being debugged.

We're not setting NI again to avoid further noise but we'd appreciate your advice.

The least brittle approach IMO is to opt into the relaunch behavior and find some sensible action to take.

The second approach would be to in some way disable the notification server registration from both the installer and during runtime. I don't recommend this approach because you'll also need to remove existing registrations (messy), and you'll need to change from the Toast-specific AUMID to the Start Menu shortcut AUMID (this will introduce an unfixable bug for users who opt out of a Start Menu entry). I suspect the fastest way to resolve this issue is adopting and adapting to the new notification server.

In TB 102 everything works.
In TB 115 the notification server is not included, missing back port of bug 1838139 and bug 1848232 to ESR 115.

I believe the notification server changed landed in Fx110.

(In reply to Nick Rishel [:nrishel] from comment #8)

The least brittle approach IMO is to opt into the relaunch behavior and find some sensible action to take.

OK, that means alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at true and fixing/adjusting the 10 seconds activation delay and the SetForegroundWindow() issue as originally reported. With all due respect, we don't believe there is anyone on the TB team capable of doing this since all this is very close to Windows system programming and the required code changes are in the Mozilla platform code.

Attachment #9385900 - Attachment is obsolete: true

Actually, I am not so sure what TB 120 means, if the current release is 115.8.1 for me as a regular user. I simply know that the notifications are useless, because clicking on the does nothing on Windows 10 Pro. This seems like a major issue to me. Why was it not fixed yet? Can somebody explain in a way understandable for simple users? (I am a software developer, but regarding TB I am just a simple user using a an e-mail client.)

TB 120 is a beta version. The bug related to TB 115 is bug 1858603.

I'm fixing a few tiny things in bug 1893349 in order to remove the 10 seconds delay and fix the display of the message after clicking on the notification.
Once that's done, we will continue the work in this bug to fix the SetForegroundWindow() not working for Thunderbird.

Status: UNCONFIRMED → NEW
Depends on: 1893349
Ever confirmed: true
Summary: Clicking on new mail notification on Windows does nothing in TB 120 beta (hence not a dupe of bug 1838139) → Clicking on new mail notification on Windows doesn't bring the application to the foreground

Continuing the work here after more info and some suggestions reported in bug 1893349 comment 12

Assignee: nobody → alessandro
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P2
Depends on: 1898709
Attachment #9401184 - Attachment is obsolete: true
Assignee: alessandro → geoff
Duplicate of this bug: 1862407
Target Milestone: --- → 128 Branch

Pushed by solange@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/8e7090d4739e
Fix bringing the app to the foreground when clicking on native Windows notifications. r=aleca

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

Comment on attachment 9404233 [details]
Bug 1863798 - Fix bringing the app to the foreground when clicking on native Windows notifications. r=aleca

[Approval Request Comment]
Approved for beta (bringing up some notification patches in advance of 128.0b1)

Attachment #9404233 - Flags: approval-comm-beta+
Regressions: 1900047

Hello,

Managed to reproduce this issue with the affected build from 2023-11-08 (20231108105500) using Windows 11.

Confirming this issue as verified fixed on 127.0b5(20240603175044) using macOS 14, Windows 11 and Ubuntu 22.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: