Closed Bug 1466573 Opened 6 years ago Closed 6 years ago

geckodriver should use -foreground to start Firefox as active application on Mac OS

Categories

(Testing :: geckodriver, enhancement, P1)

Product:
Testing
Component:
geckodriver
Platform:
x86_64
macOS
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: ato)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1466573 - Write profile prefs before constructing command.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1466573 - Avoid resetting stdout + stderr.
59 bytes, text/x-review-board-request
Details
Bug 1466573 - Start Firefox with -foreground and -no-remote.
59 bytes, text/x-review-board-request
jgraham
: review+
Details 
Compared to Windows and Linux where Firefox gets started as active application in the foreground, on Mac OS Firefox gets launched in the background. As such the `focus` and `blur` events are not getting fired for element interactions.

Workaround for now is to manually (or with some automation tools) bring Firefox into foreground.

We should use `-foreground` by default to start Firefox.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Assignee: nobody → ato
Status: NEW → ASSIGNED
As I already mentioned to Andreas on IRC there is bug 625614 which I filed years and years ago for our Mozmill tests. It might not affect geckodriver given that we don't want to show the initial start pages, but maybe it cause some other side-effects if the -foreground argument isn't added as the very last argument in the list.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> As I already mentioned to Andreas on IRC there is bug 625614 which I filed
> years and years ago for our Mozmill tests. It might not affect geckodriver
> given that we don't want to show the initial start pages, but maybe it cause
> some other side-effects if the -foreground argument isn't added as the very
> last argument in the list.

I can't say this is a problem as we were doing this for Selenium for years but the start tab always pointed to about:blank
I’ve submitted some patches for this.  The first few fixes a couple
of bugs I found along the way.

One can imagine other ways of approaching the latter two patches,
but this is one approach using predicates.  This avoids having to
duplicate the unwrapping and Option-checking of parse_arg_name()
without reparsing the arguments on each iteration.

However, I’m not too fond of the eq_ignore_ascii_case repetition,
and storing everything as functions in the firefox_args::predicates::*
module does seem a bit cumbersome.  We could avoid this if we
introduced a type that wrapped the argument and implemented a
basename equality method:

> struct Argument(String);
> 
> impl Argument (
>     fn match(&self, basename: &str) {
>         self.0.eq_ignore_ascii_case(basename)
>     }
> }
> 
> if !self.args.iter().any(|x| basename_match(x, |arg| arg.eq("no-remote"))) {
>     …
> }
Comment on attachment 8983500 [details]
Bug 1466573 - Write profile prefs before constructing command.

https://reviewboard.mozilla.org/r/249350/#review255524

::: testing/mozbase/rust/mozrunner/src/runner.rs:255
(Diff revision 1)
>          self.stderr = Some(stderr.into());
>          self
>      }
>  
>      fn start(mut self) -> Result<FirefoxProcess, RunnerError> {
> +        self.profile.user_prefs()?.write()?;

Maybe not related but how does geckodriver currently handle multiple invocations of `start()`? If that is allowed by the API we would write the prefs multiple times. Maybe it's better to move this out closer to the code which actually creates the profile.
Attachment #8983500 - Flags: review?(hskupin) → review+
Attachment #8983501 - Flags: review?(hskupin) → review?(james)
Comment on attachment 8983501 [details]
Bug 1466573 - Avoid resetting stdout + stderr.

https://reviewboard.mozilla.org/r/249352/#review255762
Attachment #8983501 - Flags: review?(james) → review+
Comment on attachment 8983503 [details]
Bug 1466573 - Start Firefox with -foreground and -no-remote.

https://reviewboard.mozilla.org/r/249356/#review255766

::: testing/mozbase/rust/mozrunner/src/runner.rs:269
(Diff revision 1)
>              .envs(&self.envs)
>              .stdout(stdout)
>              .stderr(stderr);
>  
> -        if !self.args.iter().any(|arg| basename_match(arg, predicates::profile)) {
> +        // Ensure application window gets focus, which is not the default on macOS.
> +        if !self.args

I mean this is OK, and we can probably just land it as-is if you like, but I wonder if we should do something like

```
let mut seen_profile = false;
let mut seen_foreground = false;
let mut seen_no_remote = false;
for arg in self.args.iter() {
   match arg.into() {
      ArgType::Profile(_) || ArgType::ProfileManager(_) => seen_profile = true;
      ArgType::Foreground(_) => seen_foreground = true;
      ArgType::NoRemote => seen_no_remote = true;
      _ => {}
   }
}
if !seen_profile {
    cmd.arg("-profile").arg(&self.profile_path);
}
[…]
```
so that we only iterate over the args once.
Comment on attachment 8983502 [details]
Bug 1466573 - Convert Firefox args matching to predicate.

https://reviewboard.mozilla.org/r/249354/#review255954
Attachment #8983502 - Flags: review?(hskupin)
Comment on attachment 8983503 [details]
Bug 1466573 - Start Firefox with -foreground and -no-remote.

https://reviewboard.mozilla.org/r/249356/#review255956
Attachment #8983503 - Flags: review?(hskupin)
Attachment #8983502 - Attachment is obsolete: true
Attachment #8983502 - Flags: review?(james)
Attachment #8983502 - Flags: review?(hskupin)
Comment on attachment 8983500 [details]
Bug 1466573 - Write profile prefs before constructing command.

https://reviewboard.mozilla.org/r/249350/#review255524

> Maybe not related but how does geckodriver currently handle multiple invocations of `start()`? If that is allowed by the API we would write the prefs multiple times. Maybe it's better to move this out closer to the code which actually creates the profile.

As with std::process::Command you can probably call any of the
methods after the Child has been created.
Comment on attachment 8983503 [details]
Bug 1466573 - Start Firefox with -foreground and -no-remote.

https://reviewboard.mozilla.org/r/249356/#review255766

> I mean this is OK, and we can probably just land it as-is if you like, but I wonder if we should do something like
> 
> ```
> let mut seen_profile = false;
> let mut seen_foreground = false;
> let mut seen_no_remote = false;
> for arg in self.args.iter() {
>    match arg.into() {
>       ArgType::Profile(_) || ArgType::ProfileManager(_) => seen_profile = true;
>       ArgType::Foreground(_) => seen_foreground = true;
>       ArgType::NoRemote => seen_no_remote = true;
>       _ => {}
>    }
> }
> if !seen_profile {
>     cmd.arg("-profile").arg(&self.profile_path);
> }
> […]
> ```
> so that we only iterate over the args once.

I like this approach more and have updated the patch.

I’m sure there are some clever tricks that can be used in the test
for automatically coercing &str or String to OsString, but at least
it compiles and runs.  I’ve tested that it works locally.
Attachment #8983501 - Flags: review?(hskupin)
Comment on attachment 8983503 [details]
Bug 1466573 - Start Firefox with -foreground and -no-remote.

https://reviewboard.mozilla.org/r/249356/#review256952

I will let James finish this review.
Attachment #8983503 - Flags: review?(hskupin)
Priority: -- → P1
Comment on attachment 8983503 [details]
Bug 1466573 - Start Firefox with -foreground and -no-remote.

https://reviewboard.mozilla.org/r/249356/#review257170

::: testing/mozbase/rust/mozrunner/src/firefox_args.rs:85
(Diff revision 4)
> +
> +    /// All other arguments.
> +    Other(String),
> +
> +    /// Not an argument.
> +    None,

This could be `Value(String)` since it presumably happens when we get something that's the value for an argument e.g. `--profile path` Although having said that, it would be ideal to make this also work for `--profile=path`, which doesn't quite work in the current design; you would need something like `type Arg = (Option(Arg), Option(String)` or something, and we don't actually have a usecase for this, so probably not worth doing in this PR, but maybe a good idea for the future.
Attachment #8983503 - Flags: review?(james) → review+
Comment on attachment 8983503 [details]
Bug 1466573 - Start Firefox with -foreground and -no-remote.

https://reviewboard.mozilla.org/r/249356/#review257170

> This could be `Value(String)` since it presumably happens when we get something that's the value for an argument e.g. `--profile path` Although having said that, it would be ideal to make this also work for `--profile=path`, which doesn't quite work in the current design; you would need something like `type Arg = (Option(Arg), Option(String)` or something, and we don't actually have a usecase for this, so probably not worth doing in this PR, but maybe a good idea for the future.

Note that the value here isn’t the value passed to the argument,
but the name of the unrecognised argument.

parse_arg_name() returns an Option<String> which means we may end
up with something “other” which I’ve here represented with None.
None is perhaps the wrong connotation, but I haven’t read
parse_arg_name() in detail to find out _when_ it returns None:
https://searchfox.org/mozilla-central/rev/40577076a6e7a14d143725d0288353c250ea2b16/testing/mozbase/rust/mozrunner/src/runner.rs#279-318
Flags: needinfo?(james)
So at the moment `parse_arg_name` basically does "check if this string starts with a - or (on Windows) a / and return all the charaters before the first space or =, or None if there aren't any such characters or the check fails". A more general argument parser would want to return both that first part and all the subseqent characters that we're discarding. But that isn't needed for any feature here, so just land the patch as it is.
Flags: needinfo?(james)
(In reply to Andreas Tolfsen 「:ato」 from comment #18)  

> Comment on attachment 8983500 [details] Bug 1466573 - Write profile prefs
> before constructing command.
> 
> https://reviewboard.mozilla.org/r/249350/#review255524
> 
> > Maybe not related but how does geckodriver currently handle
> > multiple invocations of `start()`? If that is allowed by the
> > API we would write the prefs multiple times. Maybe it's better
> > to move this out closer to the code which actually creates the
> > profile.
> 
> As with std::process::Command you can probably call any of the
> methods after the Child has been created.

Does this resolve your issue?
Flags: needinfo?(hskupin)
Attachment #8983503 - Flags: review?(hskupin)
Comment on attachment 8983500 [details]
Bug 1466573 - Write profile prefs before constructing command.

https://reviewboard.mozilla.org/r/249350/#review255524

> As with std::process::Command you can probably call any of the
> methods after the Child has been created.

Maybe I was not clear. Here an example:

```
start()
stop()
start()
```

The 2nd call to `start()` will write the same preferences again into the `user.js` file. I don't think this is something we want to have. Feel free to move this to a different bug given that your patch didn't make any modification for that.
Comment on attachment 8983500 [details]
Bug 1466573 - Write profile prefs before constructing command.

https://reviewboard.mozilla.org/r/249350/#review255524

> Maybe I was not clear. Here an example:
> 
> ```
> start()
> stop()
> start()
> ```
> 
> The 2nd call to `start()` will write the same preferences again into the `user.js` file. I don't think this is something we want to have. Feel free to move this to a different bug given that your patch didn't make any modification for that.

I would say it is expected behaviour if you call start() twice.
But you probably don’t want to do that.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cfc5d5a96332
Write profile prefs before constructing command. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/d264fabbfb13
Avoid resetting stdout + stderr. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/c5c9371e1527
Start Firefox with -foreground and -no-remote. r=jgraham
https://hg.mozilla.org/mozilla-central/rev/cfc5d5a96332
https://hg.mozilla.org/mozilla-central/rev/d264fabbfb13
https://hg.mozilla.org/mozilla-central/rev/c5c9371e1527
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: needinfo?(hskupin)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: