SeleniumHQ / selenium

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

[🐛 Bug]: Opacity : 0 is not considered as "Displayed" #14030

Open aarahmanqa opened 4 months ago

aarahmanqa commented 4 months ago

What happened?

In our React powered Web App, one of the checkboxes is getting isDisplayed() as false although everything is fine.

  1. The element is available in DOM.
  2. isEnabled() is true
  3. element.click() is working
  4. element.getSize() is having non-zero width and height.

When I checked, found that Selenium is considering opacity : 0 as invisible and hence making isDisplayed as false. This is happening in antd react library which is a common library among React Web Apps and this issue seems to be happening in all these areas.

Proposal is, we shall remove this check for opacity.

Additional Note: https://playwright.dev/docs/actionability#visible

In Playwright also, opacity 0 is not considered as invisible.

How can we reproduce the issue?

//As you could see here, only isDisplayed() is false, everything else is fine.

public class RadioTest {
    public static void main(String[] args) throws InterruptedException {
        WebDriver driver = new ChromeDriver();
        driver.get("https://ant.design/components/radio");
        WebElement radioButton = driver.findElement(By.xpath("(//input[@class='ant-radio-input'])[1]"));
        System.out.println("Displayed : " + radioButton.isDisplayed());
        System.out.println("Enabled : " + radioButton.isEnabled());
        System.out.println("Size : " + radioButton.getSize());
        Rectangle rect = radioButton.getRect();
        System.out.println("Rect : " + Arrays
                .stream(rect.getClass().getFields())
                .collect(Collectors
                        .toMap(
                                Field::getName,
                                field -> getValueFromField(field, rect)
                        )
                )
        );
        Thread.sleep(5000);
        driver.quit();
    }

    public static Object getValueFromField(Field field, Object rect) {
        try {
            return field.get(rect);
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        }
    }
}

Relevant log output

NA

Operating System

Mac

Selenium version

4.20.0

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

Chrome

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

NA

Are you using Selenium Grid?

No

github-actions[bot] commented 4 months ago

@aarahmanqa, 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!

diemol commented 4 months ago

I thought opacity 0 meant fully transparent, so the human eye cannot see it. Is that correct?

aarahmanqa commented 4 months ago

@diemol Yes. That was my understanding too. But in React libraries like antd they are making the opacity as 0 by default.

The element is visible here despite opacity being 0.

The elementToBeClickable on these elements is only throwing TimeoutException because of isDisplayed being false. Hence this change would be needed.

diemol commented 4 months ago

Why do they do that? Does that make sense? Have you checked with them?

diemol commented 4 months ago

My point is that if, by definition, opacity zero means transparency, it is not visible. Why should we change that if a library decides to do the opposite?

aarahmanqa commented 4 months ago

@diemol , Yes I agree.

Lets approach from end user perspective. Why would someone call isDisplayed()? To perform click or other actions.

Opacity being 0 is not going to affect such click or other actions, right? The element is going to be in DOM just that its transparent. Related Link : https://discuss.codecademy.com/t/when-is-it-better-to-use-visibility-hidden/365560/21

So, IMO, the opacity 0 check in isDisplayed() is not necessary.

Its for the same reason, the other tools like Playwright, Cypress are not doing that.

diemol commented 4 months ago

Can a human being see an element that has opacity 0? The WebDriver approach is to simulate how humans interact with sites.

Anyway, aside from that, have you reached out to those libraries? What is their approach to determining that something is visible? Why do they use zero opacity?

aarahmanqa commented 4 months ago

In this case, the element is still visible. Just that, since the opacity is 0, we are getting a lighter circle. Making it 1 manually, causing darker circle.

Have raised this query in antd and awaiting response : https://github.com/ant-design/ant-design/discussions/49061

joerg1985 commented 4 months ago

@aarahmanqa

Lets approach from end user perspective. Why would someone call isDisplayed()? To perform click or other actions.

We (the company i am working) are using isDisplayed to ensure something is shown to the user, when ignoring opacity all these tests are worthless. We do not check the element isDisplayed before clicking it, because the click will fail in this case and we do not have to explicit check it.

Almost everytime i saw someone waited for an element to get visible to click, the previouse interaction was not completed. These kind of wait are - in my mind - bad practice, as you can never tell if it is enoght to wait for visibility. e.g. the page should change by the previouse interaction, but there is an identical element on the old page.

aarahmanqa commented 4 months ago

@joerg1985 I hope I understood your perspective. Please correct me if otherwise.

You first told about how you are using isDisplayed to determine the elements are visible to the user.

I hope you know the conditions of isDisplayed. Just reiterating it here:

  1. Size should not be zero.
  2. Css style display should not be none.
  3. Opacity should not be 0.

Now, are you sure that your application is modifying only the opacity to 0 to make it invisible? As per the link I have shared above, user can still interact with an element that has opacity 0.

Ideally you should have gone for display: none to make it invisible.

Your next comment on click

Even in Selenium documentation, the preferred way to wait before click on any element is using ExpectedConditions.elementToBeClickable()

If you check the inside implementation of this method, its just isDisplayed() and isEnabled().

diemol commented 4 months ago

I interpret @joerg1985's statement as meaning that doing isDisplayed() before clicking is useless because the click would fail anyway.

On the other hand, we don't need to change because one React library changes the behavior. If we change, there has to be a stronger argument. In addition, what would happen to the folks who are relying on the zero opacity to have elements not being displayed?

joerg1985 commented 4 months ago

@aarahmanqa

Now, are you sure that your application is modifying only the opacity to 0 to make it invisible? As per the link I have shared above, user can still interact with an element that has opacity 0.

The W3C spec and the webdriver do not use isDisplayed as precondition to click the element. You might be able to perform the click without an element not interactable exception.

Even in Selenium documentation, the preferred way to wait before click on any element is using ExpectedConditions.elementToBeClickable()

This was a "in my mind" statement so others might think different ;) Ususally it does no harm to wait for elementToBeClickable but it does not help in some cases.

AutomatedTester commented 4 months ago

display: none

It does check that and is the first thing it checks if you look at the atom. https://github.com/SeleniumHQ/selenium/blob/062c125c062eea270c9ce8dafcb8f92f32e1bf51/javascript/atoms/dom.js#L573

If we look at the MDN documentation for opacity, if it is set to 0 it is not displayed. It may live in the DOM and it might have rect size, display:none won't have a rect size, but it is still ultimately it is not visible.

I've created an example as well in https://jsbin.com/xatuqevatu/ ... how would you work with any of the elements on that page if opacity is 0?

AutomatedTester commented 4 months ago

Its for the same reason, the other tools like Playwright, Cypress are not doing that.

They are allowing potential click jacking scenarios. We have a different opinion here on what is expected for users and generally not a good enough reason. As mentioned in my previous comment how would people interact with the form that is in the page...

I've created an example as well in https://jsbin.com/xatuqevatu/ ... how would you work with any of the elements on that page if opacity is 0?

diemol commented 4 months ago

Another question: for accessibility purposes, what would a screen reader do in this case?

shs96c commented 4 months ago

I should point out that it's entirely possible to use the Actions APIs to allow you to click as well, and this doesn't do the same kind of checks.

We used to talk about APIs in Selenium being one of two kinds: "do what I mean" and "do what I say". WebElement.click is definitely a "do what I mean" API -- it has a number of checks it makes to ensure that clicking is a meaningful thing to do. OTOH, Actions.click is a "do what I say" API -- it has far fewer checks, and just does what you tell it to.

@AutomatedTester is right that the behaviour we have in WebElement.click is designed to handle the common scenarios, and click-jacking was very definitely raised as a concern (and may have been what lead us to add the opacity check in the first place, IIRC). If you really, truly just want to click on "the space of the screen where the element is rendered (even if we calculate the element isn't displayed)", that Actions API is always available, and in this case is pretty easy to use.