[java][py] Upgraded EC::toBeClickable for pointer-events: none

[shbenzer]

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

• 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 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

[qodo-merge-pro]

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-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	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

[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.

[joerg1985]

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

[shbenzer]

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

[joerg1985]

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. #14030 (comment) 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 😄

[shbenzer]

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]

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

[joerg1985]

@VietND96 I don't think this should be merged, see my concerns above in #14432 (comment)

[VietND96]

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.