Closed Bug 1124604 Opened 9 years ago Closed 7 years ago

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

Categories

(Testing :: Marionette Client and Harness, defect)

Product:
Testing
Component:
Marionette Client and Harness
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed, firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: pi-marionette-client, pi-marionette-server, Whiteboard: [spec][17/01])

Attachments

(3 files)

test_example.py
4.66 KB, text/x-python
Details
Bug 1124604 - Move code for getting the outer window id into its own method.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1124604 - Add `focus` parameter to switch_to_window().
59 bytes, text/x-review-board-request
ato
: review+
Details
Attached file 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.
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.
(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.
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.
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").
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
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?
Flags: needinfo?(dburns)
(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.
Flags: needinfo?(dburns)
Blocks: 1128656
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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.
Sounds totally fine, yes.
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
Depends on: 1323185
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.
Depends on: 1334981
Depends on: 1335085
Attachment #8831292 - Flags: review?(ato)
Attachment #8831293 - Flags: review?(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.
Attachment #8831292 - Flags: review?(ato) → review+
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.
Attachment #8831293 - Flags: review?(ato) → review+
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 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.
"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.
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.
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.
Blocks: 1335457
(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 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 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 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.
Pushed by hskupin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/148aa0e88c4a
https://hg.mozilla.org/mozilla-central/rev/b415ed2abdf9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
test-only patch which we would like to see uplifted to aurora and beta. Thanks.
status-firefox52: --- → affected
status-firefox53: --- → affected
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7d5e5f56d5f
https://hg.mozilla.org/releases/mozilla-aurora/rev/2253ac8c2947
status-firefox53: affected → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
Marking as spec work because parts of the changes are helpful for other blockers for bug 721859.
Whiteboard: [spec][17/01]
Blocks: 1372434
Product: Testing → Remote Protocol

Moving bugs for Marionette client due to component changes.

Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: