Skip to content
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

WIP [dotnet] Experimental Displayed #15091

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 15, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

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

Enhancement


Description

  • Modified the Displayed property in WebElement to use a new visibility check script.

  • Replaced the usage of is-displayed.js with an inline JavaScript function.

  • Enhanced visibility checks with additional parameters for opacity and CSS visibility.


Changes walkthrough 📝

Relevant files
Enhancement
WebElement.cs
Enhanced `Displayed` property with new visibility script 

dotnet/src/webdriver/WebElement.cs

  • Updated the Displayed property to use a new inline JavaScript
    function.
  • Removed the reference to is-displayed.js.
  • Added parameters for checking opacity and CSS visibility in the
    visibility check script.
  • +2/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Validation Required

    The new checkVisibility function with checkOpacity and checkVisibilityCSS parameters needs validation to ensure it works correctly across different browsers and edge cases

    parameters.Add("script", "return arguments[0].checkVisibility({ checkOpacity: true, checkVisibilityCSS: true });");

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add defensive programming to validate the existence of required JavaScript methods before execution

    Add error handling to verify that the checkVisibility method exists on the element,
    as this is a custom JavaScript method that might not be available in all browser
    contexts.

    dotnet/src/webdriver/WebElement.cs [188]

    -parameters.Add("script", "return arguments[0].checkVisibility({ checkOpacity: true, checkVisibilityCSS: true });");
    +parameters.Add("script", @"
    +  if (typeof arguments[0].checkVisibility !== 'function') {
    +    throw new Error('checkVisibility method is not available on the element');
    +  }
    +  return arguments[0].checkVisibility({ checkOpacity: true, checkVisibilityCSS: true });
    +");
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial runtime validation to ensure the JavaScript method exists before execution, preventing silent failures in unsupported browser environments.

    8
    General
    Improve error handling to provide clear feedback when visibility checks fail

    Handle potential JavaScript exceptions that might occur during visibility check
    execution to provide more meaningful error messages.

    dotnet/src/webdriver/WebElement.cs [190-192]

    -commandResponse = this.Execute(DriverCommand.ExecuteScript, parameters);
    -return (bool)Convert.ChangeType(commandResponse.Value, typeof(bool));
    +try {
    +    commandResponse = this.Execute(DriverCommand.ExecuteScript, parameters);
    +    return (bool)Convert.ChangeType(commandResponse.Value, typeof(bool));
    +} catch (WebDriverException ex) {
    +    throw new WebDriverException("Failed to determine element visibility: " + ex.Message, ex);
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding specific exception handling with descriptive error messages will significantly improve debugging and error identification when visibility checks fail.

    7

    @nvborisenko nvborisenko marked this pull request as draft January 15, 2025 17:46
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant