fix(webdriverio): better handle window switching in webdriver classic#14025
fix(webdriverio): better handle window switching in webdriver classic#14025christian-bromann merged 7 commits intowebdriverio:mainfrom
Conversation
| }) | ||
|
|
||
| describe('handle windows in webdriver classic', () => { | ||
| it('should handle window closing and switching in WebDriver Classic mode', async () => { |
There was a problem hiding this comment.
This doesn't test the newly added functionality. Can you add a test that verifies that we now automatically switch when closing a window?
There was a problem hiding this comment.
Should this test be good enough?
describe('handle windows in webdriver classic', () => {
it('should handle window closing and switching in WebDriver Classic mode', async () => {
await browser.url('https://the-internet.herokuapp.com/iframe')
const elementalSeleniumLink = await $('/html/body/div[3]/div/div/a')
await elementalSeleniumLink.waitForDisplayed()
await elementalSeleniumLink.click()
await browser.waitUntil(async () => (await browser.getWindowHandles()).length === 2)
await browser.switchWindow('https://elementalselenium.com/')
await $('#__docusaurus_skipToContent_fallback').waitForDisplayed()
await browser.closeWindow()
const remainingWindows = await browser.getWindowHandles()
expect(remainingWindows.length).toBe(1)
await browser.switchWindow(remainingWindows[0])
// Verify we're on the original window
expect(await $('.example h3').getText()).toBe('An iFrame containing the TinyMCE WYSIWYG Editor')
})
})I have tested this PR with examples mentioned here and here and both of these are working after the changes.
| if (!this.isBidi && (!currentWindow || !tabs.includes(currentWindow))) { | ||
| // Switch to the first available window before proceeding | ||
| await this.switchToWindow(tabs[0]) | ||
| contextManager.setCurrentContext(tabs[0]) |
There was a problem hiding this comment.
@christian-bromann correct me if I am wrong but the context manager is only used in BiDi right?
eslint-plugin-wdio
@wdio/allure-reporter
@wdio/browser-runner
@wdio/appium-service
@wdio/browserstack-service
@wdio/cli
@wdio/concise-reporter
@wdio/config
@wdio/cucumber-framework
@wdio/dot-reporter
@wdio/firefox-profile-service
@wdio/globals
@wdio/jasmine-framework
@wdio/json-reporter
@wdio/junit-reporter
@wdio/lighthouse-service
@wdio/local-runner
@wdio/logger
@wdio/mocha-framework
@wdio/protocols
@wdio/repl
@wdio/reporter
@wdio/runner
@wdio/sauce-service
@wdio/shared-store-service
@wdio/smoke-test-cjs-service
@wdio/smoke-test-reporter
@wdio/smoke-test-service
@wdio/spec-reporter
@wdio/static-server-service
@wdio/sumologic-reporter
@wdio/testingbot-service
@wdio/types
@wdio/utils
@wdio/webdriver-mock-service
webdriver
webdriverio
commit: |
|
Thanks for fixing this issue. This has been a blocker for us to move to wdio9.5 version with 'wdio:enforceWebDriverClassic': true |
christian-bromann
left a comment
There was a problem hiding this comment.
Thanks for raising the PR @navin772 !
I tried to run your reproduction case:
await browser.url('https://the-internet.herokuapp.com/iframe')
const elementalSeleniumLink = await browser.$('/html/body/div[3]/div/div/a')
await elementalSeleniumLink.waitForDisplayed()
await elementalSeleniumLink.click()
await browser.waitUntil(async () => (await browser.getWindowHandles()).length === 2)
await browser.switchWindow('https://elementalselenium.com/')
await browser.$('#__docusaurus_skipToContent_fallback').waitForDisplayed()
await browser.closeWindow()
const remainingWindows = await browser.getWindowHandles()
console.log('remainingWindows.length', remainingWindows.length)
await browser.switchWindow(remainingWindows[0])
// Verify we're on the original window
console.log(await browser.$('.example h3').getText())on the current main (without your patch) and the script executed just fine with the following output:
remainingWindows.length 1
An iFrame containing the TinyMCE WYSIWYG Editor
So it seems like the patch you provided doesn't really catch the error you are trying to address. Can you provide an example for me to review this?
| } | ||
|
|
||
| const currentWindow = await this.getWindowHandle() | ||
| const currentWindow = await this.getWindowHandle().catch(() => null) |
There was a problem hiding this comment.
In what cases do we expect the command not to be available?
There was a problem hiding this comment.
Maybe, when there is no active window or the context is not initialized properly. Although, I think BiDi is somehow handling these cases and thus we are seeing these issues in classic mode only.
Hi @christian-bromann, thanks for reviewing, can you try this test as mentioned in #13882: describe('open multiple tabs and closeWindow then switchWindow', function () {
it('open multiple tabs and closeWindow then switchWindow', async function () {
await browser.url('https://www.google.com/');
await browser.newWindow('https://webdriver.io/docs/api/');
await browser.newWindow('https://github.com/');
await browser.pause(1000);
await browser.closeWindow();
await browser.switchWindow('google');
/*
//getting current handle also triggers the error
let currentHandle = await browser.getWindowHandle();
*/
});
}); |
|
@navin772 that is the behavior you are seeing and what is the expected behavior? |
|
@christian-bromann The behaviour we are currently seeing is that after we call The fix catches this |
|
@navin772 what capabilities are you using? |
|
@christian-bromann the caps: |
christian-bromann
left a comment
There was a problem hiding this comment.
I took another look at this matter I propose a different solution. We introduced the context manager to deal with window changes and mitigating issues as seen in the reported bug. This change will provide the same mitigation for both modes: classic and bidi:
diff --git a/packages/webdriverio/src/context.ts b/packages/webdriverio/src/context.ts
index 176f710af..7b2bed7a8 100644
--- a/packages/webdriverio/src/context.ts
+++ b/packages/webdriverio/src/context.ts
@@ -36,6 +36,21 @@ export class ContextManager {
isNativeContext: this.#isNativeContext
})
+ /**
+ * Listens for the 'closeWindow' browser command to handle context changes.
+ * (classic + bidi)
+ */
+ this.#browser.on('result', (event) => {
+ /**
+ * the `closeWindow` command returns:
+ * > the result of running the remote end steps for the Get Window Handles command, with session, URL variables and parameters.
+ */
+ if (event.command === 'closeWindow') {
+ this.#currentContext = (event.result as { value: string[] }).value[0]
+ return this.#browser.switchToWindow(this.#currentContext)
+ }
+ })
+
if (!this.#isEnabled()) {
return
}
@@ -77,15 +92,6 @@ export class ContextManager {
* Listens for the 'closeWindow' browser command to handle context changes.
*/
this.#browser.on('result', (event) => {
- /**
- * the `closeWindow` command returns:
- * > the result of running the remote end steps for the Get Window Handles command, with session, URL variables and parameters.
- */
- if (event.command === 'closeWindow') {
- this.#currentContext = (event.result as { value: string[] }).value[0]
- return this.#browser.switchToWindow(this.#currentContext)
- }
-
if (this.#browser.isMobile) {
if (event.command === 'getContext') {
this.setCurrentContext((event.result as { value: string }).value)|
@christian-bromann this is a much better approach handling both bidi and classic. I have tested this out and will update it accordingly. Thanks for the suggestions! |
christian-bromann
left a comment
There was a problem hiding this comment.
LGTM 👍 thanks a lot!
|
Thank you guys so much for the fix @navin772 and @christian-bromann ! so we need to pull new version of wdio to see this bug fixed right ? i.e 9.5.3 or so ? |
|
Yes, this will be out with the next |
Thanks! |
|
Hi @navin772, We had to downgrade the version due to this issue. If I find any reproducible example, I'll share it via a new bug. |
This is a regression in Chrome as reported in referenced issue. |
|
@christian-bromann Could you please share if this is the Chrome issue, why downgrade to 9.5.1 WDIO helped for the case with "wdio: enforceWebDriverClassic: true"? I didn't get it |
|
Oh, this is probably a regression then that resulted from this PR. We should check in the event listener whether there are contexts available and create one in case there aren't any. @navin772 @HannaTarasevich you want to take a stab at this fixing it? |
Proposed changes
Fixes #13989
Added an e2e test to test the changes for webdriver classic.
Types of changes
Checklist
Backport Request
//: # (The current
mainbranch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against thev8branch.)v9and doesn't need to be back-ported#XXXXXFurther comments
Reviewers: @webdriverio/project-committers