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

upgraded isEnabled to handle pointer-events: none; updated formPage.h… #14428

Closed

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Aug 23, 2024

User description

Upgraded isEnabled() to handle attribute pointer-events: none

Description

  1. updated isEnabled()
  2. added example element to formPage.html
  3. added Pytest test_should_return_false_for_enabled_element_with_pointer_events_set_to_none

Motivation and Context

Bug #14427

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

  • Enhanced the isEnabled function to handle elements with pointer-events: none, ensuring they are treated as disabled.
  • Added a new test in element_attribute_tests.py to verify that elements with pointer-events: none are correctly identified as not enabled.
  • Updated formPage.html to include a button with pointer-events: none for testing purposes.

Changes walkthrough 📝

Relevant files
Bug fix
dom.js
Enhance isEnabled function to handle pointer-events: none

javascript/atoms/dom.js

  • Added check for pointer-events: none in isEnabled function.
  • Returns false if element has pointer-events: none.
  • +5/-0     
    Tests
    element_attribute_tests.py
    Add test for pointer-events: none handling in isEnabled   

    py/test/selenium/webdriver/common/element_attribute_tests.py

  • Added test for elements with pointer-events: none.
  • Ensures is_enabled returns false for such elements.
  • +5/-0     
    formPage.html
    Add test element with pointer-events: none to formPage     

    common/src/web/formPage.html

    • Added a button with pointer-events: none for testing.
    +3/-0     

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

    @shbenzer shbenzer changed the title upgraded isEnabled to handle pointer-events: none; updated formPage.h… upgraded isEnabled to handle pointer-events: none Aug 23, 2024
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot changed the title upgraded isEnabled to handle pointer-events: none upgraded isEnabled to handle pointer-events: none; updated formPage.h… Aug 23, 2024
    Copy link

    PR Reviewer Guide 🔍

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

    Possible Bug
    The new function bot.dom.hasPointerEventsDisabled_ is used but not defined in the provided diff. Ensure this function is implemented correctly.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Accessibility
    Add an aria-disabled attribute to improve accessibility for screen readers

    Add an aria-disabled attribute to the button element to improve accessibility. This
    will help screen readers understand that the button is effectively disabled.

    common/src/web/formPage.html [194]

    -<button id="pointerEventsNone" style="pointer-events: none;"><span>I'm an effectively disabled button</span></button>
    +<button id="pointerEventsNone" style="pointer-events: none;" aria-disabled="true"><span>I'm an effectively disabled button</span></button>
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding an aria-disabled attribute significantly improves accessibility by helping screen readers understand the button's disabled state, which is important for users relying on assistive technologies.

    9
    Enhancement
    Add an assertion to verify that the element with pointer-events set to none is displayed on the page

    Add an assertion to check if the element with pointer-events set to none is actually
    displayed on the page. This ensures that the test is not just checking the enabled
    state, but also verifying that the element exists and is visible.

    py/test/selenium/webdriver/common/element_attribute_tests.py [64-67]

     def test_should_return_false_for_enabled_element_with_pointer_events_set_to_none(driver, pages):
         pages.load("formPage.html")
         inputElement = driver.find_element(By.ID, "pointerEventsNone")
    +    assert inputElement.is_displayed()
         assert not inputElement.is_enabled()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding an assertion to check if the element is displayed enhances the test by ensuring the element's presence and visibility, which is important for comprehensive test coverage.

    8
    Maintainability
    Extract the pointer-events check into a separate function for improved code organization

    Consider extracting the hasPointerEventsDisabled_ check into a separate function for
    better readability and maintainability. This would make the isEnabled function
    cleaner and easier to understand.

    javascript/atoms/dom.js [200-203]

    -// check for whether the element has pointer-events: none
    -if (bot.dom.hasPointerEventsDisabled_(el)) {
    +if (bot.dom.isPointerEventsDisabled(el)) {
       return false;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Extracting the pointer-events check into a separate function can improve code readability and maintainability, making the isEnabled function cleaner and easier to understand. However, it is not a critical change.

    7
    Best practice
    Replace inline styles with a CSS class for better maintainability

    Consider adding a class to the button element instead of using inline styles. This
    would improve maintainability and allow for easier styling changes in the future.

    common/src/web/formPage.html [194]

    -<button id="pointerEventsNone" style="pointer-events: none;"><span>I'm an effectively disabled button</span></button>
    +<button id="pointerEventsNone" class="pointer-events-none"><span>I'm an effectively disabled button</span></button>
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a CSS class instead of inline styles improves maintainability and allows for easier styling changes in the future. This is a best practice but not crucial for functionality.

    6

    Copy link

    @rr-it rr-it left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Implementation is logically wrong.

    Please read #14427 (comment)

    def test_should_return_false_for_enabled_element_with_pointer_events_set_to_none(driver, pages):
    pages.load("formPage.html")
    inputElement = driver.find_element(By.ID, "pointerEventsNone")
    assert not inputElement.is_enabled()
    Copy link

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This is wrong:

    • The element is enabled.
    • The element is only not clickable (does not receive pointer events)
    • The element can still be triggered by keyboard

    Please read #14427 (comment)

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @@ -190,5 +190,8 @@
    </form>

    <input id="vsearchGadget" name="SearchableText" type="text" size="18" value="" title="Hvad søger du?" accesskey="4" class="inputLabel" />

    <button id="pointerEventsNone" style="pointer-events: none;"><span>I'm an effectively disabled button</span></button>
    Copy link

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This is wrong. This is not a disabled button.
    It does only not receive any pointer events. It can still be triggered by keyboard.

    Also see #14427 (comment)

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @shbenzer shbenzer closed this Aug 23, 2024
    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