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

Bug 14229 fixed #14389

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Aug 13, 2024

User description

Updatedis_displayed functionality to ensure that form elements with child id's of "tagName" are handled safely. Added a test

Description

  1. Updated Domcore.js to handle FormHTMLElements safely
  2. Added form element with child input id of "tagName" to JavascriptPage.html for testing
  3. Added line in test_should_allow_the_user_to_tell_if_an_element_is_displayed_or_not() in Visibility_Tests.py to ensure this functionality is tested moving forward

Motivation and Context

Bug #14229

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

Bug fix, Tests


Description

  • Updated isElement function in domcore.js to handle HTMLFormElement safely and prevent failures when tagName is "tagName".
  • Added a form element with a child input id of "tagName" to javascriptPage.html for testing.
  • Enhanced test_should_allow_the_user_to_tell_if_an_element_is_displayed_or_not in visibility_tests.py to include the new form element.

Changes walkthrough 📝

Relevant files
Bug fix
domcore.js
Handle HTMLFormElement safely in isElement function           

javascript/atoms/domcore.js

  • Added a condition to handle HTMLFormElement safely in isElement
    function.
  • Ensured node.tagName.toUpperCase() does not fail when tagName is
    "tagName".
  • +5/-0     
    Tests
    visibility_tests.py
    Add test for form element visibility with specific child input id

    py/test/selenium/webdriver/common/visibility_tests.py

  • Added a test case to check visibility of form element with child input
    id of "tagName".
  • +1/-0     
    javascriptPage.html
    Add form element with specific child input id for testing

    common/src/web/javascriptPage.html

  • Added a form element with child input id of "tagName" for testing
    purposes.
  • +4/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Redundant Check
    The condition !!node is redundant in line 175 as it is already checked in line 177. Consider removing it to simplify the condition.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure the element exists before checking its visibility

    Add a check to ensure that the element with ID 'aParentFormId' exists before
    asserting its visibility to avoid potential NoSuchElementException.

    py/test/selenium/webdriver/common/visibility_tests.py [32]

    -assert driver.find_element(by=By.ID, value="aParentFormId").is_displayed() is True
    +element = driver.find_element(by=By.ID, value="aParentFormId")
    +assert element is not None and element.is_displayed() is True
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug by ensuring the element exists before performing operations on it, which can prevent runtime errors.

    9
    Enhancement
    Improve tag name comparison to handle different cases

    Replace the strict equality check "FORM" == opt_tagName with
    opt_tagName.toUpperCase() == "FORM" to handle different cases of opt_tagName and
    improve the function's flexibility.

    javascript/atoms/domcore.js [175]

    -(!opt_tagName || "FORM" == opt_tagName);
    +(!opt_tagName || opt_tagName.toUpperCase() == "FORM");
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the flexibility of the function by handling different cases of the tag name, which enhances robustness and usability.

    8
    Simplify the condition by removing the redundant instance check

    The condition node instanceof HTMLFormElement is redundant because the subsequent
    checks already ensure that the node is an element and optionally matches the tag
    name "FORM". Simplify the condition by removing the instance check.

    javascript/atoms/domcore.js [173-175]

    -if (node instanceof HTMLFormElement) {
    -  return !!node && node.nodeType == goog.dom.NodeType.ELEMENT &&
    -  (!opt_tagName || "FORM" == opt_tagName);
    +if (!!node && node.nodeType == goog.dom.NodeType.ELEMENT &&
    +  (!opt_tagName || "FORM" == opt_tagName)) {
    +  return true;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion simplifies the code by removing a redundant check, which improves readability and maintainability without altering functionality.

    7
    Add a submit type to the form's input element for better usability

    Add a type="submit" attribute to the input element within the form to clarify its
    function as a submit button, enhancing form usability and accessibility.

    common/src/web/javascriptPage.html [283-284]

     <form id="aParentFormId">
    -  <input type="text" name="tagName">
    +  <input type="submit" name="tagName">
     </form>
     
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While adding a submit type can improve usability, the current context does not indicate that the input is intended to be a submit button, making this suggestion less relevant.

    3

    @AutomatedTester AutomatedTester merged commit c3dd52e into SeleniumHQ:trunk Aug 14, 2024
    28 of 30 checks passed
    @AutomatedTester
    Copy link
    Member

    failing tests seem unrelated. Thanks for the PR

    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