Closed Bug 1489967 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XUL, defect, P2)

Product:
Core
Component:
XUL
Version:
62 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

Keep active state when Firefox is moved to background and focusmanager test mode is enabled
2.63 KB, patch
enndeakin
: review+
pascalc
: approval-mozilla-beta+
Details | Diff | Splinter Review 
(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.
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.
> 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.
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.
Flags: needinfo?(enndeakin)
You can just add a method to nsFocusManager.
Flags: needinfo?(enndeakin)
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?
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
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P2
Ok, that wasn't the reason. Looks like the question still stands.
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.
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
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.
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.
Attachment #9008711 - Attachment is obsolete: true
Attachment #9008712 - Flags: review?(enndeakin)
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.
Attachment #9008712 - Flags: review?(enndeakin) → review+
Pushed by hskupin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c5be7e57ba0c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Please request uplift to beta when you get a chance (I'm guessing from comment 13 you'd like this in 63)
Flags: needinfo?(hskupin)
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?
Flags: needinfo?(hskupin) → needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
(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 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
Attachment #9008712 - Flags: approval-mozilla-beta?
(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.
(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.
Neil, Henrik, are we good to go on this uplift request or should we wait more if there are still some issues to solve?
Flags: needinfo?(hskupin)
Flags: needinfo?(enndeakin)
From my side it would be fine. No test job using the focusmanager testmode showed any failures regarding this change.
Flags: needinfo?(hskupin)
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.
Attachment #9008712 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(enndeakin)
Whiteboard: [checkin-needed-beta]
See Also: → 704583
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: