Add ability to check if a NodeElement is disabled

[evangelion1204]

Hi,

I would like to implement a method called isDisabled for the NodeElement to check if an element is disabled or not. This can come quite handy because in AjaxRequest you often disable Buttons/Elements. This could be done quite analog to isChecked. Is this new function wished and how could it be named? isDisabled or isEnabled - cause you normally add the attribute disabled to an element isDisabled seems more logical, from the properties isEnabled.

Thanks,

Michael

[aik099]

:+1:

[aik099]

@stof , any thoughts about the correct naming?

[stof]

I'm not sure this method is really needed. It is a matter of checking the disabled attribute (isChecked is different as its state will change when manipulating the form)

[aik099]

But you can change disabled state from JS and then getAttribute('disabled') will return value from HTML attribute that was present on initial page load.

Maybe we should introduce getNodeProperty method (will do same as $('element').prop('name')) does. And change the isChecked to use that. Because right now we have getAttribute for retrieving HTML attributes of DOM node, but we don't have any method for retrieving JS properties of a node. For BrowserKit driver it will fallback to getAttribute.

[evangelion1204]

As far as I know disabled is not an attribute, the initial value is set via the attribute but it is a property, some browser may change it if the property changes. But normally this should not be the case. A workaround via attribute is possible but may result in wrong checks.

getNodeProperty would be even better, cause it would work with every property.

[aik099]

Then in the NodeElement we can create method isEnabled which will just call !...->getDriver()->getNodeProperty($this->getXpath(), 'disabled').

[stof]

The issue with getNodeProperty is that we cannot implement it in BrowserKitDriver. The fallback to the HTML attribute is only true for a few DOM properties. It is far from being a general case

[aik099]

Then we don't implement it in MinkBrowserKitDriver. Since getNodeProperty uses JS to work then it will only be implemented in JS-aware drivers. I think we can live with that.

Yes, idea with fallback was wrong one.

[stof]

@aik099 but isDisabled is something which could make sense in MinkBrowserKitDriver

[aik099]

Now I'm confused. First you say that no such thing as isDisabled is needed because it can be checked via ->getAttribute('disabled'). Now you say that suddenly MinkBrowserKitDriver needs it as well.

Then we're back at initial subject of discussion: how to name the method on the driver: isDisabled or isEnabled. JS-aware drivers will use JS for it and MinkBrowserKitDriver will do just getAttribute('disabled').

And getNodeProperty will be JS-aware drivers only feature then.

[stof]

no, I'm saying that if you decide to include it, you cannot implement it using a generic getNodeProperty which would need not work in non-JS drivers

[evangelion1204]

I think the problem is that the line ...->getDriver()->getNodeProperty($this->getXpath(), 'checked'). is in the NodeElement not the Driver itself, the getNodeProperty function will not be implemented in non JS-aware drivers and not work anymore.

[aik099]

no, I'm saying that if you decide to include it, you cannot implement it using a generic getNodeProperty which would need not work in non-JS drivers

Then I can see the light at the end of a tunnel. As told in https://github.com/Behat/Mink/issues/521#issuecomment-42435990 we just do 2 separate methods and all drivers will be happy. In turn the getNodeProperty can be used to implement isDisabled and isChecked for all JS-based drivers.

[aik099]

I think the problem is that the line ...->getDriver()->getNodeProperty($this->getXpath(), 'checked'). is in the NodeElement not the Driver itself, the getNodeProperty function will not be implemented in non JS-aware drivers and not work anymore.

You're wrong. Latest code from NodeElement:

public function isChecked()
{
    return (Boolean) $this->getSession()->getDriver()->isChecked($this->getXpath());
}

[stof]

Then I can see the light at the end of a tunnel. As told in #521 (comment) we just do 2 separate methods and all drivers will be happy. In turn the getNodeProperty can be used to implement isDisabled and isChecked for all JS-based drivers.

Selenium1 and Selenium2 at least have a native method to check isChecked. I don't think making them use a custom JS evaluation rather than the driver method is a good idea.

[aik099]

Selenium1 and Selenium2 at least have a native method to check isChecked

I went too far with improvements then. We'll keep native method call of course.

[evangelion1204]

This sounds fine, because isDisabled is more than enough in the moment.

Still a getNodeProperty method would be nice too, this could just be implemented for all drivers supporting it. But will not be used by isChecked or isDisabled.

[aik099]

We can use getNodeProperty for drivers that has it inside the driver itself to get disabled property value, why not.

[stof]

I would not add getNodeProperty. I would rather go for #499 which is much more powerful than getting only a DOM property.

[aik099]

Agreed.

[aik099]

@evangelion1204 , you can send PR's now ;)

[TerjeBr]

Is there any progress on adding the "isDisabled" method. (I was needing it today.)

[aik099]

None. If any progress would happen, then it will be on #499.

[TerjeBr]

Why does this depend on #499 ? As you have driver dependent implementations of "isChecked" you should probably have driver dependent implementations of "isDisabled".

I see #499 as just an additional "nice to have".

[TerjeBr]

@stof said in https://github.com/minkphp/Mink/issues/521#issuecomment-42497277 "I would not add getNodeProperty". I understood that to mean that "isDisabled" still could go ahead.

[aik099]

https://github.com/minkphp/Mink/issues/521#issuecomment-330497102

@TerjeBr , because in this issue discussion we've agreed that instead of just making getNodeProperty method we'd better implement a way to execute a js on a given element for all js-enabled drivers. That's what #499 is all about by the way.

[aik099]

https://github.com/minkphp/Mink/issues/521#issuecomment-330498385

@TerjeBr , the discussion happened 3 years ago. I've honestly don't remember what was it. This could also mean, that NodeElement::isDisabled method could be implemented using functionality described in #499 instead of adding new getNodeProperty method. Yet we need to have a fallback code for non-js enabled drivers too.

In any case you've welcome to send a PR.

[vijaycs85]

I might be wrong, by the look of it, we already have executeScript() and evaluateScript() in Behat\Mink\Driver\DriverInterface. All we need is an implementation of executeScript() (and fallback as mentioned by @aik099) and NodeElement::isDisabled method to use executeScript() to get disabled status. This way it wouldn't be just DOM.