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

[dotnet] Added Deprecation to WebElement.GetAttribute() per #13334 #14676

Merged
merged 10 commits into from
Oct 31, 2024

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Oct 29, 2024

User description

Description

Added Deprecation to WebElement.GetAttribute() per #13334

Motivation and Context

#13334

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

  • Added a deprecation warning to the GetAttribute method in the IWebElement interface.
  • Suggested using the GetDomAttribute method as an alternative.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files
Enhancement
IWebElement.cs
Deprecate `GetAttribute` method in `IWebElement` interface

dotnet/src/webdriver/IWebElement.cs

  • Added an Obsolete attribute to the GetAttribute method.
  • Provided a deprecation warning message suggesting the use of
    GetDomAttribute.
  • +1/-0     

    💡 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: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Deprecation Notice
    Verify that the deprecation message is clear and provides sufficient guidance for users to migrate to the new method.

    Copy link
    Contributor

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @nvborisenko
    Copy link
    Member

    driver.Url = "https://nuget.org";
    
    var input = driver.FindElement(By.Id("search"));
    input.SendKeys("abc");
    
    var val = input.GetAttribute("value");
    var val2 = input.GetDomAttribute("value");
    var val3 = input.GetDomProperty("value");
    
    Console.WriteLine(val); // "abc"
    Console.WriteLine(val2); // ""
    Console.WriteLine(val3); // abc

    @nvborisenko nvborisenko requested a review from jimevans October 29, 2024 21:55
    @shbenzer
    Copy link
    Contributor Author

    driver.Url = "https://nuget.org";
    
    var input = driver.FindElement(By.Id("search"));
    input.SendKeys("abc");
    
    var val = input.GetAttribute("value");
    var val2 = input.GetDomAttribute("value");
    var val3 = input.GetDomProperty("value");
    
    Console.WriteLine(val); // "abc"
    Console.WriteLine(val2); // ""
    Console.WriteLine(val3); // abc

    good catch, I forgot to add GetDomProperty() to the deprecation note

    @nvborisenko
    Copy link
    Member

    We have to double check which one is correct: GetAttribute or GetDomAttribute. I though the correct one is GetAttribute.

    @shbenzer
    Copy link
    Contributor Author

    We have to double check which one is correct: GetAttribute or GetDomAttribute. I though the correct one is GetAttribute.

    According to @titusfortner in #13334

    . getAttribute() assumes that people don't know the difference between a property and an attribute and don't want to be bothered to figure it out (a decent assumption), so it uses a bunch of fancy JavaScript that does a pretty good job of guessing whether the user is looking for the property or the attribute and returning that value. It's great convenience, but it is now implemented with a client side atom which means a large payload gets sent to the driver/server on every usage.

    We decided in TLC Meeting 1/18 that we would go ahead and mark this feature deprecated to encourage people to move to the more precise property and attribute methods.

    @nvborisenko
    Copy link
    Member

    All right, GetAttribute() is deprecated.

    Since it is breaking change of core (most used interface) functionality, let's add info when it will be completely removed. In v5 or in v6? It should be definitely major selenium version, but which one... Usually we remove deprecated methods in 2 minor releases, but this another case, this would be real pain for all users. So major release should be considered. But if v5 will be released in 2 months, then it is not enough time range - then v6 is preferable.

    @shbenzer
    Copy link
    Contributor Author

    I agree that major releases should be considered. Since v5 is too soon, I'll put v6 in.

    Worst case we make a change later and make it v7.

    @nvborisenko
    Copy link
    Member

    @shbenzer please do not merge! Kind of code-freeze after release. Keep it open for a while.

    @nvborisenko
    Copy link
    Member

    @shbenzer please check failing CI build, easy to fix.

    @shbenzer shbenzer requested a review from nvborisenko October 31, 2024 19:01
    @shbenzer
    Copy link
    Contributor Author

    seemed to be missing the system import as well

    @nvborisenko nvborisenko merged commit ac523a5 into SeleniumHQ:trunk Oct 31, 2024
    10 checks passed
    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.

    2 participants