Closed
Bug 1124604
Opened 10 years ago
Closed 8 years ago
Add `focus` parameter to switch_to_window() to stay in compat with switch_to_frame()
Categories
(Testing :: Marionette Client and Harness, defect)
Testing
Marionette Client and Harness
Tracking
(firefox52 fixed, firefox53 fixed, firefox54 fixed)
RESOLVED
FIXED
mozilla54
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Keywords: pi-marionette-client, pi-marionette-server, Whiteboard: [spec][17/01])
Attachments
(3 files)
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•10 years ago
|
||
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•10 years ago
|
||
(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.
Assignee | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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: 10 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
(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)
Assignee | ||
Comment 7•10 years ago
|
||
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 → ---
Comment 8•10 years ago
|
||
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.
Updated•10 years ago
|
Keywords: ateam-marionette-client,
ateam-marionette-server
Assignee | ||
Comment 9•10 years ago
|
||
Sounds totally fine, yes.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 10•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8831292 -
Flags: review?(ato)
Attachment #8831293 -
Flags: review?(ato)
Comment 15•8 years ago
|
||
mozreview-review |
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 16•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 17•8 years ago
|
||
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.
Assignee | ||
Comment 18•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 19•8 years ago
|
||
"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 hidden (mozreview-request) |
Assignee | ||
Comment 21•8 years ago
|
||
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.
Assignee | ||
Comment 22•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•8 years ago
|
||
(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•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 28•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
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
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/148aa0e88c4a https://hg.mozilla.org/mozilla-central/rev/b415ed2abdf9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 32•8 years ago
|
||
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]
Comment 34•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7d5e5f56d5f https://hg.mozilla.org/releases/mozilla-aurora/rev/2253ac8c2947
Flags: in-testsuite+
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
Comment 35•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ef49463c6eb5 https://hg.mozilla.org/releases/mozilla-beta/rev/9bbd59201808
Whiteboard: [checkin-needed-beta]
Assignee | ||
Comment 36•8 years ago
|
||
Marking as spec work because parts of the changes are helpful for other blockers for bug 721859.
Whiteboard: [spec][17/01]
Updated•2 years ago
|
Product: Testing → Remote Protocol
Assignee | ||
Comment 37•2 years ago
|
||
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.
Description
•