In TraversableElement->findField(), provide a way to find the first visible item rather than the first item.

[alberto56]

TraversableElement->fillField(), clickLink(), etc., use findField(), findLink(), etc. to know what to click or fill.

Might it not be better to define new find functions which find only visible elements? For example, in addition to findLink(), a findVisibleLink() would cycle through all links and find the first visible one.

In turn, clickLink() would use findVisibleLink() rather than findLink() to know what to click.

Take the following use case: a website contains several links "xyz", but only one is visible at any given time. With the current method, if the first xyz is invisible, clickLink(xyz) fails. With the proposed method, the visible element could be clicked.

[neochief]

I solved this by overriding find* functions in my FeaturesContext and WebAsser classes.

[aik099]

Interesting. Since Mink does what user can do, then I think, that all mentioned methods should operate on visible elements only. I see no point filling in field, that user can't see, because user can't fill it either.

[neochief]

There's another point of view. We'd might rather have all runners behave the same way. Take a Goutte runner, as example. It's not possible to determine visibility of the element there. So, findField will behave very differently in Selenium runner and Goutte runner.

[aik099]

So find* analogs for visible elements would work only in Selenium and similar, but throw an exception in other headless browsers, like Goutte. Seems fair.

[neochief]

Right, but standard fillField(), clickLink(), etc. should stay untouched.

[aik099]

Sure, to keep backwards compatibility.

[mnapoli]

I have overridden all find* methods too, and click/fill methods too. Like it has been said, there's no point in handling hidden fields. Without "visibility filtering", I end up using a lot of unneeded CSS selectors, where I can just use labels instead.

And if a driver (Goutte, ...) can't handle elements visibility, then that's a reason not to use it (or at least use it when appropriate), but it's not a reason to draw every other driver down.

If we settle on the lowest common denominator, then a lot of methods would need to be removed (because not supported in every driver).

[aik099]

@stof , should we do something about it?

Idea of creating another set of find_, click_, etc. methods is horrible code duplication to me. Changing Mink to operate on visible elements by default is not only BC break, but also will make all headless drivers to behave differently (an exception thrown, when visibility is checked before allowing any further action).

[ratajczak]

I agree with @mnapoli

[aik099]

@mnapoli, have you sent a PR with you override code?

I guess just wrapping find, findAll calls with new removeInvisibleElements method would be enough. Or if we're 100% sure, that Mink should only allow to operate on visible elements only, then we can do this right inside findAll.

As said before, then the headless drivers still will be able to operate on invisible elements, since they can't detect what is visible.

[mnapoli]

@mnapoli, have you sent a PR with you override code?

@aik099 Nope

[ratajczak]

What do you think about something like that?

public function find($selector, $locator)
{
    $items = $this->findAll($selector, $locator);

    if (count($items) && !method_exists(current($items), 'isVisible')) {
      return current($items);
    }

    foreach ($items as $item) {
      if ($item->isVisible()) {
        return $item;
      }
    }
}

I believe that would prevent breaking backward compatibility.
I'm still not sure if it's worth at all using drivers which are not visibility aware, silly CSS bug can hide an element and scenario would pass where real user would be blocked.