1710275 - nsMsgMailSession::GetTopmostMsgWindow() broken since bug 1509060

Status: RESOLVED FIXED

(MailNews Core :: General, defect)

Description

[José M. Muñoz]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.93 Safari/537.36

Steps to reproduce:

In bug 1708073 comment #13 I noticed that that nsMsgMailSession::GetTopmostMsgWindow() no longer returns the top-most 3pane or stand-alone message window. Most call sites
https://searchfox.org/comm-central/search?q=GetTopmostMsgWindow%28&path=
don't seem to care as long as the get at least a 3pane window, but here
https://searchfox.org/comm-central/rev/dbdd31e28cca8282dc44fd2cc4280af40e1cb50b/mailnews/local/src/nsLocalMailFolder.cpp#2829
it is important to get the correct top-most window in which the user clicked the "download rest of message" link.

I have the impression that this call-site
https://searchfox.org/comm-central/rev/dbdd31e28cca8282dc44fd2cc4280af40e1cb50b/mailnews/base/src/nsMessengerWinIntegration.cpp#131
would prefer the current defective behaviour by activating a 3pane and not a stand-alone window.

In any case, this needs to be fixed as suggested in bug 1509060 comment #1 / bug 1489833 comment #3.

Comment 1

[José M. Muñoz]

https://searchfox.org/comm-central/rev/dbdd31e28cca8282dc44fd2cc4280af40e1cb50b/mailnews/compose/src/nsMsgCompose.cpp#156
may also rely on getting the correct top-most window, something like: Open a message in a stand-alone window, correct and incorrect charset, reply or forward/"edit as new". Looks like the code gets the charset from the top-most window.

Comment 2

[José M. Muñoz]

Further to the previous comment: As predicted, opening a message in a stand-alone window, changing the charset and then replying doesn't pick up the charset that was just set any more. It still works when replying form the main window.

Comment 3

[Ping Chen [:rnons] (away)]

In nsMailSession.cpp, we are using domElement.getAttribute("windowtype") to get the window type, so I was thinking maybe we can use js to detect if a window is active or not, then toggle an attribute on the html element. I open a new msg window, inspect it in devtools, run the following js in the console tab

window.addEventListener('focus', () => console.log('focus'))
window.addEventListener('blur', () => console.log('blur'))

There are sub-frames in msg window, when focusing a sub-frame, the msg window itself loses focus, so we don't really know if msgwindow is active.

I also tried visibilitychange event, but it's not triggered when using cmd+tab to switch window. I have no idea now.

Comment 4

[Magnus Melin [:mkmelin]]

Probably check focus manager: https://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIFocusManager.idl maybe activeWindow?

Comment 5

[Ping Chen [:rnons] (away)]

Attachment: Bug 1710275 - Use nsIFocusManager.activeWindow to fix nsIMsgMailSession.topmostMsgWindow. r=mkmelin

Previously GetTopmostMsgWindow returns the first 3pane or messageWindow found, this patch uses nsIFocusManager to guarantee it's the topmost window.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ddee6960495f
Use nsIFocusManager.activeWindow to fix nsIMsgMailSession.topmostMsgWindow. r=mkmelin

Comment 7

[José M. Muñoz]

The patch fixes bug 1708073 and the scenario from comment #1. Please make sure that the call site in nsMessengerWinIntegration.cpp still behaves as desired. That code was added after the breakage had occurred in bug 1509060, so there a 3pane window would always have been activated which is now no longer the case.

Comment 8

[Ping Chen [:rnons] (away)]

Attachment: Bug 1710275 - Activate the most recent 3pane window when click the tray icon on Windows. r=mkmelin

Comment 9

[Ping Chen [:rnons] (away)]

OK, made a new patch.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f4d8dbfde1d5
Activate the most recent 3pane window when click the tray icon on Windows. r=mkmelin

Comment 11

[José M. Muñoz]

Any reason for not calling GetMostRecentWindow(u"mail:3pane", getter_AddRefs(domWindow)); directly like in most other call sites? The wrapper
https://searchfox.org/mozilla-central/rev/048146721b3c1a568257afb4325c7a8d94334e2d/xpfe/appshell/nsWindowMediator.cpp#235
unnecessarily calls GetMostRecentWindow(u"navigator:browser", ...) first. The same comment applies to:
https://searchfox.org/comm-central/rev/b9e78aad665f4d6d159adcb508e1fe55ed7c9ac8/mailnews/base/src/WinUnreadBadge.jsm#199.
I suggest to file another bug to replace those two calls.