Catch the NoSuchElementException if the element is not present in DOM

[focbenz]

The driver.findElement(locator) method can produce a NoSuchElementException which is currently not handled by the visibilityOfElementLocated method. The proposed change also catches the NoSuchElementException if the element is not present in DOM thrown by findElement(locator) and return null in method visibilityOfElementLocated.

If there is no argument against returning null for the visibilityOfElementLocated method in case the element with locator is not present in the DOM this take care of the unhandled NoSuchElementException that might occur because of timing problems.

• By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement

[GQAssurance]

I believe failing to throw the NoSuchElementException is deliberate. The calling functions are meant to handle that exception.

[mills-andrew]

I have to agree with GQAssurance, not throwing the exception allows the user to properly catch exceptions. If you application requires this sort of change, i suggest building helper classes that will handle this exception for you.

[focbenz]

The pull request came from using Testbench with Vaadin and we already have duplicated the method from Selenium code in our tests - so this was just an improvement I wanted to share.
I was already expecting blowback for the pull request and instead pointers towards using presenceOfElementLocated, but while checking out other methods I saw "better" error handling dealing with the fact that an element was not present while checking for visibility.

If you take a look at the method invisibilityOfElementLocated also the case where the element is not actually in the DOM is gracefully handled by catching the NoSuchElementException:

} catch (NoSuchElementException e) {
  // Returns true because the element is not present in DOM. The
  // try block checks if the element is present but is invisible.
  return true;
} catch (StaleElementReferenceException e) {
  // Returns true because stale element reference implies that element
  // is no longer visible.
  return true;
}

[gauravrajput44]

I am kind of agree with this change

[GQAssurance]

@focbenz I didn't intend for my comment to look like "blowback." It's just that the response of ECs to return WebExceptions was decided long ago, for better or for worse.

If anything, I'd suggest that invisibilityOfElementLocated is the function which is misdesigned, because it appears to have been built to expect 2 distinct conditions: both the invisibility of an existing element, -AND- the non-presence of an element.

I also would prefer that ECs never return webExceptions (but, instead, simply return false). However, making such a change would have significant magnitude. Presumably it should be discussed fully as a feature request (not in a pull request) and should require redesigning many ECs to maintain consistency.

At the very least, this change if accepted as-is would break years of code built on the assumption that an exception is returned when this EC is tested. This certainly warrants some discussion.

[focbenz]

I fully understand that such major decisions have been made before I or we stumbled upon such a problem in just one of the visibility methods offered. Since I am not working with the code on a day to day base and cannot turn this into a feature request maybe this can be included by someone else into a better suited feature request.

[GQAssurance]

@focbenz understood.

Do you have any thoughts on my suggestion that invisibilityOfElementLocated is in fact the incorrect EC? As it offers less precision than its counterpart.

For what it's worth, I imagine you'd have to work with the Selenium elders for quite a while to gain acceptance of this kind of fundamental change. I am also simply a user like yourself, but I felt like replying so you would not feel your PR was being ignored for no reason.

[focbenz]

No issues with the feedback - happy to also get some deeper insight.

Do you have any thoughts on my suggestion that invisibilityOfElementLocated is in fact the incorrect EC? As it offers less precision than its counterpart.

We are actually trying to get the visibility of the object confirmed, but in case the application is not behaving correctly - mostly due to timing problems - the object is not part of the DOM tree and wrapping this in a try catch NoSuchElementException block is the better option.

[shs96c]

Hi. Sorry for the delay in reviewing this. It's kicked off a very active conversation with many of the core committers, and we've been slow to make a decision, and I've been awful at communicating that. My sincere apologies. I'll do a final review tomorrow, but I'm inclined to merge it so that the behaviour of this expected condition is the same in WebDriverWait as in FluentWait

[GQAssurance]

Hi. Sorry for the delay in reviewing this. It's kicked off a very active conversation with many of the core committers, and we've been slow to make a decision, and I've been awful at communicating that. My sincere apologies. I'll do a final review tomorrow, but I'm inclined to merge it so that the behaviour of this expected condition is the same in WebDriverWait as in FluentWait

Is there a forum to view the discussion, for those of us who are curious?

[barancev]

I just want to add that the same logic should be expanded to other similar conditions.

[CLAassistant]

All committers have signed the CLA.

[okyrylenko]

any updates on this?

[focbenz]

Awesome ... that was a sudden decision in the end!