1124604 - Add `focus` parameter to switch_to_window() to stay in compat with switch_to_frame()

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

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

Attachment: test_example.py

switch_to_window() misses the focus parameter, similar what switch_to_frame() already has. 

A workaround for now would be to call the following code in the test because there is also no focus method available directly via the marionette object:

self.marionette.execute_script(""" window.focus(); """)

Attached you can find an example for a locally patched switch_to_window() method, which does a focus by default but let the client opt-out.

Comment 1

[Andreas Tolfsen ❲:ato❳]

I have a couple of questions:

1. Why doesn't switchToWindow focus the window it switches to?  I thought WebDriver was meant to do this.

2. If the focus argument is optional and can be replaced by calling executeScript("Window.focus()"), why does it need to be in Marionette?  Because this isn't compatible with WebDriver I'd rather go the opposite way and remove the argument for switchToFrame.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andreas Tolfsen (:ato) from comment #1)
> I have a couple of questions:
> 
> 1. Why doesn't switchToWindow focus the window it switches to?  I thought
> WebDriver was meant to do this.
> 
> 2. If the focus argument is optional and can be replaced by calling
> executeScript("Window.focus()"), why does it need to be in Marionette? 
> Because this isn't compatible with WebDriver I'd rather go the opposite way
> and remove the argument for switchToFrame.

I'm confused. 1. seems to say focusing when switching is compatible with WebDriver, but 2. seems to say it isn't.

The relationship between switch_to_window and switch_to_frame seems a question of consistency (or parity), not compatibility.

Comment 3

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

So I checked http://www.w3.org/TR/webdriver/#running-without-window-focus, and it looks like that a window doesn't really need the focus to be used. Any activity should be able to run in the background. So focus would be a nice to have.

There is one thing we still would have to check! All the popup menus eg. the location bar auto-complete. When those are getting tested and the window is not focused, we have seen failures in Mozmill. Also with the focus testing mode active.

Comment 4

[David Burns :automatedtester]

I think adding the focus kwarg here will actually be confusing since people will assume, just by it's name that it will bring windows to the front and give it OS level focus. Marionette can't do that (since we are not going to be writing platform specific code to do this which is just fraught with other "delights").

Comment 5

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

David, by issuing window.focus() the window will not only brought to foreground but also gets the OS level focus AFAIK. It will receive all the keyboard inputs. All necessary OS level code should be part of Firefox and we do not have to care about it at all.

What is in your opinion different from switch_to_frame(), and why one of the methods works while the other shouldn't?

Comment 6

[David Burns :automatedtester]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> David, by issuing window.focus() the window will not only brought to
> foreground but also gets the OS level focus AFAIK. 

This doesn't happen on OSX, can you show me details where this does work?

> What is in your opinion different from switch_to_frame(), and why one of the
> methods works while the other shouldn't?

focus=true is the default. For some reason b2g when using keyboard don't actually want to focus on it which is where it came from.

Comment 7

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

Over on bug 1128656 I have noticed that switch_to_window() always focuses (selects) the tab you want to switch to. This is bad because this blocks us from testing background tabs. With that having a focus parameter in switch_to_frame we could solve that.

Because of those problems I'm going to reopen this bug.

Comment 8

[David Burns :automatedtester]

If we add this the default will be to focus and then you have to explicitly pass in an argument that you dont want it to focus. Marionette is supposed to do user emulation, a user won't be interacting with background tabs except for using them to do something.

Comment 9

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

Sounds totally fine, yes.

Comment 10

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

So I was working on this bug today and added a couple more test cases. For one test case I always get failures and that is when opening a new window, and focusing again the former window. Then the get most recent window call of the window mediator always returns the new window, even if it's not the foreground window. It took me a while until I figured out that the issue is the "focusmanager.testmode: True" preference setting. 

I will file a bug early next week for that broken behavior, and maybe have to skip the affected tests for now.

Comment 11

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

Attachment: Bug 1124604 - Move code for getting the outer window id into its own method.

There are several places which make use of DOMWindowUtils to determine the
outer window id. Lets centralize it to a single method to avoid duplication.

Review commit: https://reviewboard.mozilla.org/r/107848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107848/

Comment 12

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

Attachment: Bug 1124604 - Add `focus` parameter to switch_to_window().

To be able to run tests for backgrounds tabs, and to stay in compatibility
with switch_to_frame(), switch_to_window() has to support the `focus` parameter.

Review commit: https://reviewboard.mozilla.org/r/107850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107850/

Comment 13

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

Comment on attachment 8831292 [details]
Bug 1124604 - Move code for getting the outer window id into its own method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107848/diff/1-2/

Comment 14

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

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107850/diff/1-2/

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8831292 [details]
Bug 1124604 - Move code for getting the outer window id into its own method.

https://reviewboard.mozilla.org/r/107848/#review109304

::: testing/marionette/driver.js:333
(Diff revision 2)
>      return this.curFrame;
>    }
>  };
>  
> +/**
> + * Gets the outer window id for the specified window.

s/id/ID/

::: testing/marionette/driver.js:338
(Diff revision 2)
> + * @return {string}
> + *     Returns the unique server-assigned ID of the window.

Slightly misleading now, let’s change this to just say “Unique window ID” or something.

::: testing/marionette/driver.js:341
(Diff revision 2)
> +GeckoDriver.prototype.getOuterWindowId = function (win) {
> +  let id = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindowUtils)
> +      .outerWindowID;
> +
> +  return id.toString();
> +};

Make this a top-level function not associated with `GeckoDriver.prototype`.  Otherwise good.

Comment 16

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

https://reviewboard.mozilla.org/r/107850/#review109306

::: testing/marionette/browser.js:42
(Diff revision 2)
> -  } else if (tab.hasOwnProperty("linkedBrowser")) {
> +  } else if ("linkedBrowser" in tab) {
>      // Firefox
>      return tab.linkedBrowser;
>  
>    } else {
> -      new UnsupportedOperationError("getBrowserForTab() not supported.");
> +      throw new UnsupportedOperationError("getBrowserForTab() not supported.");

Two many spaces (-;

And drop final stop in string.

::: testing/marionette/browser.js:68
(Diff revision 2)
> -  } else if (win.hasOwnProperty("gBrowser")) {
> +  } else if ("gBrowser" in win) {
>      // Firefox
>      return win.gBrowser;
>  
>    } else {
> -      new UnsupportedOperationError("getBrowserForTab() not supported.");
> +      throw new UnsupportedOperationError("getBrowserForTab() not supported.");

Indentation, and drop final stop in string.

::: testing/marionette/browser.js:279
(Diff revision 2)
> -      } else {
> +        } else if ("selectedTab" in this.tabBrowser) {
> -        // Firefox
> +          // Firefox
> -        this.tabBrowser.selectedTab = this.tab;
> +          this.tabBrowser.selectedTab = this.tab;
> +
> +        } else {
> +          throw new UnsupportedOperationError("switchToTab() not supported.");

Drop final stop.

::: testing/marionette/client/marionette_driver/marionette.py:1569
(Diff revision 2)
>          directed at the new window.
>  
>          :param window_id: The id or name of the window to switch to.
> +
> +        :param focus: A boolean value which determins whether to focus
> +            the window that we just switched to.

s/that we just switched to//

::: testing/marionette/driver.js:1211
(Diff revision 2)
> + * @param {boolean} focus
> + *      A boolean value which determins whether to focus
> + *      the window that we just switched to. Defaults to true.

* Mark as optional (`=`).
* determins → determines
* s/that we just switched to//

::: testing/marionette/harness/marionette_harness/tests/unit/test_switch_window_chrome.py:23
(Diff revision 2)
> +    def tearDown(self):
> +        self.close_all_windows()
> +
> +        super(TestSwitchWindowChrome, self).tearDown()

Switch back to content here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_switch_window_chrome.py:28
(Diff revision 2)
> +    def open_background_window(self):
> +        with self.marionette.using_context("chrome"):
> +            self.marionette.execute_script("""
> +              window.open("about:blank", null, "location=1,toolbar=1");
> +              window.focus();
> +            """)

Is it a _background_ window when the window is focussed?

::: testing/marionette/harness/marionette_harness/tests/unit/test_switch_window_chrome.py:42
(Diff revision 2)
> +            self.marionette.navigate(self.test_page)
> +            link = self.marionette.find_element(By.ID, "new-window")
> +            link.click()
> +
> +    def test_switch_tabs_for_new_background_window_without_focus_change(self):
> +        # Bug 1334981 - with testmode enabled getMostRecentWindow detects the wrong window

Good catch!

::: testing/marionette/harness/marionette_harness/tests/unit/test_switch_window_content.py:16
(Diff revision 2)
>  
> -    def testJSWindowCreationAndSwitching(self):
> -        test_html = self.marionette.absolute_url("test_windows.html")
> +    def setUp(self):
> +        super(TestSwitchToWindowContent, self).setUp()
> -        self.marionette.navigate(test_html)
>  
> -        def open_window_with_link():
> +        if self.marionette.session_capabilities['platformName'] == 'darwin':

Double quotes.

Comment 17

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

My last patch actually regressed our tests and that because of the addition of the throw statement for getBrowserForTab and getTabBrowser. I think I will revert this for now, return null and we can change it later.

Comment 18

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

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

https://reviewboard.mozilla.org/r/107850/#review109306

> Two many spaces (-;
> 
> And drop final stop in string.

We actually have to drop this line due to mass-bustage when throwing an exception here. For now we should revert to return null.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b94f99fa3e14&selectedJob=73047558&filter-tier=1&filter-tier=2&filter-tier=3

> Indentation, and drop final stop in string.

Same as above.

> Switch back to content here.

What do you mean by that? None of our tests which run code in chrome switch back to content in tearDown.

> Is it a _background_ window when the window is focussed?

window.open() returns the newly created window, which we do not use here. Instead we focus the original window, which moves the new window to the background.

Comment 19

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

"resource:///modules/RecentWindow.jsm" as used in my patch is not available for Fennec. And it's indeed not necessary because there is only a single window. I have to add an extra check to not cause a new regression for Marionette tests on Android.

Comment 20

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

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107850/diff/2-3/

Comment 21

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

The latest try build still shows test failures with focusing the right window for the background window test:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb631358d735&filter-tier=1&filter-tier=2&filter-tier=3

The assertion for the correct index fails right after opening the background window. As it looks like the window is not moved to the background. This also happens with focusmanager.testingmode turned off.

Lets see if I can figure out the problem on my Linux machine. Otherwise we should skip this test for now.

Comment 22

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

I'm not sure what's going on with this patch on my other machine or even on a one-click loaner. In both cases when the patch is applied the new tests test_switch_window_*.py are missing. The try build still showed those running and a failure in the chrome one.

Comment 23

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

Comment on attachment 8831292 [details]
Bug 1124604 - Move code for getting the outer window id into its own method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107848/diff/2-3/

Comment 24

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

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107850/diff/3-4/

Comment 25

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

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107850/diff/4-5/

Comment 26

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> I'm not sure what's going on with this patch on my other machine or even on
> a one-click loaner. In both cases when the patch is applied the new tests
> test_switch_window_*.py are missing. The try build still showed those
> running and a failure in the chrome one.

I’m OK with switching the problematic test off for Linux for the moment.  We can try to fix this later.

Comment 27

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

https://reviewboard.mozilla.org/r/107850/#review109306

> What do you mean by that? None of our tests which run code in chrome switch back to content in tearDown.

That will presumably start the text test in chrome context?  That would be undesirable.

Comment 28

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

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

https://reviewboard.mozilla.org/r/107850/#review109306

> That will presumably start the text test in chrome context?  That would be undesirable.

No, we always reset the context between each test as the following example also proves:

TEST-START | test_minimized.py TestMinimizedTestCase.test1
before setting chrome: content
after setting chrome: chrome
TEST-PASS | test_minimized.py TestMinimizedTestCase.test1 | took 184ms
TEST-START | test_minimized.py TestMinimizedTestCase.test2
next test: content
TEST-PASS | test_minimized.py TestMinimizedTestCase.test2 | took 157ms

Comment 29

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8831293 [details]
Bug 1124604 - Add `focus` parameter to switch_to_window().

https://reviewboard.mozilla.org/r/107850/#review109306

> No, we always reset the context between each test as the following example also proves:
> 
> TEST-START | test_minimized.py TestMinimizedTestCase.test1
> before setting chrome: content
> after setting chrome: chrome
> TEST-PASS | test_minimized.py TestMinimizedTestCase.test1 | took 184ms
> TEST-START | test_minimized.py TestMinimizedTestCase.test2
> next test: content
> TEST-PASS | test_minimized.py TestMinimizedTestCase.test2 | took 157ms

OK.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/148aa0e88c4a
Move code for getting the outer window id into its own method. r=ato
https://hg.mozilla.org/integration/autoland/rev/b415ed2abdf9
Add `focus` parameter to switch_to_window(). r=ato

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/148aa0e88c4a
https://hg.mozilla.org/mozilla-central/rev/b415ed2abdf9

Comment 32

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

test-only patch which we would like to see uplifted to aurora and beta. Thanks.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7d5e5f56d5f
https://hg.mozilla.org/releases/mozilla-aurora/rev/2253ac8c2947

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ef49463c6eb5
https://hg.mozilla.org/releases/mozilla-beta/rev/9bbd59201808

Comment 36

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

Marking as spec work because parts of the changes are helpful for other blockers for bug 721859.

Comment 37

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

Moving bugs for Marionette client due to component changes.