[🚀 Feature]: Methods `Select.select*()` should NOT allow selecting DISABLED option (or select)

[asolntsev]

Feature and motivation

Currently Select methods work even the option is disabled. Or select is disabled.

I suggest these selectByFoo methods should throw an exception.

Usage example

new Select(element).selectByVisibleText("fish");

Pull request

see https://github.com/SeleniumHQ/selenium/pull/10814

[github-actions[bot]]

@asolntsev, 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]

Is this something you see in all browsers or in an specific browser? I am probably wrong, but I was reading the WebDriver spec and in 14.1 (https://www.w3.org/TR/webdriver1/#element-click), step 7 talks about

If element is not disabled

But I assume it talks about the whole element, not the specific options. I just want to understand better the problem, and figure out if this needs a fix in Selenium or in the WebDriver spec.

@titusfortner @AutomatedTester @jimevans what do you think?

[asolntsev]

@diemol I am not sure about the spec, but in reality both <select> and <option> can have disabled attribute.

1. In first case, you cannot select any options
2. In second case, you can select other options except the disabled one.

[titusfortner]

No, Selenium should not prevent a user from clicking anything they want. A disabled element could theoretically have any number of JS event listeners.

That's why the spec allows all the other actions to be taken on a disabled option element except for the part about changing "selectedness".

The driver is doing the right thing and Selenium is doing the right thing.

[titusfortner]

Watir, on the other hand, does prevent this, because as a tester, 99% of the time trying to select something that is disabled is wrong, so we've added extra logic to raise an error for it.

@asolntsev we discussed this a couple years ago about why Watir subclasses elements. This is a good use case. Watir automatically waits for an element to be enabled if it is one of Input, Button, Select, Option and otherwise ignores whether the element is enabled.

[titusfortner]

Hmm, to argue with myself a second, this *is in an opinionated class. The intention of "select" is different from the intention of "click" even if that is the implementation detail happening below.

[titusfortner]

This is one of those weird places where Selenium tries to have it both ways. It's "just browser automation" (aka "do what I say"), except it also has these support classes to provide "do what I mean" behaviors, but only in some cases, and not implemented in a way that can really be uniformly applied throughout a test framework.

[titusfortner]

Ok, I've convinced myself that this is should be considered a bug for this class as written. But as @pujagani pointed out in the PR, this isn't just a Java issue.

[asolntsev]

This is a good question how the library should behave. :)

I made a small experiment.

1. Give a disabled select (like <select id="frozen" disabled><option value=""></option><option value="anna">Anna</option></select>), or
2. Given a select with disabled option (like <select id="frozen"><option value=""></option><option value="anna" disabled>Anna</option></select>)

In both cases, Selenium behaves differently from real browser/user:

    Select select = new Select($("#frozen"));
    select.selectByVisibleText("Anna");   // does NOT fail, looks like succeeded
    select.getFirstSelectedOptions();   // returns ANOTHER option (the first empty option)

While the end-users cannot select any options at all. I believe this is a bug. Clicking this option is not even possible in #1 case - the end user doesn't even see the options.

[diemol]

One thing I am still missing is if this behavior is present in just one browser or in all of them. Which browsers have you tried?

[titusfortner]

I believe all browsers are doing the right thing according to the spec. The issue is only whether Selenium should be more opinionated in this support class implementation, and I think @asolntsev makes a good case for it.

The user should be allowed to "click" a disabled option element and the browsers will do what the spec tells them to without error.

If the user wants to "select" an option via the higher level Select class, though, that method should error if the option is not in a state that can be selected. The current implementation just executes the click which, per the spec, doesn't change "selectedness." If the intent is to select, Selenium should error.

[titusfortner]

So I'm looking through this in the Ruby bindings, and it brings up a bunch of weird edge cases...

Should getOptions() include disabled options? Should deselectAll() error if there is a disabled option?

I also don't think any of these methods should work for a disabled Select list, which probably means putting the disabled check in the constructor.

@asolntsev thoughts?

[asolntsev]

@titusfortner Hi. I would say that

1. Yes, getOptions() should include disabled options. Users might want to check that some of them are disabled and some not.
2. Yes, deselectAll() should work if there is a disabled option. As a result of this operation, all options become disabled - that's exactly what user needs.

But if the whole <select> is disabled then probably these methods could even throw an exception, yes.

[titusfortner]

Ruby - f207270082d762764fd0b05328f87eb6ea5efcbe Python - e2bbb54153370ee56b43e7260aef5923ec826be4 .NET - fa85effa0e77d946a3f4dcc94a9d0b6b407c2749

@harsha509 needs to help me with the JS, because what I'm doing isn't working. :) https://github.com/SeleniumHQ/selenium/commit/ab017bb4c3aa335fad76963c8919d4af2d3c71a2

[harsha509]

landed in JS with pr https://github.com/SeleniumHQ/selenium/pull/11029!

Closing as implemented in all language bindings!

[volnoboy-broadcom]

Good day @asolntsev and @titusfortner, I am using python bindings and these changes broke our tests. Please write about such changes in the changelog of all affected languages. It took me some time to figure out that the Select class was updated since nothing is written about it in the changelog.

Our use-case is simple, if the Select is disabled, it should have a default option selected. We check that the proper option is selected by default.

As @asolntsev have written

"Yes, getOptions() should include disabled options. Users might want to check that some of them are disabled and some no."

I think it's a totally valid case to check what option is selected in the disabled mode. The problem is that in Python the change was added inside the constructor. There is no way to check the options, because I cannot initiate an object.

class Select:
    def __init__(self, webelement) -> None:
        if webelement.tag_name.lower() != "select":
            raise UnexpectedTagNameException(
                "Select only works on <select> elements, not on <%s>" %
                webelement.tag_name)
        if not webelement.is_enabled():
            raise NotImplementedError("Select element is disabled and may not be used.")
        self._el = webelement

[asolntsev]

@titusfortner Seems reasonable, I suggest registering a bug in python bindings.

[ShaheedHaque]

I think it's a totally valid case to check what option is selected in the disabled mode. The problem is that in Python the change was added inside the constructor. There is no way to check the options, because I cannot initiate an object.

@titusfortner, @asolntsev I have a variant of the problem noted by @volnoboy-broadcom; I just want to examine the value of a disabled <select>, and the NotImplementedError("Select element is disabled and may not be used.") in the constructor prevents this. I too would like this part of 0eb286a8fd of reverted/redone.

[titusfortner]

This class is for selecting elements. You don't need it to get values.

WebElement selectElement = driver.findElement(By.name("selectomatic"));
selectElement.getAttribute(desiredValue);
selectElement.findElement(By.cssSelector(option[attribute=attributeValue])).getAttribute(desiredValue)

[asolntsev]

@titusfortner yes and no. :)

1. From one side, javadoc talks only about selecting/deselecting options":

Models a SELECT tag, providing helper methods to select and deselect options.

2. From other side, class Select has methods getWrappedElement(), isMultiple(), getOptions(), getAllSelectedOptions() which all are totally valid methods for a disabled select.

I don't see any reasons why cannot user check those, e.g. assertThat(select.getAllSelectedOptions()).isEmpty();.

[titusfortner]

That's the point, though. This class is opinionated and is a shortcut for working with Select objects in a specific way. If the object is not enabled you literally can't work with it, so it doesn't make sense to support that functionality with this class. We provide the normal classes for working with elements, including select and option elements in an unopinionated fashion where you can query information.

This is literally the conversation I've been having about how Selenium is different from Cypress/Playwright and that "Selenium isn't a testing tool" because the classes that aren't in Support package are not opinionated. If you want different opinions from what is in the Select class, then build your own opinionated classes based on what is available in Selenium.

[ShaheedHaque]

This class is for selecting elements. You don't need it to get values.

First, consider then that the class used to be perfectly usable for getting values. And as your example shows very well:

WebElement selectElement = driver.findElement(By.name("selectomatic"));
selectElement.getAttribute(desiredValue);
selectElement.findElement(By.cssSelector(option[attribute=attributeValue])).getAttribute(desiredValue)

the WebElement is a clumsy way to get the things that this class used to make easy.

Second, if the intent of the exception in the constructor is to enforce an opinion as to the behaviour (or access to the behaviour) of other methods, then what should I expect to happen if the element is enabled on construction of this class, but then disabled afterwards? Do the methods being protected suddenly become un-opionated? Isn't is necessary to do the check on invocation of each method in any event?

You are the maintainer, and surely you'll make the call, but I hope you reconsider the exception on construction.

[emanlove]

Being a bit picky here but NotImplementedError seems like the wrong exception to throw. The other uses of this error are actually when there is a subclass and that subclass has not been implemented. Instead I would expect to sound more like ElementDisabledException and found within selenium.common.exceptions.

I think @ShaheedHaque raises some good points within

Second, if the intent of the exception in the constructor is to enforce an opinion as to the behaviour (or access to the behaviour) of other methods, then what should I expect to happen if the element is enabled on construction of this class, but then disabled afterwards? Do the methods being protected suddenly become un-opionated? Isn't is necessary to do the check on invocation of each method in any event?

my emphasis on what I see starts to become a core question.

[titusfortner]

I discussed with @symonk about using NotImplementedError. It's being used here because that's the error that is getting used in similar places elsewhere in Selenium code. It isn't the right error, but fixing it is outside the scope of this particular feature because it requires more comprehensive changes.

Support package classes are optional/additional/add-on/informational/suggestions. It is not core functionality; it is wrappers of core functionality, designed to show people what is possible not to be objective truth. There is nothing special about its implementation compared to any other implementation other than being distributed by Selenium. If anyone doesn't like the implementation provided, please please copy/paste this free and open code and make one that works better for you. I'm a little cavalier with this because I'd almost prefer to delete these classes entirely than maintain them; they are straddling the line of what Selenium has decided to be and not to be, and maintaining them just adds confusion about Selenium's intentions.

Of course @ShaheedHaque raises some good points, but the scenario being discussed is not what this class was designed for, and choosing to support it is just as arbitrary as choosing to prevent people from using the class in an invalid state. All of these are "opinions" in the way I'm speaking of them. There are multiple scenarios (e.g., select element is then isn't enabled) and different ways to handle each and tradeoffs for each choice. No one implementation can be correct and performant for every possible scenario. So, because this is a class in the Support package we pick one. The one we picked recently is slightly different than the one we picked before because I think it better represents what Selenium should be doing with the Support classes relative to its core classes.

That said, I'll bring this up at the next TLC meeting in case the rest of the team disagrees with me.

[asolntsev]

@titusfortner I would ask a practical question: what problem you are trying to solve by moving the disableness to Select constructor? The only achievement I see is "forbid people use it". Well, few less lines of code.

Both are not users' problems.

If the change doesn't solve any real problems, is it a good change?

[titusfortner]

Yes, I'm making a point about what Support classes are designed to be. It sounds like Selenide uses these classes directly instead of having its own implementation, though?

[asolntsev]

I'm making a point about what Support classes are designed to be.

Yes, currently Selenide uses few classes from org.openqa.selenium.support package. But I think we can easily replace classes like Select with our own implementation if needed.

[titusfortner]

Let's discuss at next TLC meeting (Thursday 5pm your time, I think, in selenium-tlc slack channel).

[papegaaij]

I think it makes perfect sense for Select to check if the select element and its options are enabled, but only when interaction is requested on these elements. A select does have a selection when its disabled. The original request was about not being able to select disabled options, which seems like a valid request to me. The change does not implement that request. It is still possible to select disabled options, but now it is no longer possible to instantiate a Select when the select is disable at that moment. Even worse, it is now possible to operate on a disabled select, when it is disabled after instantiation of the Select. This wasn't possible before.

This change broke our tests, because we use Select to read the selected value of a select. In some cases the element is disabled. I had to change the code like this, which is not an improvement IMHO:

- return new Select(field).getFirstSelectedOption().getAttribute("value");
+ return field.findElements(By.tagName("option"))
+     .stream()
+     .filter(WebElement::isSelected)
+     .findFirst()
+     .get()
+     .getAttribute("value");

[titusfortner]

We agreed at the TLC meeting to restore this functionality for now. If we decide to make this change in the future, we'll mark it as deprecated first.