Closed Bug 1398111 Opened 7 years ago Closed 6 years ago

Missing focus events for element interaction when Firefox is in background

Categories

(Remote Protocol :: Marionette, defect, P1)

57 Branch
defect

Tracking

(firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug, )

Details

Attachments

(7 files, 2 obsolete files)

Attached file Selenium testcase
https://github.com/mozilla/geckodriver/issues/906

Using geckodriver to run the following test fails because the focus event is missing when Firefox runs in background.

I ported the test to Marionette and by surprise it is working fine.

So something is related to how geckodriver starts Firefox, uses prefs, or whatever. At least it is not the missing "-foreground" argument for Firefox.
I thought this was what the "focusmanager.testmode" preference was
for.  As far as I can tell we are setting that in all the relevant
places (geckoinstance.py, prefs.rs, server.js).
I don't know what's causing this issue yet. Just something we do differently with Geckodriver.
Maybe we miss a command line parameter, but that is not that visible right now because we only print -marionette at the moment. Bug 1398057 will change that, so that all arguments are getting printed out.
Depends on: 1398057
(In reply to Henrik Skupin (:whimboo) from comment #4)

> Maybe we miss a command line parameter, but that is not that
> visible right now because we only print -marionette at the
> moment. Bug 1398057 will change that, so that all arguments are
> getting printed out.

If we were missing the -marionette flag the Marionette server would
never start and the tests would time out.  I don’t think this is
what’s causing the difference in behaviour.
Just a random thought: Could it be platform dependent? geckodriver
spawns the firefox subprocess, could this affect focus in some way
on, say, Windows?
(In reply to Andreas Tolfsen ‹:ato› from comment #5)
> > Maybe we miss a command line parameter, but that is not that
> > visible right now because we only print -marionette at the
> > moment. Bug 1398057 will change that, so that all arguments are
> > getting printed out.
> 
> If we were missing the -marionette flag the Marionette server would
> never start and the tests would time out.  I don’t think this is

Please read my last comment correctly. As I said we only log the -marionette argument at the moment but none others. Even not the -profile one. To start investigating if that is related in how the process gets started I only want to have bug 1398057 fixed, which makes it easier to spot differences.

I will be able to have a look soon on it.
(In reply to Henrik Skupin (:whimboo) from comment #7)

> Please read my last comment correctly. As I said we only log the
> -marionette argument at the moment but none others. Even not the
> -profile one. To start investigating if that is related in how the
> process gets started I only want to have bug 1398057 fixed, which
> makes it easier to spot differences.

First fixing bug 1398057 is complicating matters unnecessarily.
Which flags are being passed to Firefox can easily be checked with
standard Unix tooling:

Marionette:

> % ./mach marionette test --headless &
> [1] 13127
> % ps aux | grep firefox
> ato      15020 67.0  0.4 1698668 159736 pts/0  Rl   09:58   0:00 /home/ato/src/gecko/obj-x86_64-pc-linux-gnu/dist/bin/firefox -no-remote -marionette -profile /tmp/tmpT9MUEy.mozrunner

WPT:

> % ./mach wpt --test-type wdspec
> % ps aux | grep firefox
> ato      15222  103  0.7 1961460 243784 pts/1  Rl   10:00   0:02 /home/ato/src/gecko/obj-x86_64-pc-linux-gnu/dist/bin/firefox -marionette -profile /tmp/rust_mozprofile.umAgR6lDe4UQ

The only difference is -no-remote which I wouldn’t expect to make
a difference.

I have to reiterate that I think a comparison of preferences in
effect is a more actionable course of action here.

Apropos platform differences, did you confirm that it’s
reproducible on Linux/mac OS?  The reporter is using Windows, which
I’d expect would have different focus rules.
Priority: -- → P3
I suspect that this is a Marionette issue rather than a geckodriver one.
Component: geckodriver → Marionette
Depends on: 1466573
(In reply to Andreas Tolfsen 「:ato」 from comment #8)
> I have to reiterate that I think a comparison of preferences in
> effect is a more actionable course of action here.

I finally did that yesterday by nearly removing any preference in Marionette client, but it all works fine. I will go ahead and do some deeper tests today now that bug 1466658 will be fixed soon, which was a bit disturbing.

> Apropos platform differences, did you confirm that it’s
> reproducible on Linux/mac OS?  The reporter is using Windows, which

Yes, I see it clearly on Mac.

Also this problem is very annoying when running the wdspec tests locally. So I will have a look at it.
Assignee: nobody → hskupin
Component: Marionette → geckodriver
Priority: P3 → P2
So I removed all the preferences for Marionette except the "marionette.*" and "focusmanager.testmode" ones. When I run the test I always see the focus events fired whether Firefox is in foreground or background. Only changing "focusmanger.testmode" here makes a difference.

When running the same test via geckodriver and Selenium, "focusmanager.testmode" is marked as set to `true` in `about:config` but the focus events are not fired when Firefox runs in the background. 

I have seen that the focus manager has logging modules (Focus, and FocusNavigation) [1] but using those via `MOZ_LOG=sync,Focus,FocusNavigation` and `MOZ_LOG_FILE=path` nothing gets printed to the specified log file.

Neil, do you have any idea why the focusmanager doesn't always work? Are there any other dependencies than the preference? It's hard to believe that this is related in how an external application starts Firefox. Also would it be possible to use the logging module to figure out this problem?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#84-85
Flags: needinfo?(enndeakin)
Oh, the `MOZ_LOG` environment variable should actually contain `sync,Focus:5,FocusNavigation:5` to get it all logged.
As it can be seen in the logs only for the Marionette case the browser window got raised, and the input element received the focus:

> [40599:Main Thread]: D/Focus Window 0x119108020 Raised [Currently: 0x0 0x0]
> [40599:Main Thread]: D/Focus   Raised Window: 0x119108020 chrome://browser/content/browser.xul
> [40599:Main Thread]: D/Focus <<Focus begin>>
> [40599:Main Thread]: D/Focus Element input has been focused
> [40599:Main Thread]: D/Focus  from window
Does the test run entirely in the background? I see no attempt in the log to raise the window manually nor does the OS appear to do so. In both cases when the test preference is set, you should see 'Window XXX Raised' in the log.

Thus, the test is running as if the window is supposed to be in the background, so no focus events will be sent. If you want the test to run as simulated in the foreground you need to raise it first (window.focus() which will raise it in test mode, or some OS call which really raises it)
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #17)
> Does the test run entirely in the background? I see no attempt in the log to
> raise the window manually nor does the OS appear to do so. In both cases
> when the test preference is set, you should see 'Window XXX Raised' in the
> log.

For both Marionette and geckodriver I let the test start in the background. For none of them the Firefox window was in the foreground. So why does it work for Marionette?

> Thus, the test is running as if the window is supposed to be in the
> background, so no focus events will be sent. If you want the test to run as
> simulated in the foreground you need to raise it first (window.focus() which
> will raise it in test mode, or some OS call which really raises it)

I will check that tomorrow, and report back. Thanks.
(In reply to Neil Deakin from comment #17)
> Does the test run entirely in the background? I see no attempt in the log to
> raise the window manually nor does the OS appear to do so. In both cases
> when the test preference is set, you should see 'Window XXX Raised' in the
> log.

Would you mind to elaborate a bit more? I'm not sure I fully understand the requirements here to get the testing mode working.

The requirements we have are that tests are running in Firefox which is NOT the front-most application. Right now geckodriver starts Firefox without `-foreground`, which means that on MacOS Firefox initially opens in the background and stays there for the whole testrun. We are going to add this argument via bug 1466573. But even when we have this argument and I manually raise a different application as the top-most one, the test also fails.

When I use marionette-client it will start Firefox via -foreground by default. So focus events are raised when Firefox is the top-most application but also when I bring a different application to the front. When I remove the -foreground argument in mozrunner Marionette still works even when I do not manually bring it into foreground. The focus evens are always raised.

Please note that geckodriver utilizes the same command end-points of Marionette in Firefox as marionette-client does. So there is no difference in how those are called.

> Thus, the test is running as if the window is supposed to be in the
> background, so no focus events will be sent. If you want the test to run as
> simulated in the foreground you need to raise it first (window.focus() which
> will raise it in test mode, or some OS call which really raises it)

Ok, so you mean that when Marionette server in Firefox is initialized, it has to call `window.focus()` to get focus events raised even when the window is in the background? Is `window` in this case the chrome window, or a DOMWindow of the current tab?
Flags: needinfo?(enndeakin)
If you start Firefox normally in the background (in some manner), then you wouldn't expect to get any focus events.

But if you are trying to run Firefox in the background but simulate it being run in the foreground, then you need to simulate the window being brought to the foreground. In testmode, you should be able to do that with window.focus(). window is the top-level chrome window.
Flags: needinfo?(enndeakin)
Ah, ok. I think now I understood. And indeed when I add a call to `focus()` for the chrome window for example at the following location I actually get it working!

https://dxr.mozilla.org/mozilla-central/rev/752465b44c793318cef36df46ca5ff00c3d8854a/testing/marionette/driver.js#454

Neil, one last question. Given that someone could fiddle with Firefox and other applications while the tests are running, I assume the `focus()` call should probably happen for every command where we expect focus/blur events to get fired?
Status: NEW → ASSIGNED
Component: geckodriver → Marionette
Flags: needinfo?(enndeakin)
Priority: P2 → P1
I can answer myself. Yes it IS needed for each command. When I bring Firefox to the background between two commands the focus events start to fail again.
Flags: needinfo?(enndeakin)
Depends on: 1334981
Neil, I'm trying to figure out how best to open a window in the background, while Firefox itself is running in the background with the test mode enabled. At the moment we are using the following code:

> window.open("about:blank", null, "location=1,toolbar=1");
> window.focus();

It works fine when Firefox runs in the foreground, but when it's in the background the newly opened window is not getting moved to the background. This happens with a build which includes the patch on bug 1334981. Might this be a separate bug?
Flags: needinfo?(enndeakin)
(In reply to Henrik Skupin (:whimboo) from comment #22)
> I can answer myself. Yes it IS needed for each command. When I bring Firefox
> to the background between two commands the focus events start to fail again.

Thinking more about this, I wonder if it would be really a good idea to use it for each command. It would cause inappropriate focus changes, which would be fine for Selenium tests, but eg. for Marionette and Firefox UI tests, we also want to interact with Windows in the background. 

So my proposal would be to only focus for element interaction commands which are `click`, `clear`, and `send keys`. But that would open up the question what to do with `execute_script`. There are cases when tests interact with elements and focus events are wanted, but also those where we have to retrieve focus related data without changing the focus of the chrome window.

For `switchToWindow` we already have the `focus` argument present. Maybe we use that to set the focus on the chrome window if wanted, and skip all the other commands. That would mean that tests can fail when users bring Firefox to foreground and move it directly to the background while tests are running.

Given that this is not what we want, maybe this is a compromise? But note that when multiple tests run in parallel, and a new testrun gets started, the z-position of the last started Firefox will change, and the new one will be top-most (once the -foreground fix landed for OS X).

Andreas and David, what do you think?
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #26)

> (In reply to Henrik Skupin (:whimboo) from comment #22)
> 
> > I can answer myself. Yes it IS needed for each command. When I
> > bring Firefox to the background between two commands the focus
> > events start to fail again.
> 
> Thinking more about this, I wonder if it would be really a good
> idea to use it for each command. It would cause inappropriate
> focus changes, which would be fine for Selenium tests, but eg. for
> Marionette and Firefox UI tests, we also want to interact with
> Windows in the background.

Why would it be fine for the Firefox window to be brought in and out
of focus with Selenium tests but not with Marionette or Firefox UI
tests?

Focus changes would surely also have adverse effect on content
tests, and we should bear in mind that WebDriver users expect to be
able to run multiple instances (with separate profiles) in parallel.
Having them compete for focus attention is not ideal.

> So my proposal would be to only focus for element interaction
> commands which are `click`, `clear`, and `send keys`. But that
> would open up the question what to do with `execute_script`. There
> are cases when tests interact with elements and focus events are
> wanted, but also those where we have to retrieve focus related
> data without changing the focus of the chrome window.

Focussing the window before interaction is not a winning strategy.
The focus events for the document and any elements in the document
should be fired irregardless of window focus.

> For `switchToWindow` we already have the `focus` argument
> present. Maybe we use that to set the focus on the chrome window
> if wanted, and skip all the other commands. That would mean that
> tests can fail when users bring Firefox to foreground and move it
> directly to the background while tests are running.

Focussing on window switching is also wrong.  The browser should
_think_ that the window is in focus and despatch focus events as if
it was.  The effect we want to achieve is for window focus to not
have any effect on focus related events.

This is also called out in the WebDriver standard:

> In accordance with the focus section of the [HTML] specification,
> commands are unaffected by whether the operating system window has
> focus or not.

Bringing the window into focus when the browser starts is
probably fine.  We will do this with -foreground soon and calling
ChromeWindow.focus() on initialising Marionette should not have any
adverse effect, as it is the default behaviour of new application
windows on all systems to be brought into focus when they start.

What I don’t understand from your findings is whether it is
sufficient to focus the window once to being with to activate the
focus manager’s test mode, or if it needs to be done before every
interaction.  If the latter is the case, I would probably say the
test mode is not providing all the functionality that we need.

> Given that this is not what we want, maybe this is a
> compromise? But note that when multiple tests run in parallel, and
> a new testrun gets started, the z-position of the last started
> Firefox will change, and the new one will be top-most (once the
> -foreground fix landed for OS X).

I didn’t understand this?  What was the compromise?
Flags: needinfo?(ato)
I think it makes the most sense to discuss all this tomorrow, so we don't have to write long replies. I hope you will have the time tomorrow.
Blocks: 1419394
Given that Marionette and geckodriver start Firefox the exact same way, I can only assume that one of the preferences set by Marionette but not geckodriver could be the reason for this difference.

Given that preferences are set not only in Marionette but also in mozprofile, I will first get the latter via bug 905404 removed first.
Depends on: 905404
Ok, so this difference in behavior for the first start is not triggered by the differences in the preferences! It's actually the sequence of commands send. Compared to geckodriver which only starts a single session, Marionette is starting 3 of them! And when doing that, also runs `WebDriver:deleteSession`!

And here is the crux:

https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/testing/marionette/driver.js#2823-2831

This command sets the focus of the mainFrame to the top-level window.

But given that this isn't done at all in `WebDriver:newSession` the chrome window never has the focus, and as such we do not report the `focus` and `blur` events.

As such we should at least focusing the mainFrame when creating a new session.
No longer depends on: 905404
So what works is the following:

* Firefox is started in foreground
* Another application is brought into foreground
* Marionette calls `focus()` to focus the current chrome window
* The test is executed

As result the `focus` and `blur` events are received.

But what doesn't work is:

* Firefox is started in foreground
* Marionette calls `focus()` to focus the current chrome window
* Another application is brought into foreground
* The test is executed

Here the `focus` and `blur` events aren't received.

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?

Thanks!
Flags: needinfo?(enndeakin)
Depends on: 1335457
Attachment #8984045 - Attachment is obsolete: true
Attachment #8984046 - Attachment is obsolete: true
So the patch I'm going to attach here will make it possible for both Marionette and geckodriver driven tests to receive focus and blur events, when Firefox is in foreground or in background! But it will stop working once the Firefox window is physically brought into foreground and background again. In such a case we would have to focus the chrome window again, which is actually questionable. See my question in comment 33.

Also this will allow us to run Selenium tests in parallel, and all instances of Firefox show the `focus` and `blur` events!
When switching between chrome windows Marionette doesn't update
its references for the mainFrame (current chrome window) and
curFrame (current frame inside the chrome window). Instead it
gets currently only updated when a new chrome window opens.

It means that in most cases Marionette sets the focus on the
wrong chrome window and frame.

MozReview-Commit-ID: ChwzGguWurx
Attachment #9006936 - Flags: review?(ato)
To allow the focusmanager testmode to raise focus events even with
the Firefox window in the background, the current chrome window has
to be virtually brought into foreground. This should be at least
always done when creating a new session.
Attachment #9006937 - Flags: review?(ato)
With the fix on bug 1334981 the focus manager test mode has
no longer to be disabled for the unit tests.
Attachment #9006942 - Flags: review?(ato)
Comment on attachment 9006936 [details] [diff] [review]
[marionette] Update mainFrame and curFrame when switching chrome windows

Review of attachment 9006936 [details] [diff] [review]:
-----------------------------------------------------------------

Before you land this, please drop the MozReview-Commit-ID metadata
from the commit message.

::: testing/marionette/driver.js
@@ +1624,3 @@
>      this.curBrowser = this.browsers[winProperties.outerId];
> +    this.mainFrame = this.curBrowser.window;
> +    this.curFrame = null;

So this is obviously the right thing to do, but I worry that this
conflicts with https://bugzilla.mozilla.org/show_bug.cgi?id=1311041
and that it might affect some of the weirder focus-related tests
for Marionette (Mn and FxUI).

That in itself is of course not a reason _not_ to land the change,
so I’m going to give this r+ but with the caveat that you need to
be very careful to run all the tests, including FxUI.
Attachment #9006936 - Flags: review?(ato) → review+
Comment on attachment 9006937 [details] [diff] [review]
[marionette] Virtually set Firefox as top-most application in WebDriver:NewSession

Review of attachment 9006937 [details] [diff] [review]:
-----------------------------------------------------------------

This is going to cause a lot of extra work for me, but OK.
Attachment #9006937 - Flags: review?(ato) → review+
Attachment #9006942 - Flags: review?(ato) → review+
(In reply to Andreas Tolfsen ❲:ato❳ from comment #39)
> ::: testing/marionette/driver.js
> @@ +1624,3 @@
> >      this.curBrowser = this.browsers[winProperties.outerId];
> > +    this.mainFrame = this.curBrowser.window;
> > +    this.curFrame = null;
> 
> So this is obviously the right thing to do, but I worry that this
> conflicts with https://bugzilla.mozilla.org/show_bug.cgi?id=1311041

Why do you think that? I actually don't see a correlation between the window handle changes and the focusing state of the chrome window.

> and that it might affect some of the weirder focus-related tests
> for Marionette (Mn and FxUI).

Oh I missed to run the fxui tests in that try build. I triggered the test jobs now and so far it looks all promising.

(In reply to Andreas Tolfsen ❲:ato❳ from comment #40)
> This is going to cause a lot of extra work for me, but OK.

Why a lot of work? This is just a small movement of code only affecting new session and delete session.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb251478e1b4
[marionette] Update mainFrame and curFrame when switching chrome windows. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5fc5af2bc3
[marionette] Virtually set Firefox as top-most application in WebDriver:NewSession. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5200e74b118
[marionette] Never disable focus manager test mode for unit tests. r=ato
https://hg.mozilla.org/mozilla-central/rev/fb251478e1b4
https://hg.mozilla.org/mozilla-central/rev/1d5fc5af2bc3
https://hg.mozilla.org/mozilla-central/rev/f5200e74b118
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(In reply to Henrik Skupin (:whimboo) from comment #41)

> (In reply to Andreas Tolfsen ❲:ato❳ from comment #39)
> 
> > ::: testing/marionette/driver.js
> > @@ +1624,3 @@
> > >      this.curBrowser = this.browsers[winProperties.outerId];
> > > +    this.mainFrame = this.curBrowser.window;
> > > +    this.curFrame = null;
> > 
> > So this is obviously the right thing to do, but I worry that this
> > conflicts with https://bugzilla.mozilla.org/show_bug.cgi?id=1311041
> 
> Why do you think that? I actually don't see a correlation between the window
> handle changes and the focusing state of the chrome window.

Refactoring how ChromeWindows and content browsers are tracked
necessarily means changing how they are accessed, stored, and
referenced.  In particular it does away with all three of curBrowser,
mainFrame, and curFrame.

Since Marionette has pretty particular focussing rules, getting
these right has so far been the greatest of challenges.  Any focussing
related change then made on central has to be retrofitted into the
new system.  I haven’t looked at it in detail yet, but it could of
course be that this change actually make it _easier_ and that I can
remove some of the weird focus-related workaround I’ve so far had
to employ.
I see. Thanks for the details.
> 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.
Flags: needinfo?(enndeakin)
Depends on: 1489967
There are no known regressions from this patch. So I would like to request an uplift to mozilla-beta. Thanks.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
We won't be able to uplift the patch on bug 1334981 for esr60, which is the requirement for uplifting those commits. As such we cannot fix the focus issue in esr60.
Component: geckodriver → Marionette
Blocks: 1332064
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: