SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
30.7k stars 8.19k forks source link

[🐛 Bug]: ExpectedCondition::elementToBeClickable does not consider CSS `pointer-events: none` #14427

Open rr-it opened 2 months ago

rr-it commented 2 months ago

What happened?

The docs mention

An expectation for checking an element is visible and enabled such that you can click it. See https://www.selenium.dev/selenium/docs/api/java/org/openqa/selenium/support/ui/ExpectedConditions.html#elementToBeClickable(org.openqa.selenium.By)

So elementToBeClickable should check if an element is clickable.

Elements that have CSS pointer-events: none set are not clickable. This should be considered in the check for clickability.

See https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events#values

How can we reproduce the issue?

The following <button>-element is not clickable:

HTML:

<button class="disabled">…</button>

CSS:

.disabled {
    pointer-events: none;
}

But elementToBeClickable only checks if an element is there and enabled. It does not check for the CSS property pointer-event: none - which might make an element non-clickable.

https://github.com/SeleniumHQ/selenium/blob/26f9aa2821641444c8946e484c72369d51289b7b/java/src/org/openqa/selenium/support/ui/ExpectedConditions.java#L617-L672

Adding element.getCssValue("pointer-events") != "none" to the condition might give the expected result.

Relevant log output

IMO not relevant here

Operating System

IMO not relevant here

Selenium version

IMO not relevant here

What are the browser(s) and version(s) where you see this issue?

IMO not relevant here

What are the browser driver(s) and version(s) where you see this issue?

IMO not relevant here

Are you using Selenium Grid?

No response

github-actions[bot] commented 2 months ago

@rr-it, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

shbenzer commented 2 months ago

Just because a pointer-event is set to none doesn't mean an element isn't clickable.

To use your example of a disable button: I can click it, but nothing should happen. If I were writing tests for security permissions, for example, I would want to be able to simulate clicking said button when it is enabled and when it is disabled, then assert that the result of clicking the button when enabled does not occur when the button is disabled.

rr-it commented 2 months ago

@shbenzer AFAIK your mentioned case does already not work for buttons "disabled" via pointer-events: none.

To use your example of a disable button: I can click it, but nothing should happen.

If you trigger a click on a button like <button style="pointer-events: none">…</button> an exception ElementClickInterceptedException might get thrown:

[ElementClickInterceptedException] element click intercepted:
Element <button type="button" class="dropdown-item disabled">...</button> is not clickable at point (121, 269).
Other element would receive the click: <li>...</li>

Here the underlying element in the background receives the click instead.


Just because a pointer-event is set to none doesn't mean an element isn't clickable.

This contradicts what https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events#none says:

pointer-events: none: The element on its own is never the target of pointer events.


ExpectedCondition::elementToBeClickable does already not return "really disabled" elements. What I like to change here is, that ExpectedCondition::elementToBeClickable handles "pointer-events: none-disabled" elements the same way - as those are already not clickable either.

E.g. a "really disabled" button via attribute disabled:

<button disabled>Test</button>

Also see:

shbenzer commented 2 months ago

While I still believe that elements should be clickable when they're disabled, you do make a good point. I just did some testing in Chrome in Vue.js on a button that is disabled and not only do I similarly get the same ElementClickInterceptedException you described when trying to click a disabled element, but element.is_enabled() also returns true for the disabled button.

<button type="button class="v-btn v-btn--disabled v-btn--has-bg theme--light v-size--small" disabled="disabled" style="" class="v-btn__content"><span>I'm a disabled button</span></button>

I think this might be evidence of a larger issue, and more than just the EC::elementToBeClickable needs to be updated. I think at least 3 things need to be updated to handle new disabled elements and pointer-events: none:

rr-it commented 2 months ago

IMO reading the docs WebDriver element enabled we might check what's the result of /session/{session id}/element/{element id}/enabled (WebElement.is_enabled()) in the following cases:

Enabled:

Disabled by attribute disabled:

Disabled by pointer-events: none:

Check all these cases with different browser engines:

As the 'element enabled' info might come directly from the browser engine.

shbenzer commented 2 months ago

Only thing that needed to be upgraded was isEnabled(). Thank you for pointing this out @rr-it

rr-it commented 2 months ago

I'd like to keep a difference between is enabled and is clickable. So not in any case is enabled equals is clickable.

So my conclusion:

shbenzer commented 2 months ago

Is_clickable checks if an element is enabled before clicking - by upgrading is_enabled to handle pointer-events:none, is_clickable should handle it correctly

rr-it commented 2 months ago

Is_clickable checks if an element is enabled before clicking - by upgrading is_enabled to handle pointer-events:none, is_clickable should handle it correctly

Actually someone might want to check for elements that are 'is_enabled' but are not 'is_clickable' at the same time.

I do not want to change any meaning of neither 'is_enabled' nor 'is_clickable'. I only want to fix 'is_clickable', so that it fulfills its meaning. I do not want to change the behaviour of 'is_enabled', as this would change the meaning of 'is_enabled'.


Button A: is_enabled: no and is_clickable: no

<button disabled>Button A</button>

Button B: is_enabled: yes and is_clickable: no

(Here the current implementation needs fixing as it states: is_enabled: yes and is_clickable: yes)

<button style="pointer-events: none">Button B</button>

shbenzer commented 2 months ago

Instances where an element is clickable require it to be of the following element types:

TagName.BUTTON TagName.INPUT TagName.OPTGROUP TagName.OPTION TagName.SELECT TagName.TEXTAREA

These tags are intended to be interacted with, in some way, by the mouse (at minimum by selecting them for keyboard interaction in the case of TagName.TEXTAREA).

While, yes, if one of these tags have pointer-events: none as an attribute, one might still be able to interact with it through the keyboard, to me that would be an exception to accepted practices. The buttons are effectively disabled to the average user. They won't be able to be targeted with anything except pure keyboard navigation (e.g. Key.TAB).

EC::ToBeClickable will accurately tell you the element can't be clicked Is_Enabled() will accurately tell you that the button is effectively disabled

@diemol, do you disagree?

rr-it commented 2 months ago

About this comment:

Is_Enabled() will accurately tell you that the button is effectively disabled

@shbenzer If we go on with your implemenation, it will only tell that a button is disabled for mouse. It won't tell anymore if it is enabled or disabled for keyboard.

I would not call a button "effectively disabled" if that button is disabled for mouse only, but is still enabled for keyboard.


These tags are intended to be interacted with, in some way, by the mouse (at minimum by selecting them for keyboard interaction in the case of TagName.TEXTAREA).

shbenzer commented 2 months ago

That is a good point, I hadn't considered that accessibility standards might require a site to be keyboard-only accessible.