1489967 - Keep focus on Chrome window when switching to a different application with focusmanager test mode enabled

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Neil Deakin from comment #46)
> > Neil, what should be the correct behavior with `focusmanager.testmode` in
> > the last scenario? Do we really have to set the focus over and over again to
> > the chrome window, or can we simply keep it focused even when another
> > application is brought into foreground? The latter is actually what I would
> > expect from this testmode. Maybe that is something the testmode doesn't
> > cover yet?
> 
> The test mode causes us to not bring a Mozilla window to the foreground when
> any requested by us (such as via window.focus())
> 
> I think what you are asking for is for the window to be treated as if it was
> still focused when one switches to a window of another application. I
> suppose one quick way to do that is to just check if the testmode is enabled
> in nsWebShellWindow::WindowDeactivated and return early if so.

That is correct. Thank you. I will try to get this working.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I checked the existing code of the focus manager and also took the fix for bug 1334981 into account:

https://hg.mozilla.org/mozilla-central/rev/1f16a2ff45ab

And I think because `nsWebShellWindow::WindowDeactivated` calls `nsFocusManager::WindowLowered` a better place for the early skip should be here:

https://hg.mozilla.org/mozreview/gecko/file/default/dom/base/nsFocusManager.cpp#l764

Also this would keep the focumanager testmode checks in the focus manager only, without spreading it across components.

Comment 2

[Neil Deakin]

> And I think because `nsWebShellWindow::WindowDeactivated` calls
> `nsFocusManager::WindowLowered` a better place for the early skip should be
> here:
> 

No, since WindowLowered is called from several other places where you do not want to return early.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, I see. Now I wonder if it would be ok to expose the sTestMode (or should that even be isTestMode) as member of FocusManager. Otherwise I would have to read the test mode preference each time inside of `nsWebShellWindow::WindowDeactivated`. Other modules may also benefit from it.

Comment 4

[Neil Deakin]

You can just add a method to nsFocusManager.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So I tried that but due to my lack of knowledge in C++ and how everything is glued together the build jobs are failing with:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19cde48e7b2b8282e48dddfefd9380354147b807&selectedJob=198591067

> /builds/worker/workspace/build/src/xpfe/appshell/nsWebShellWindow.cpp:493:27: error: no member named 'IsTestMode' in 'nsCOMPtr<nsIFocusManager>'

Can I assume that this is because `nsWebShellWindow::WindowDeactivated` is using `do_GetService(FOCUSMANAGER_CONTRACTID)`, and that the method would have to be added to the file `nsIFocusManager.idl`? But this is a public interface, right? Do we want to expose this mode this way?

Or is there something else in this patch which is wrong?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, my failure. I accidentally used the `.` notation to access the method, while it is a pointer. The next try should hopefully succeed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=389d03de25aa5bdf1dbfed9a0c6268f53ea545ee&selectedJob=198597146

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, that wasn't the reason. Looks like the question still stands.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Neil, maybe you can help me with comment 5? Not sure if the IDL solution will work because when I try that I get the following error by the compiler:

> 4:44.84 In file included from /Volumes/data/code/gecko/obj/debug-full/xpfe/appshell/Unified_cpp_xpfe_appshell0.cpp:47:
>  4:44.84 /Volumes/data/code/gecko/xpfe/appshell/nsWebShellWindow.cpp:493:39: error: too few arguments to function call, single argument '_retval' was not specified
>  4:44.84   if (fm && window && !fm->IsTestMode())
>  4:44.84                        ~~~~~~~~~~~~~~ ^
>  4:44.84 /Volumes/data/code/gecko/obj/debug-full/dist/include/nsIFocusManager.h:66:25: note: 'IsTestMode' declared here
>  4:44.84   JS_HAZ_CAN_RUN_SCRIPT NS_IMETHOD IsTestMode(bool *_retval) = 0;
>  4:44.84                         ^
>  4:44.84 /Volumes/data/code/gecko/obj/debug-full/dist/include/nscore.h:133:29: note: expanded from macro 'NS_IMETHOD'
>  4:44.84 #define NS_IMETHOD          NS_IMETHOD_(nsresult)
>  4:44.85                             ^
>  4:44.85 /Volumes/data/code/gecko/obj/debug-full/dist/include/nscore.h:124:27: note: expanded from macro 'NS_IMETHOD_'
>  4:44.85 #define NS_IMETHOD_(type) virtual type

I'm not sure what the problem is here.

Thanks.

Comment 9

[Neil Deakin]

You should be able to just use:

nsFocusManager* fm = nsFocusManager::GetFocusManager();

instead of do_GetService. Then you can call methods of nsFocusManager directly rather than going through the interface.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That worked fine. But testing this change with a build from the following try push it doesn't work:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b17b54a4e1850deaa8bdbc09133e82c259f0f3f8&selectedJob=198653722

Not sure if I did something wrong or if that proposed change is not enough. I will add some more logs locally and hopefully figure it out myself.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Keep window active if focusmanager test mode is enabled

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Keep active state when Firefox is moved to background and focusmanager test mode is enabled

The test mode can be used to virtually give a Chrome window the
focus even with Firefox being in the background.

Currently when such a window has the focus and another application
is moved to the foreground, the active state is lost. This means that
for example Selenium tests which are run in parallel and using
different instances of Firefox will not receive the expected
"focus" and "blur" events.

This patch checks for the test mode and if enabled doesn't
lower the window, which will keep the focused state.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please note that with this fix in Firefox the following issue with Selenium will be fixed for geckodriver:
https://github.com/mozilla/geckodriver/issues/906

Here a try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a85184fd703bb0f69b9d31f191484ade36688f71

Comment 14

[Neil Deakin]

Comment on attachment 9008712 [details] [diff] [review]
Keep active state when Firefox is moved to background and focusmanager test mode is enabled

While this might create different behaviour, it should be ok for something that is only in focus test mode.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be7e57ba0c
Keep active state when Firefox is moved to background and focusmanager test mode is enabled. r=enndeakin

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c5be7e57ba0c

Comment 17

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance (I'm guessing from comment 13 you'd like this in 63)

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I would really like to have it in 63 but I worried about possible side-effects. But so far I haven't seen any regression which could have been caused by this patch.

Neil, do you see problems with uplifting this patch to beta?

Comment 19

[Neil Deakin]

Have you tried running various test suites with the preference enabled? (mochitest, tryserver, etc...)

I think this change should be ok since it is testing only.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Neil Deakin from comment #19)
> Have you tried running various test suites with the preference enabled?
> (mochitest, tryserver, etc...)

The patch has already been landed a while ago and is present on central since then. No test job so far was failing because of this change. For mochitests, reftests, and maybe others it won't even make a difference because the focus manager test mode is not in use at all yet.
 
> I think this change should be ok since it is testing only.

Thanks!

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 9008712 [details] [diff] [review]
Keep active state when Firefox is moved to background and focusmanager test mode is enabled

Approval Request Comment
[Feature/Bug causing the regression]: no regression
[User impact if declined]: code is only run in automation and when the preference `focusmanager.testmode` is flipped to true.
[Is this code covered by automated tests?]: yes, in Mn and Wd jobs
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: n/a
[String changes made/needed]: no

Comment 22

[Neil Deakin]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> The patch has already been landed a while ago and is present on central
> since then. No test job so far was failing because of this change. For
> mochitests, reftests, and maybe others it won't even make a difference
> because the focus manager test mode is not in use at all yet.

I realize that, but I'm asking if you have tried running the test suites with the test preference always enabled.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Neil Deakin from comment #22)
> I realize that, but I'm asking if you have tried running the test suites
> with the test preference always enabled.

Ok, so when I tried to run those tests without my patch on this bug (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=638877e421df199cec5a8bec7a858419204b57c1&selectedJob=197634051) lots of tests were crashing due to an IME problem. So I didn't try to run the tests again, because this underlying bug 1489029 hasn't been fixed yet.

Comment 24

[Pascal Chevrel:pascalc]

Neil, Henrik, are we good to go on this uplift request or should we wait more if there are still some issues to solve?

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

From my side it would be fine. No test job using the focusmanager testmode showed any failures regarding this change.

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9008712 [details] [diff] [review]
Keep active state when Firefox is moved to background and focusmanager test mode is enabled

Uplift accepted for 63 beta 11, thanks.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c51520b3d2f3