-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[java] Fix: Convert By.className to css selector for Firefox inside Shadow DOM #16085
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
[java] Fix: Convert By.className to css selector for Firefox inside Shadow DOM #16085
Conversation
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for your changes. Also, update the related issue for this, it is pointing me to another PR for some reason.
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
if (this == o) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid formatting changes as part of a PR. If required, create a separate PR for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review @pujagani 🙏
-
I understand the concern about avoiding browser-specific logic in core classes like
ShadowRoot
. I'm working on isolating the logic into a utility class (e.g.,SelectorResolver
) to keepShadowRoot
clean and compliant with the architecture. I'll update the PR accordingly. -
I'll also remove any unrelated formatting changes from this PR and keep it focused on the bug fix only.
-
Regarding tests: I’ll add test coverage specifically for Firefox +
By.className
inside Shadow DOM to verify the selector resolution behavior. -
I'll update the linked issue to correctly point to this PR and add context for the changes.
-
For the CLA — I'll ensure it's signed shortly so the workflow can proceed.
Thanks again for the guidance. Will push the updates soon.
} | ||
|
||
@Override | ||
public WebElement findElement(By by) { | ||
By resolved = FirefoxClassNameWorkaround.resolveForShadow(by, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would highly recommend not adding browser-specific logic here.
I am not convinced of the approach used in this PR, I would like others to chime in. @diemol |
@nityanandbb, what bug is this PR fixing? The related issue is a pull request. |
@diemol This PR addresses an issue where driver.findElement(By.className(...)) and driver.findElements(...) fail in the latest versions of Firefox. Firefox no longer consistently supports By.className in certain DOM contexts, such as inside Shadow DOMs. To fix this, I've added logic that detects if the browser is Firefox and automatically converts By.className into By.cssSelector as a workaround. |
Can you please share the link to the GitHub issue? |
Sure @diemol, the related GitHub issue is #15961. |
Hi @diemol, thanks for the review! |
I'm not familiar with the Java bindings, but in Python, we always convert class name into a css selector. There is no "class name" locator strategy in the WebDriver Spec, so I don't see how all bindings don't already do this... which leaves me confused what this PR is trying to do. |
Thank you for your PR; however, the approach was a workaround instead of a proper fix. Perhaps you can check other open issues and send another PR, if you need guidance, please join our Slack channel https://www.selenium.dev/support/ The Java bindings do convert those locators to CSS before sending the payload to the driver, but this is done at a central place. We probably forgot to add the shadow DOM commands to this logic as well. I implemented the fix on #16149. |
User description
🔗 Related Issues
Fixes #15961
(Handles
InvalidSelectorException
when usingBy.className
inside Shadow DOM in Firefox)💥 What does this PR do?
This PR fixes a known bug where
By.className
fails withInvalidSelectorException
in Firefox when used to find elements inside a Shadow DOM.By.className("foo")
➝By.cssSelector(".foo")
if Shadow DOM context is detectedShadowRoot
handling methods🔧 Implementation Notes
FirefoxClassNameWorkaround
(package-private) inorg.openqa.selenium.remote.shadow
ShadowRoot
to resolve the selector before callingfindElement
orfindElements
.cssSelector
FirefoxClassNameWorkaroundTest
💡 Additional Considerations
btn primary
) will be rejected with a helpful error message🔄 Types of changes
Thank you Selenium maintainers for your time and review!
PR Type
Bug fix
Description
Fix
By.className
failing withInvalidSelectorException
in Firefox Shadow DOMAdd Firefox-specific workaround converting className to CSS selector
Implement compound class name validation with clear error messages
Add comprehensive unit tests for Firefox and non-Firefox behavior
Diagram Walkthrough
File Walkthrough
ShadowRoot.java
Integrate Firefox className workaround in ShadowRoot
java/src/org/openqa/selenium/remote/ShadowRoot.java
FirefoxClassNameWorkaround
classfindElement
andfindElements
methodsequals
methodFirefoxClassNameWorkaround.java
Add Firefox className to CSS selector converter
java/src/org/openqa/selenium/remote/shadow/FirefoxClassNameWorkaround.java
validation
resolveForShadow
method for ShadowRoot integration