Skip to content

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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

nityanandbb
Copy link

@nityanandbb nityanandbb commented Jul 24, 2025

User description

🔗 Related Issues

Fixes #13229
(Handles InvalidSelectorException when using By.className inside Shadow DOM in Firefox)

💥 What does this PR do?

This PR fixes a known bug where By.className fails with InvalidSelectorException in Firefox when used to find elements inside a Shadow DOM.

  • Adds a Firefox-specific workaround: By.className("foo")By.cssSelector(".foo") if Shadow DOM context is detected
  • Leaves other browsers (e.g., Chrome, Edge) unchanged
  • Ensures compound class names are rejected with a clear message
  • Applies only within the ShadowRoot handling methods

🔧 Implementation Notes

  • Introduced FirefoxClassNameWorkaround (package-private) in org.openqa.selenium.remote.shadow
  • Updated ShadowRoot to resolve the selector before calling findElement or findElements
  • Added logic to extract and convert className to a valid .cssSelector
  • Wrote unit tests for Firefox and non-Firefox behavior in FirefoxClassNameWorkaroundTest

💡 Additional Considerations

  • Compound class names (e.g., btn primary) will be rejected with a helpful error message
  • Future improvement could generalize this conversion logic beyond just ShadowRoot
  • No breaking change introduced

🔄 Types of changes

  • Bug fix (backwards compatible)
  • Tests added
  • New feature
  • Breaking change
  • Cleanup only

Thank you Selenium maintainers for your time and review!


PR Type

Bug fix


Description

  • Fix By.className failing with InvalidSelectorException in Firefox Shadow DOM

  • Add 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

flowchart LR
  A["By.className"] --> B["FirefoxClassNameWorkaround"]
  B --> C{"Firefox + Shadow DOM?"}
  C -->|Yes| D["Convert to By.cssSelector"]
  C -->|No| E["Use original By"]
  D --> F["ShadowRoot.findElement"]
  E --> F
Loading

File Walkthrough

Relevant files
Bug fix
ShadowRoot.java
Integrate Firefox className workaround in ShadowRoot         

java/src/org/openqa/selenium/remote/ShadowRoot.java

  • Import FirefoxClassNameWorkaround class
  • Apply workaround resolution in findElement and findElements methods
  • Minor code formatting changes in equals method
+7/-8     
FirefoxClassNameWorkaround.java
Add Firefox className to CSS selector converter                   

java/src/org/openqa/selenium/remote/shadow/FirefoxClassNameWorkaround.java

  • Create new package-private class for Firefox-specific workaround
  • Implement browser detection logic for Firefox capabilities
  • Add className to CSS selector conversion with compound class
    validation
  • Provide public resolveForShadow method for ShadowRoot integration
+44/-0   

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@selenium-ci selenium-ci added the C-java Java Bindings label Jul 24, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Fragile Implementation

The extractClassName method uses string parsing on by.toString() which is fragile and could break if the toString format changes. Consider using reflection or a more robust approach to extract the className value from the By.ByClassName instance.

private static String extractClassName(By by) {
    String raw = by.toString().replace("By.className:", "").trim();
    return raw.replaceAll("\\s+", "");
}
Exception Handling

The broad catch block catches all exceptions and silently returns false, which could mask important errors like ClassCastException or other runtime issues. Consider catching more specific exceptions or adding logging.

} catch (Exception ignored) {
    return false;
}
Input Validation

The convertToCss method doesn't validate if the extracted className is empty or null, which could result in invalid CSS selectors like "." being generated.

static By convertToCss(By by) {
    String className = extractClassName(by);
    if (className.contains(" ")) {
        throw new InvalidSelectorException(
            "Compound class names not supported in Firefox Shadow DOM. Use single class name or By.cssSelector instead."
        );
    }
    return By.cssSelector("." + className);
}

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Escape special characters in CSS selectors

The CSS selector construction doesn't escape special characters that are valid
in class names but need escaping in CSS selectors. Add proper CSS escaping to
handle characters like colons, dots, or brackets.

java/src/org/openqa/selenium/remote/shadow/FirefoxClassNameWorkaround.java [26-34]

 static By convertToCss(By by) {
     String className = extractClassName(by);
     if (className.contains(" ")) {
         throw new InvalidSelectorException(
             "Compound class names not supported in Firefox Shadow DOM. Use single class name or By.cssSelector instead."
         );
     }
-    return By.cssSelector("." + className);
+    String escapedClassName = className.replaceAll("([:.\\[\\]()])", "\\\\$1");
+    return By.cssSelector("." + escapedClassName);
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a potential bug where class names with special CSS characters would create invalid selectors, and provides a correct fix to escape them, preventing runtime errors.

Medium
Possible issue
Use reflection for reliable field access

The string parsing approach is fragile and may break with different
By.ByClassName implementations. Use reflection to access the private className
field directly for more reliable extraction.

java/src/org/openqa/selenium/remote/shadow/FirefoxClassNameWorkaround.java [36-39]

 private static String extractClassName(By by) {
-    String raw = by.toString().replace("By.className:", "").trim();
-    return raw.replaceAll("\\s+", "");
+    try {
+        java.lang.reflect.Field field = by.getClass().getDeclaredField("className");
+        field.setAccessible(true);
+        return (String) field.get(by);
+    } catch (Exception e) {
+        throw new RuntimeException("Failed to extract className from By.ByClassName", e);
+    }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that parsing the toString() output is fragile and proposes a more robust reflection-based approach, which significantly improves code quality and maintainability.

Medium
Learned
best practice
Add null parameter validation

Add null checks for the by parameter and the extracted className before using
them. This prevents potential NullPointerException when these values are null or
when string operations are performed on null values.

java/src/org/openqa/selenium/remote/shadow/FirefoxClassNameWorkaround.java [26-34]

 static By convertToCss(By by) {
+    if (by == null) {
+        throw new IllegalArgumentException("By selector cannot be null");
+    }
     String className = extractClassName(by);
+    if (className == null || className.isEmpty()) {
+        throw new IllegalArgumentException("Class name cannot be null or empty");
+    }
     if (className.contains(" ")) {
         throw new InvalidSelectorException(
             "Compound class names not supported in Firefox Shadow DOM. Use single class name or By.cssSelector instead."
         );
     }
     return By.cssSelector("." + className);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add null checks and validation for parameters, properties, and return values before using them to prevent NullReferenceException and other runtime errors

Low
  • More

Copy link
Contributor

@pujagani pujagani left a 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;
Copy link
Contributor

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.

Copy link
Author

@nityanandbb nityanandbb Jul 25, 2025

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 keep ShadowRoot 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);
Copy link
Contributor

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.

@pujagani
Copy link
Contributor

I am not convinced of the approach used in this PR, I would like others to chime in. @diemol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants