Skip to content

Commit

Permalink
Fix getListOfAllNodeElementParents includes stopAt node (#341)
Browse files Browse the repository at this point in the history
* Rename getListOfAllNodeElementParents to getAncestors

* Rename nodeElement to node

* Add failing test

* Add missing param annotation for node param

* Don't modify string case on each while iteration

* Don't call getParent() twice per iteration

* Don't include stopAt node in ancestor list

* Rename nodeElements to nodes

* Make stopAt optional

* Apply StyleCI fixes

* Fix elements with body as parent are never visible

Problem:
When an element has the body as it's parent, nodeIsVisibleInViewport always returns false.

Cause:
The method is telling getAncestors() to stop at body, so the parents list is empty. Then the method is assuming that if the parents list is empty, the element is detached from the DOM.

Fix:
Check the elements parent directly instead of the filtered parents list returned from getAncestors().
  • Loading branch information
Chekote authored Apr 16, 2020
1 parent 31b0898 commit 6fc7683
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 13 deletions.
25 changes: 14 additions & 11 deletions src/Medology/Behat/Mink/FlexibleContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -1741,7 +1741,7 @@ public function nodeIsFullyVisibleInViewport(NodeElement $element)
{
$driver = $this->assertSelenium2Driver('Checks if a node Element is fully visible in the viewport.');
if (!$driver->isDisplayed($element->getXpath()) ||
count(($parents = $this->getListOfAllNodeElementParents($element, 'html'))) < 1
count(($parents = $this->getAncestors($element, 'html'))) < 1
) {
return false;
}
Expand Down Expand Up @@ -1786,9 +1786,10 @@ public function nodeIsVisibleInViewport(NodeElement $element)
{
$driver = $this->assertSelenium2Driver('Checks if a node Element is visible in the viewport.');

$parents = $this->getListOfAllNodeElementParents($element, 'body');
$parents = $this->getAncestors($element, 'body');

if (!$driver->isDisplayed($element->getXpath()) || count($parents) < 1) {
// if the element is displayed, or it is detached from the DOM, it is not visible
if (!$driver->isDisplayed($element->getXpath()) || !$element->getParent()) {
return false;
}

Expand Down Expand Up @@ -2039,22 +2040,24 @@ public function findRadioButton($label)
}

/**
* Get list of of all NodeElement parents.
* Returns all ancestors of the specified node element.
*
* @param string $stopAt html tag to stop at
* @param NodeElement $node the node element to fetch ancestors for.
* @param string|null $stopAt html tag to stop at. This node will NOT be included in the returned list.
*
* @return NodeElement[]
*/
private function getListOfAllNodeElementParents(NodeElement $nodeElement, $stopAt)
private function getAncestors(NodeElement $node, $stopAt = null)
{
$nodeElements = [];
while ($nodeElement->getParent() instanceof NodeElement) {
$nodeElements[] = ($nodeElement = $nodeElement->getParent());
if (strtolower($nodeElement->getTagName()) === strtolower($stopAt)) {
$nodes = [];
while (($node = $node->getParent()) instanceof NodeElement) {
if (strcasecmp($node->getTagName(), $stopAt) === 0) {
break;
}

$nodes[] = $node;
}

return $nodeElements;
return $nodes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@
use Behat\Mink\Session;
use Medology\Behat\Mink\FlexibleContext;
use PHPUnit_Framework_MockObject_MockObject;
use PHPUnit_Framework_TestCase;
use Tests\TestCase;

/**
* Instantiates the FlexibleContext so it can be used in Unit Tests functions.
*/
abstract class FlexibleContextTest extends PHPUnit_Framework_TestCase
abstract class FlexibleContextTest extends TestCase
{
/** @var Session|PHPUnit_Framework_MockObject_MockObject */
protected $sessionMock;
Expand Down
87 changes: 87 additions & 0 deletions tests/Medology/Behat/Mink/FlexibleContext/GetAncestorsTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
<?php

namespace Tests\Medology\Behat\Mink\FlexibleContext;

use Behat\Mink\Element\NodeElement;
use PHPUnit_Framework_MockObject_MockObject as MockObject;

class GetAncestorsTest extends FlexibleContextTest
{
private $body;
private $div;
private $list;
private $listItem;
private $button;

public function setUp()
{
parent::setUp();

// Given I have an HTML body
$this->body = $this->mockNode('body');

// And the body has a div
$this->div = $this->mockNode('div', $this->body);

// And the div has an unordered list
$this->list = $this->mockNode('ul', $this->div);

// And the list has a list item
$this->listItem = $this->mockNode('li', $this->list);

// And the list item has a button
$this->button = $this->mockNode('button', $this->listItem);
}

public function testAllAncestorsAreReturned()
{
// When I pass the button to allAncestors()
$ancestors = $this->invokeMethod($this->flexible_context, 'getAncestors', [$this->button]);

// Then all ancestors should be returned in the correct order
$this->assertCount(4, $ancestors, 'Number of returned ancestors should be 4');
$this->assertSame($this->listItem, $ancestors[0], "Button's first ancestor should be list item");
$this->assertSame($this->list, $ancestors[1], "Button's second ancestor should be list");
$this->assertSame($this->div, $ancestors[2], "Button's third ancestor should be div");
$this->assertSame($this->body, $ancestors[3], "Button's fourth ancestor should be body");
}

public function testStopAtIsNotReturned()
{
// When I pass the button to allAncestors() and request that it stop at "body"
$ancestors = $this->invokeMethod($this->flexible_context, 'getAncestors', [$this->button, 'body']);

// Then all ancestors except body should be returned in the correct order
$this->assertCount(3, $ancestors, 'Number of returned ancestors should be 3');
$this->assertSame($this->listItem, $ancestors[0], "Button's first ancestor should be list item");
$this->assertSame($this->list, $ancestors[1], "Button's second ancestor should be list");
$this->assertSame($this->div, $ancestors[2], "Button's third ancestor should be div");
}

/**
* Creates a mocked NodeElement with an optional parent.
*
* @param string $tagName the type of node element to mock
* @param NodeElement|null $parent the optional parent for the node element
* @return MockObject|NodeElement
*/
protected function mockNode($tagName, NodeElement $parent = null)
{
$node = $this->createMock(NodeElement::class);
$node->method('getTagName')->willReturn($tagName);
$node->method('getParent')->willReturn($parent);

return $node;
}

public function tearDown()
{
$this->body = null;
$this->div = null;
$this->list = null;
$this->listItem = null;
$this->button = null;

parent::tearDown();
}
}
30 changes: 30 additions & 0 deletions tests/TestCase.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

namespace Tests;

use PHPUnit_Framework_TestCase;
use ReflectionClass;
use ReflectionException;

class TestCase extends PHPUnit_Framework_TestCase
{
/**
* Call protected/private method of a class.
*
* @param object|string $object Instantiated object that we will run method on
* @param string $methodName Method name to call
* @param array $parameters array of parameters to pass into method
*
* @throws ReflectionException if there is a problem invoking the method
*
* @return mixed method return
*/
public function invokeMethod($object, $methodName, array $parameters = [])
{
$reflection = new ReflectionClass($object);
$method = $reflection->getMethod($methodName);
$method->setAccessible(true);

return $method->invokeArgs((is_string($object) ? null : $object), $parameters);
}
}

0 comments on commit 6fc7683

Please sign in to comment.