selenium icon indicating copy to clipboard operation
selenium copied to clipboard
verified publisher icon SeleniumHQ

upgraded EC::toBeClickable for pointer-events: none

Open shbenzer opened this issue 1 year ago • 8 comments

User description

Upgraded ExpectedCondition::toBeClickable methods to correctly handle pointer-events: none.

Description

  • Upgraded ExpectedCondition::toBeClickable methods to correctly handle pointer-events: none.
  • Upgraded webdriverwait_tests.py::test_expected_condition_element_to_be_clickable
  • added example to javascript.html for above test

Motivation and Context

Bug #14427

Types of changes

  • [x] 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

  • [x] I have read the contributing document.
  • [x] My change requires a change to the documentation.
  • [x] I have updated the documentation accordingly.
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

PR Type

Bug fix, Tests

Description

  • Enhanced ExpectedCondition::elementToBeClickable methods to correctly handle elements with pointer-events: none.
  • Added a test case in webdriverwait_tests.py to verify that elements with pointer-events: none are not considered clickable.
  • Updated javascriptPage.html to include a button with pointer-events: none for testing purposes.

Changes walkthrough 📝

Relevant files
Bug fix
ExpectedConditions.java
Enhance elementToBeClickable to handle pointer-events: none

java/src/org/openqa/selenium/support/ui/ExpectedConditions.java

  • Updated elementToBeClickable methods to check for pointer-events:
    none.
  • Ensures elements with pointer-events: none are not considered
    clickable.
    		•  +2/-2     
    Tests
    webdriverwait_tests.py
    Add test for pointer-events: none in clickable elements   

    py/test/selenium/webdriver/common/webdriverwait_tests.py

  • Added test case for elements with pointer-events: none.
  • Ensures elements with pointer-events: none are not clickable.
    		•  +7/-0     
    javascriptPage.html
    Add example button with pointer-events: none                         

    common/src/web/javascriptPage.html

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

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

    shbenzer avatar Aug 23 '24 16:08 shbenzer

    PR Reviewer Guide 🔍

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

    Performance Concern
    The added check for 'pointer-events' CSS property might introduce a slight performance overhead, as it requires an additional call to getCssValue() for each element.

    Code Duplication
    The same logic is duplicated in two methods (elementToBeClickable). Consider extracting this logic into a separate private method to improve maintainability.

    qodo-code-review[bot] avatar Aug 23 '24 16:08 qodo-code-review[bot]

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    ✅ Use pytest.raises instead of a try-except block for exception testing
    Suggestion Impact:The try-except block was replaced with a pytest.raises context manager, improving test clarity and aligning with pytest best practices.

    code diff:

    -    try:
-        WebDriverWait(driver, 0.5).until(EC.element_to_be_clickable((By.ID, "btnPointerEventsNone")))
-        assert False
-    except:
-        pass
+    with pytest.raises(TimeoutException):
+        WebDriverWait(driver, 0.7).until(EC.element_to_be_clickable((By.ID, "btnPointerEventsNone")))

    Replace the try-except block with an assertion using pytest.raises to improve test
    clarity and follow pytest best practices.

    py/test/selenium/webdriver/common/webdriverwait_tests.py [282-286]

    -try:
+with pytest.raises(TimeoutException):
     WebDriverWait(driver, 0.5).until(EC.element_to_be_clickable((By.ID, "btnPointerEventsNone")))
-    assert False
-except:
-    pass
    • [ ] Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Replacing the try-except block with pytest.raises improves test clarity and aligns with pytest best practices, making the test more readable and idiomatic.

    		9
    Enhancement
    Extract the element clickability check into a separate method

    Consider extracting the element clickability check into a separate method to improve
    code readability and reusability.

    java/src/org/openqa/selenium/support/ui/ExpectedConditions.java [622]

    -if (element != null && element.isEnabled() && (element.getCssValue('pointer-events').toLowerCase() != 'none')) {
+private boolean isElementClickable(WebElement element) {
+    return element != null && element.isEnabled() && !POINTER_EVENTS_NONE.equalsIgnoreCase(element.getCssValue("pointer-events"));
+}
+...
+if (isElementClickable(element)) {
    Suggestion importance[1-10]: 8

    Why: Extracting the clickability check into a separate method enhances code readability and reusability, making the code cleaner and easier to maintain.

    		8
    ✅ Add an assertion to verify the presence and non-clickability of the element
    Suggestion Impact:The commit implemented the suggestion to use pytest.raises to assert that the element is not clickable, although it did not add an assertion for visibility and enabled state.

    code diff:

    +    with pytest.raises(TimeoutException):
+        WebDriverWait(driver, 0.7).until(EC.element_to_be_clickable((By.ID, "btnPointerEventsNone")))

    Add an assertion to verify that the element with pointer-events: none is present but
    not clickable, to ensure the test is comprehensive.

    py/test/selenium/webdriver/common/webdriverwait_tests.py [281-286]

     element = driver.find_element(By.ID, "btnPointerEventsNone")
-try:
+assert element.is_displayed() and element.is_enabled(), "Element should be visible and enabled"
+with pytest.raises(TimeoutException):
     WebDriverWait(driver, 0.5).until(EC.element_to_be_clickable((By.ID, "btnPointerEventsNone")))
-    assert False
-except:
-    pass
    • [ ] Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding an assertion ensures the test is comprehensive by verifying that the element is present and not clickable, enhancing the test's robustness.

    		8
    Maintainability
    Introduce a constant for the 'none' value in the pointer-events check

    Consider using a constant for the 'none' value in the pointer-events check to
    improve maintainability and reduce the risk of typos.

    java/src/org/openqa/selenium/support/ui/ExpectedConditions.java [622]

    -if (element != null && element.isEnabled() && (element.getCssValue('pointer-events').toLowerCase() != 'none')) {
+private static final String POINTER_EVENTS_NONE = "none";
+...
+if (element != null && element.isEnabled() && !POINTER_EVENTS_NONE.equalsIgnoreCase(element.getCssValue("pointer-events"))) {
    Suggestion importance[1-10]: 7

    Why: Using a constant for the 'none' value improves maintainability by reducing the risk of typos and making the code easier to update if the value changes.

    		7

    qodo-code-review[bot] avatar Aug 23 '24 16:08 qodo-code-review[bot]

    It might be better to raise a speaking WebDriverException instead of just returning false, this could help people to understand what is wrong.

    Beside this, i am not sure this will work as expected. The webdriver will click the center of the element, if this would hit the element or a child inside it is considered as a working click and will be performed, otherwise a ElementClickInterceptedException is raised.

    So i would expect element.click() to work in case the element has pointer-events: none and there is a child with a different pointer-events value inside it, covering the center of the element.

    joerg1985 avatar Sep 04 '24 11:09 joerg1985

    PS: comparing strings with != is a reference check in java, this must be changed to .equals

    joerg1985 avatar Sep 04 '24 11:09 joerg1985

    It might be better to raise a speaking WebDriverException instead of just returning false, this could help people to understand what is wrong.

    Beside this, i am not sure this will work as expected. The webdriver will click the center of the element, if this would hit the element or a child inside it is considered as a working click and will be performed, otherwise a ElementClickInterceptedException is raised.

    So i would expect element.click() to work in case the element has pointer-events: none and there is a child with a different pointer-events value inside it, covering the center of the element.

    I'm totally up for suggestions and don't mind reworking the solution. What do you think would be a better way of accomplishing this?

    My thinking was that we already raise a webdriverexception from the element.click() if the element and no children are clickable, so wouldn't adding the pointer-events:none to the check if an element is clickable be sufficient? Or are you saying we should update the exception to tell you what the cause of it not being clickable is?

    PS: comparing strings with != is a reference check in java, this must be changed to .equals

    Good catch

    shbenzer avatar Sep 04 '24 13:09 shbenzer

    Don't get me wrong, but i think the only way to check if something is clickable is to click and check exactly the expected action happens. And this check does not cover cases other unexpected things are happening too. And this check is not invertable, e.g. to check the click will open a popup is possible, but it might have run javascript and modified the original page too. It might be possible in the future to implement a clickAndExpectNoChangeToThePage or clickAndExpectNoClickEventHandlerCalled using BiDi (or now using the CDP).

    People are assuming the ExpectedConditions.elementToBeClickable does really check everything e.g. https://github.com/SeleniumHQ/selenium/issues/14030#issuecomment-2129334529 but this is not true. And even adding a statement like might return incorrect results to the javadoc will alot of people not read or ignore.

    So i think it would be the best to deprecate it now and remove it in Selenium 5. This will probably not happen, just a little dream of me 😄

    joerg1985 avatar Sep 04 '24 14:09 joerg1985

    That was one of my original concerns with this bug ticket. At the end of the day I can move my mouse around and click anything I want, things just might not happen.

    I like the idea of toBeClickable being refactored into something like toBeInteractable or clickHasConsequences. The check instead being will something happen if you click it, not can you click it

    shbenzer avatar Sep 04 '24 14:09 shbenzer

    Adding an EC of toBeInteractable would probably be best. Would serve as a check for the element not interactable exception, as well. https://w3c.github.io/webdriver/#dfn-error-code

    shbenzer avatar Sep 04 '24 23:09 shbenzer

    @VietND96 I don't think this should be merged, see my concerns above in https://github.com/SeleniumHQ/selenium/pull/14432#issuecomment-2328743898

    joerg1985 avatar Nov 12 '24 10:11 joerg1985

    Ah yes, I saw. I just adding a label for filtering PRs that have the potential to merge. Of course, the PR needs to be reviewed and approved.

    VietND96 avatar Nov 12 '24 11:11 VietND96