Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[js] switchTo().window() should use a 'handle' parameter rather than name #2718

Merged
merged 3 commits into from
Sep 7, 2016

Conversation

Standard8
Copy link
Contributor

@Standard8 Standard8 commented Sep 5, 2016

This is to match the W3C Webdriver spec, and fixes switchTo().window() for use with geckodriver.

Note that the python driver already uses handle rather than name

I've tried running the tests locally, but either they are taking a very long time, or there's something wrong with them on my machine.

* @param {number} opt_searchAttempt Which attempt this is at finding the
* window to switch to.
*/
nsCommandProcessor.prototype.switchToWindow = function(response, parameters,
opt_searchAttempt) {
var lookFor = parameters.name;
var lookFor = parameters.handle;
Copy link
Member

@andreastt andreastt Sep 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/javascript/firefox-driver is the Selenium implementation of a WebDriver for Firefox. Since it generally isn’t W3C compatible, it shouldn’t change.

We can just drop this change.

@Standard8
Copy link
Contributor Author

@andreastt Thank you for the hints, I've undone the webdriver for Firefox change, and added the W3C compliance detection.

I also worked out how to run the nodejs tests, and I've added a couple of test cases to test that the parameter and w3c detection works correctly.

For reference, cd into javascript/node/selenium-webdriver, npm install then npm test. Need geckodriver in $PATH.

@andreastt
Copy link
Member

r? @jleyba

@@ -1654,9 +1654,10 @@ class TargetLocator {
* when the driver has changed focus to the specified window.
*/
window(nameOrHandle) {
let paramName = this.driver_.getExecutor().w3c ? 'handle' : 'name';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just send the parameter twice than break encapsulation here. There's already precedence with webelement IDs

@jleyba
Copy link
Contributor

jleyba commented Sep 6, 2016

Looks like we're missing e2e coverage for window switching in

https://github.com/SeleniumHQ/selenium/blob/master/javascript/node/selenium-webdriver/test/window_test.js

Mind adding a test or 2?

@Standard8
Copy link
Contributor Author

Mind adding a test or 2?

@jleyba It took a bit of work, but I've now added a test, it seems to work fine.

@jleyba jleyba merged commit be7ff6f into SeleniumHQ:master Sep 7, 2016
@Standard8 Standard8 deleted the fix-javascript-switchtowindow branch September 8, 2016 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants