Repository navigation
[java][dotnet] Support relative locators from elements and shadow roots - #17907
Conversation
PR Summary by QodoFix By.getJavascriptExecutor() to validate unwrapped driver JS support
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
9ce385e to
bdc7fc4
Compare
|
Code review by qodo was updated up to the latest commit bdc7fc4 |
|
Quick follow-up, since this has been sitting a while. CI never started on this fork PR (the workflow runs are On the cross-binding note: I compared this with .NET. Python does not hit this path, since |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
bdc7fc4 to
689eff9
Compare
|
@diemol @pujagani would you have a few minutes to review this? It was suggested I also ping @SeleniumHQ/selenium-tlc. Small Java fix: |
Code Review by Qodo
1. Single shadow finds lack coverage
|
diemol
left a comment
There was a problem hiding this comment.
This fix unblocks element/shadow-root-scoped relative locators, but neither call site (RelativeLocator.java nor RelativeLocatorServerSide.java) passes the context through to the FIND_ELEMENTS atom, so the atom's root always defaults to document.
That means calls like element.findElements(with(...)) will now silently search the entire page instead of just the element's subtree, whereas before they at least failed with a clear exception.
|
@diemol There's a new test in |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Code review by qodo was updated up to the latest commit eaeffda |
|
@Mochxd it seems the new added test is failing. Were you able to execute it on your end before submitting the code? |
|
@diemol I went through the remote log before changing anything.
Grid is calling On the client, I was going to convert that locator to a JSON-safe map first, same as the client, and keep the test. I have not pushed that yet. If you would rather keep relative locators document-scoped on the server, or do this a different way, say so and I will follow that. |
String.valueOf() turned relative locator maps into Map.toString() on the wire, so Grid could not run element-scoped relative locators.
|
@diemol I reproduced The locator was going over the wire as I dropped the stringify so the nested map stays JSON. After that the scoped Grid test passes. |
|
@diemol any updates? |
🔗 Related Issues
No existing issue. I hit this while looking at how relative locators resolve their search context.
💥 What does this PR do?
By.getJavascriptExecutor()resolves the driver withgetWebDriver(context), which unwraps aWrapsDriver, but then checkscontext instanceof JavascriptExecutorand castsdriver. The guard and the cast look at two different objects.The practical effect is that element-scoped relative locators throw.
RemoteWebElementimplementsWrapsDriverbut notJavascriptExecutor, so:fails with
IllegalArgumentException: Context does not provide a mechanism to execute JS: ...even though the driver it just unwrapped can execute JavaScript perfectly well. The same call works when made on the driver.The mismatch can also turn an intended
IllegalArgumentExceptioninto aClassCastException, when a context is itself aJavascriptExecutorbut the driver it wraps is not.Checking
driverinstead ofcontextfixes both. On its own that would have been incomplete: both relative-locator call sites invoke theFIND_ELEMENTSatom without a root, so the atom falls back todocumentand an element-scoped call would silently search the whole page. This PR also passes the search context as the atom root when it is not the driver, on both the client and server-side paths.🔧 Implementation Notes
By.getJavascriptExecutor(): one-word change. Left the exception message pointing atcontext, as that's the object the caller passed in.RelativeLocator/RelativeLocatorServerSide: passcontextas the atom's second argument when it is not aWebDriver; passnullfor a driver context so the atom keeps using the document.javascript/atoms/. The atom already accepts an optional root and already scopes candidate search with it.A unit test covers a context that wraps a JavaScript-capable driver. It reuses
StubDriverrather than a mock, which meant adding:helpersto theSmallTestsdeps.A browser test covers the scoped-search path: two matching candidates, one inside the context element and one outside. Element-scoped search returns only the inner one; driver-level search on the same page still returns both.
💡 Additional Considerations
For comparison, .NET already unwraps
IWrapsDriverinRelativeBy.GetExecutorwhen the context itself isn't anIJavaScriptExecutor. Its documented contract is "context is not IJavaScriptExecutor or wraps a driver that does." This brings Java in line with that.One difference I'm deliberately leaving alone: .NET walks the wrapper chain;
getWebDriver()unwraps a single level.Python doesn't hit this, since
RelativeByis only handled at the driver level.On shadow roots: I had this wrong in an earlier version of this description.
ShadowRootdoes implementWrapsDriver, andRelativeByis notBy.Remotable, soElementLocationfalls through tolocator.findElements(context)with the shadow root as the context. It therefore reaches this path and is passed as the atom root, wherequerySelectorAllscopes it correctly. I would still not call shadow DOM covered: there is no fixture for it here, and XPath inside a shadow tree is a separate weak spot in the atom.🔄 Types of changes