[dotnet] Unwrap element wrapper recursively

[RenderMichael]

User description

Description

In many places, we only unwrap IWrapsElement once. This, however, has led to user issues.

This PR fixes that in a pain point from a user report. However, we should audit our usage of IWrapsElement and make sure this problem does not exist elsewhere.

There's also a lot of code duplication (or code that should be duplicated, but is implemented differently). We should centralize the element unwrapping logic.
 

Motivation and Context

Fixes #14513

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix, Tests

---

Description

• Fixed recursive unwrapping of IWrapsElement to handle nested wrappers.

• Added error handling for self-referencing element wrappers.

• Introduced new tests to validate double-wrapped element handling.

• Added WebElementWrapper class for testing wrapped elements.

---

Changes walkthrough 📝

	Relevant files
Bug fix	PointerInputDevice.cs Enhance `ConvertElement` to handle nested wrappers              dotnet/src/webdriver/Interactions/PointerInputDevice.cs Updated ConvertElement to recursively unwrap IWrapsElement. Added error handling for self-referencing wrappers. Improved exception message for invalid element conversion. +9/-2      WheelInputDevice.cs Enhance `ConvertElement` to handle nested wrappers              dotnet/src/webdriver/Interactions/WheelInputDevice.cs Updated ConvertElement to recursively unwrap IWrapsElement. Added error handling for self-referencing wrappers. Improved exception message for invalid element conversion. +8/-2
Tests	BasicMouseInterfaceTest.cs Add test for double-wrapped elements in mouse actions        dotnet/test/common/Interactions/BasicMouseInterfaceTest.cs Added test for handling double-wrapped elements in mouse actions. Verified MoveToElement and Click functionality with wrapped elements. +18/-0    BasicWheelInterfaceTest.cs Add test for double-wrapped elements in wheel actions        dotnet/test/common/Interactions/BasicWheelInterfaceTest.cs Added test for handling double-wrapped elements in wheel actions. Verified ScrollToElement functionality with wrapped elements. +17/-0    WebElementWrapper.cs Introduce `WebElementWrapper` for testing                                dotnet/test/common/WebElementWrapper.cs Added WebElementWrapper class implementing IWebElement and IWrapsElement. Used for testing wrapped element scenarios. +105/-0

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 14513 - Fully compliant Compliant requirements: Fix issue with EventFiringWebDriver's wrapped elements not working with Actions Support nested/multiple levels of element wrapping Handle IWrapsElement implementation correctly Ensure Actions.MoveToElement() works with wrapped elements

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Infinite Loop Risk The recursive unwrapping while loop could potentially enter an infinite loop if there's a circular reference between wrapped elements that isn't caught by the ReferenceEquals check while (elementWrapper != null) { elementReference = elementWrapper.WrappedElement as IWebDriverObjectReference; if (ReferenceEquals(elementWrapper, elementWrapper.WrappedElement)) { throw new InvalidOperationException("Cannot determine root element: element wrapper wraps itself"); } elementWrapper = elementWrapper.WrappedElement as IWrapsElement; } Code Duplication The ConvertElement method is duplicated between PointerInputDevice and WheelInputDevice. Consider extracting to a shared utility class private Dictionary<string, object> ConvertElement() { IWebDriverObjectReference? elementReference = this.target as IWebDriverObjectReference; if (elementReference == null) { IWrapsElement? elementWrapper = this.target as IWrapsElement; while (elementWrapper != null) { elementReference = elementWrapper.WrappedElement as IWebDriverObjectReference; if (ReferenceEquals(elementWrapper, elementWrapper.WrappedElement)) { throw new InvalidOperationException("Cannot determine root element: element wrapper wraps itself"); } elementWrapper = elementWrapper.WrappedElement as IWrapsElement; } } if (elementReference == null) { throw new ArgumentException($"Target element cannot be converted to {nameof(IWebDriverObjectReference)}"); } Dictionary<string, object> elementDictionary = elementReference.ToDictionary(); return elementDictionary;

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent infinite recursion Add a maximum recursion depth check to prevent potential stack overflow from deeply nested element wrappers. dotnet/src/webdriver/Interactions/PointerInputDevice.cs [600-610] +const int MaxRecursionDepth = 100; +int depth = 0; while (elementWrapper != null) { + if (depth++ > MaxRecursionDepth) + { + throw new InvalidOperationException("Maximum element wrapper depth exceeded"); + } elementReference = elementWrapper.WrappedElement as IWebDriverObjectReference; if (ReferenceEquals(elementWrapper, elementWrapper.WrappedElement)) { throw new InvalidOperationException("Cannot determine root element: element wrapper wraps itself"); } elementWrapper = elementWrapper.WrappedElement as IWrapsElement; } Apply this suggestion Suggestion importance[1-10]: 8 __ Why: Adding a maximum recursion depth check is crucial for preventing stack overflow errors in production code, especially when dealing with potentially deeply nested element wrappers. This is a significant safety improvement.	Medium
	Handle null wrapped elements Add null check for WrappedElement to prevent NullReferenceException if an IWrapsElement implementation returns null. dotnet/src/webdriver/Interactions/PointerInputDevice.cs [600-610] while (elementWrapper != null) { - elementReference = elementWrapper.WrappedElement as IWebDriverObjectReference; - if (ReferenceEquals(elementWrapper, elementWrapper.WrappedElement)) + var wrappedElement = elementWrapper.WrappedElement; + if (wrappedElement == null) + { + throw new InvalidOperationException("Element wrapper returned null for WrappedElement"); + } + elementReference = wrappedElement as IWebDriverObjectReference; + if (ReferenceEquals(elementWrapper, wrappedElement)) { throw new InvalidOperationException("Cannot determine root element: element wrapper wraps itself"); } - elementWrapper = elementWrapper.WrappedElement as IWrapsElement; + elementWrapper = wrappedElement as IWrapsElement; } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The suggestion adds important null checking for WrappedElement to prevent potential NullReferenceException, which could occur if an IWrapsElement implementation incorrectly returns null. This improves error handling and debugging.	Medium
Learned best practice	Add parameter validation to fail fast when required parameters are null Add a null check for the target parameter at the start of the ConvertElement method to fail fast if the target is null, rather than letting it fail later when trying to use it. dotnet/src/webdriver/Interactions/PointerInputDevice.cs [597-603] private Dictionary<string, object> ConvertElement() { + ArgumentNullException.ThrowIfNull(target, nameof(target)); + IWebDriverObjectReference? elementReference = this.target as IWebDriverObjectReference; if (elementReference == null) { IWrapsElement? elementWrapper = this.target as IWrapsElement; while (elementWrapper != null) [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6	Low
Update

[RenderMichael]

This protects against a user that has something like

public class MyElementWrapper : IWebElement, IWrapsElement
{
    public IWebElement WrappedElement => this;
}

An infinite loop is probably the last thing we want here.

Should we have stronger protections? Maybe a maximum unwrap count, or cycle detection?

[RenderMichael]

According to the history, IWebElementReference was renamed to IWebDriverObjectReference in the past. Fixing that in the string as well.

Should the message be improved? IWebDriverObjectReference is an internal type, an implementation detail. The real error here is:

"target element does not ultimately wrap `WebElement`; cannot use fully custom `IWebElement` here"

I'm not sure if that's what we want to tell users though.