Skip to content

Conversation

arnonax-tr
Copy link
Contributor

@arnonax-tr arnonax-tr commented Sep 30, 2025

User description

💥 What does this PR do?

Adds a helper class WindowSwitcher and an extension method WithWindowOpenedBy that facilitates switching between windows.
For example:

driver.WithWindowOpenedBy(() => driver.FindElement(By.Id("openWindowId")).Click())
     .Do(() => {
         // Perform whatever you want with the new window, for example:
         driver.FindElement(By.Id("anElementOnTheNewWindow").Click();
         Assert.That(driver.Title, Is.EqualTo("The new window!"));
     });
// Then continue to do stuff on the original window:
driver.FindElement(By.Id("anElementOnTheOriginalWindow").Click();

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Add WindowSwitcher class for managing browser window switching

  • Add WithWindowOpenedBy extension method for WebDriver

  • Include comprehensive unit tests for window switching functionality

  • Provide fluent API for handling popup windows


Diagram Walkthrough

flowchart LR
  A["WebDriver"] -- "WithWindowOpenedBy()" --> B["WindowSwitcher"]
  B -- "Do(action)" --> C["New Window"]
  C -- "Auto Switch Back" --> A
  B -- "SwitchToNewWindow()" --> C
  B -- "SwitchToOriginalWindow()" --> A
Loading

File Walkthrough

Relevant files
Enhancement
WebDriverExtensions.cs
Add window switching extension method                                       

dotnet/src/support/Extensions/WebDriverExtensions.cs

  • Add WithWindowOpenedBy extension method for IWebDriver
  • Import OpenQA.Selenium.Support.UI namespace
  • Minor nullable annotation fix for IJavaScriptExecutor
+16/-1   
WindowSwitcher.cs
New WindowSwitcher class implementation                                   

dotnet/src/support/UI/WindowSwitcher.cs

  • Create new WindowSwitcher class with window management functionality
  • Implement Do() method for safe window switching with automatic return
  • Add SwitchToNewWindow() and SwitchToOriginalWindow() methods
  • Include comprehensive XML documentation with examples
+103/-0 
Tests
WindowSwitcherTest.cs
Unit tests for WindowSwitcher                                                       

dotnet/test/support/UI/WindowSwitcherTest.cs

  • Add unit test class for WindowSwitcher functionality
  • Test window switching behavior with popup windows
  • Verify correct window handle management and title validation
  • Include test setup and teardown methods
+49/-0   

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-support Issue or PR related to support classes labels Sep 30, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Nullable Annotation

The change removes the nullable annotation from the executor variable. Ensure GetDriverAs(driver) cannot return null at this point or that the null-coalescing throw guarantees safety in all target frameworks and analyzer settings.

private static object? ExecuteJavaScriptInternal(IWebDriver driver, string script, object?[] args)
{
    IJavaScriptExecutor executor = GetDriverAs<IJavaScriptExecutor>(driver)
        ?? throw new WebDriverException("Driver does not implement IJavaScriptExecutor");

    return executor.ExecuteScript(script, args);
Original Handle Timing

The original window handle is captured inside the WindowSwitcher constructor after receiving the new handle. If the current window changes between opening and construction, switching back may target an unexpected window. Consider capturing the original handle before invoking the action or passing it explicitly.

public WindowSwitcher(IWebDriver driver, string newWindowHandle)
{
    this.driver = driver;
    this.newWindowHandle = newWindowHandle;
    originalHandle = this.driver.CurrentWindowHandle;
}

@selenium-ci
Copy link
Member

Thank you, @arnonax-tr for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

Copy link
Contributor

qodo-merge-pro bot commented Sep 30, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add validation to the constructor
Suggestion Impact:The commit added null checks for driver and newWindowHandle in the constructor and documented the ArgumentNullException, though it did not add the equality check against the original handle.

code diff:

+    /// <exception cref="ArgumentNullException">Either <paramref name="driver"/> or <paramref name="newWindowHandle"/> is null.</exception>
     /// <remarks>
     /// <para>
     /// It is recommended to use the <see cref="WebDriverExtensions.WithWindowOpenedBy"/> to instantiate this
@@ -29,8 +30,8 @@
     /// </remarks>
     public WindowSwitcher(IWebDriver driver, string newWindowHandle)
     {
-        this.driver = driver;
-        this.newWindowHandle = newWindowHandle;
+        this.driver = driver ?? throw new ArgumentNullException(nameof(driver));
+        this.newWindowHandle = newWindowHandle ?? throw new ArgumentNullException(nameof(newWindowHandle));
         originalHandle = this.driver.CurrentWindowHandle;
     }

Add validation to the WindowSwitcher constructor to check for null arguments and
to ensure the new window handle is different from the original one.

dotnet/src/support/UI/WindowSwitcher.cs [30-35]

 public WindowSwitcher(IWebDriver driver, string newWindowHandle)
 {
-    this.driver = driver;
-    this.newWindowHandle = newWindowHandle;
+    this.driver = driver ?? throw new ArgumentNullException(nameof(driver));
+    this.newWindowHandle = newWindowHandle ?? throw new ArgumentNullException(nameof(newWindowHandle));
     originalHandle = this.driver.CurrentWindowHandle;
+    if (originalHandle == this.newWindowHandle)
+    {
+        throw new ArgumentException("The new window handle cannot be the same as the original one.", nameof(newWindowHandle));
+    }
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies missing argument validation in the constructor, which improves the class's robustness and provides clearer error messages for invalid usage.

Medium
General
Add a generic overload for Do
Suggestion Impact:The commit added Do(Func) and refactored Do(Action) to delegate to the generic overload, matching the suggestion’s intent.

code diff:

@@ -52,10 +53,38 @@
     /// </example>
     public void Do(Action action)
     {
+        _ = Do(() =>
+        {
+            action();
+            return 0;
+        });
+    }
+
+    /// <summary>
+    /// Invokes the provided function on the newly opened window, and returns to use the original window afterward. This method returns whatever the function returns.
+    /// </summary>
+    /// <param name="func">The function to invoke on the newly opened window.</param>
+    /// <returns>Anything that <paramref name="func"/> returns.</returns>
+    /// <example>
+    /// <code>
+    /// const string expectedResult = "abc";
+    /// var result = driver.WithWindowOpenedBy(() => driver.FindElement(By.Id("openWindowId")).Click())
+    ///     .Do(() => {
+    ///         // Perform whatever you want with the new window, for example:
+    ///         driver.FindElement(By.Id("anElementOnTheNewWindow").Click();
+    ///         return driver.FindElement(By.Id("resultElement")).Text;
+    ///     });
+    /// Assert.That(result, Is.EqualTo(expected));
+    /// // Then continue to do stuff on the original window:
+    /// driver.FindElement(By.Id("anElementOnTheOriginalWindow").Click();
+    /// </code>
+    /// </example>
+    public T Do<T>(Func<T> func)
+    {
         SwitchToNewWindow();
         try
         {
-            action();
+            return func();
         }
         finally
         {

Add a generic overload for the Do method that accepts a Func to allow returning
a value from the action performed in the new window.

dotnet/src/support/UI/WindowSwitcher.cs [53-64]

 public void Do(Action action)
 {
     SwitchToNewWindow();
     try
     {
         action();
     }
     finally
     {
         SwitchToOriginalWindow();
     }
 }
 
+/// <summary>
+/// Performs the provided function on the newly opened window, and returns to use the original window afterward.
+/// </summary>
+/// <param name="func">The function to perform on the newly opened window.</param>
+/// <typeparam name="T">The type of the value to be returned.</typeparam>
+/// <returns>The value returned by the function.</returns>
+public T Do<T>(Func<T> func)
+{
+    SwitchToNewWindow();
+    try
+    {
+        return func();
+    }
+    finally
+    {
+        SwitchToOriginalWindow();
+    }
+}
+

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion proposes a valuable enhancement by adding a generic overload to the Do method, which significantly increases its versatility by allowing a value to be returned from the new window.

Medium
Implement IDisposable for robust state management

Implement IDisposable on the WindowSwitcher class to ensure the driver's focus
is always returned to the original window, even if exceptions occur during
manual switching.

dotnet/src/support/UI/WindowSwitcher.cs [9-13]

-public class WindowSwitcher
+public class WindowSwitcher : IDisposable
 {
     private readonly IWebDriver driver;
     private readonly string newWindowHandle;
     private readonly string originalHandle;
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This is a good design suggestion that makes the class more robust and idiomatic for manual window switching by using the IDisposable pattern, although the existing Do method already handles the primary use case safely.

Low
Learned
best practice
Harden cleanup on switch-back

Guard against failures when switching back by catching and suppressing
window-not-found errors so cleanup is resilient if the new window is closed or
the driver context changes during the action.

dotnet/src/support/UI/WindowSwitcher.cs [53-64]

 public void Do(Action action)
 {
     SwitchToNewWindow();
     try
     {
         action();
     }
     finally
     {
-        SwitchToOriginalWindow();
+        try
+        {
+            SwitchToOriginalWindow();
+        }
+        catch (NoSuchWindowException)
+        {
+            // Original window may be gone; ignore to avoid masking the original exception.
+        }
+        catch (WebDriverException)
+        {
+            // Be defensive: ignore switch-back failures during cleanup.
+        }
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Enforce deterministic and safe resource handling; ensure cleanup on all code paths including exceptions.

Low
  • Update

@cgoldberg cgoldberg changed the title Add WindowSwitcher class [dotnet] Add WindowSwitcher class Sep 30, 2025
@arnonax-tr
Copy link
Contributor Author

Regarding the 2 other PR Code Suggestions, I don't think they're relevant:

Implement IDisposable for robust state management

If you're using the Do method, then you're safe. Otherwise, I don't think we can determine that switching back to the original window is the correct behavior in case of an error.

Harden cleanup on switch-back

I don't think we should swallow a NoSuchWindowException when we try to return from a Do method, nor ignoring or handling any other exception. Any exception that would be thrown from SwitchToOriginalWindow() should be bubbled up to the caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-support Issue or PR related to support classes C-dotnet .NET Bindings Review effort 2/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants