From 6fc7683e212d95a9e47a748257a233874188d4be Mon Sep 17 00:00:00 2001
From: Donald Tyler <domhnall.tilliere@gmail.com>
Date: Thu, 16 Apr 2020 14:08:01 -0500
Subject: [PATCH] Fix getListOfAllNodeElementParents includes stopAt node
 (#341)

* 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().
---
 src/Medology/Behat/Mink/FlexibleContext.php   | 25 +++---
 .../FlexibleContext/FlexibleContextTest.php   |  4 +-
 .../Mink/FlexibleContext/GetAncestorsTest.php | 87 +++++++++++++++++++
 tests/TestCase.php                            | 30 +++++++
 4 files changed, 133 insertions(+), 13 deletions(-)
 create mode 100644 tests/Medology/Behat/Mink/FlexibleContext/GetAncestorsTest.php
 create mode 100644 tests/TestCase.php

diff --git a/src/Medology/Behat/Mink/FlexibleContext.php b/src/Medology/Behat/Mink/FlexibleContext.php
index 7e105430..2637f030 100644
--- a/src/Medology/Behat/Mink/FlexibleContext.php
+++ b/src/Medology/Behat/Mink/FlexibleContext.php
@@ -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;
         }
@@ -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;
         }
 
@@ -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;
     }
 }
diff --git a/tests/Medology/Behat/Mink/FlexibleContext/FlexibleContextTest.php b/tests/Medology/Behat/Mink/FlexibleContext/FlexibleContextTest.php
index e000b698..250b602d 100644
--- a/tests/Medology/Behat/Mink/FlexibleContext/FlexibleContextTest.php
+++ b/tests/Medology/Behat/Mink/FlexibleContext/FlexibleContextTest.php
@@ -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;
diff --git a/tests/Medology/Behat/Mink/FlexibleContext/GetAncestorsTest.php b/tests/Medology/Behat/Mink/FlexibleContext/GetAncestorsTest.php
new file mode 100644
index 00000000..9db63296
--- /dev/null
+++ b/tests/Medology/Behat/Mink/FlexibleContext/GetAncestorsTest.php
@@ -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();
+    }
+}
diff --git a/tests/TestCase.php b/tests/TestCase.php
new file mode 100644
index 00000000..acea9642
--- /dev/null
+++ b/tests/TestCase.php
@@ -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);
+    }
+}