Count positional selectors in prevAll(), parents() and nextAll() in walk order

CHANGELOG.md

@@ -58,6 +58,7 @@ QueryPath Changelog
 - Consolidate the sibling and ancestor walks behind `next()`, `prev()`, `nextAll()`, `prevAll()`, `nextUntil()`, `prevUntil()`, `parent()`, `parents()`, `parentsUntil()` and `closest()` into one helper, so a fix to one no longer has to be repeated in its mirror (issue #115). As a side effect, `parentsUntil()` no longer loops forever on an element that is not attached to the document, and `nextUntil('')` and `prevUntil('')` treat an empty selector as no selector, as the other methods already did, instead of throwing `QueryPath\CSS\ParseException`. `closest('')` returns an empty set, as in jQuery. `nextUntil()`, `prevUntil()` and `parentsUntil()` also stop at an element already collected from an earlier starting element, as `nextAll()` and `prevAll()` do, so `nextUntil()` on each of 2000 siblings falls from about 100 ms to 1 ms
 - **Behaviour change.** `prevAll()`, `prevUntil()`, `nextAll()`, `nextUntil()`, `siblings()`, `parent()` and `closest()` now return the results for a multi-element set in jQuery's order, instead of grouped by starting element (issue #117). `prevAll()` and `prevUntil()` return reverse document order, as `parents()` and `parentsUntil()` already did, and the others return document order. `qp($xml, 'c')->add('d')->prevAll()` now returns `c, b, a` rather than `b, a, c`. `next()` and `prev()` keep the order of the starting elements, as in jQuery, and the results for a single starting element are unchanged
 - **Behaviour change.** A positional pseudo-class (`:first`, `:eq()`, `:last`, ...) in the stop selector of `nextUntil()`, `prevUntil()` and `parentsUntil()` now counts across the whole document, as jQuery's does, instead of seeing each sibling or ancestor as a one-element set (issue #118). `nextUntil(':first')` used to stop at once and now walks to the end, and `nextUntil(':eq(3)')` stops at the fourth element in the document. `closest()` no longer matches a positional selector at all, as in jQuery, where `closest(':first')` used to return the element itself. `next()`, `prev()` and `parent()` still test each candidate on its own, which agrees with jQuery for a single starting element
+- **Behaviour change.** A positional pseudo-class in the selector of `prevAll()`, `parents()` and `nextAll()` now counts in walk order, before the results are sorted, as jQuery does (issue #134). `qp($xml, '#e')->prevAll(':first')` returns the nearest previous sibling, `d`, instead of the first one, `a`, and `parents(':first')` returns the parent. For a set of more than one element, the walks from each element are joined in match-set order, repeats included, and counted along that list. Add `QueryPath\CSS\DOMTraverser::filterList()`, which filters a list of nodes counting positional pseudo-classes in the order given
 
 # 4.2.0
 

src/CSS/DOMTraverser.php

@@ -98,6 +98,13 @@ class DOMTraverser implements Traverser
 	 */
 	protected $resolvingIndex;
 
+	/**
+	 * The candidates in the order filterList() counts them, or NULL to count in document order.
+	 *
+	 * @var DOMNode[]|null
+	 */
+	protected $candidateList;
+
 	/**
 	 * Build a new DOMTraverser.
 	 *
@@ -195,18 +202,74 @@ public static function filterNodes(
 		$selector,
 		$scopeNode = null,
 		?PseudoClass $psHandler = null
+	): SplObjectStorage {
+		return self::filterInOrder($nodes, null, $selector, $scopeNode, $psHandler);
+	}
+
+	/**
+	 * As filterNodes(), but positional pseudo-classes on the subject count along the list in the
+	 * order given, repeats included, as jQuery.filter() counts a traversal's walk.
+	 *
+	 * @param DOMNode[]        $nodes
+	 *   The candidates, all from one document, in the order to count them.
+	 * @param string           $selector
+	 *   A CSS selector.
+	 * @param DOMNode|null     $scopeNode
+	 * @param PseudoClass|null $psHandler
+	 *
+	 * @return SplObjectStorage
+	 *   The matching nodes, once each, in the order they first appear in the list.
+	 * @throws ParseException
+	 */
+	public static function filterList(
+		array $nodes,
+		$selector,
+		$scopeNode = null,
+		?PseudoClass $psHandler = null
+	): SplObjectStorage {
+		$unique = new SplObjectStorage();
+		foreach ($nodes as $node) {
+			$unique->offsetSet($node);
+		}
+
+		return self::filterInOrder($unique, array_values($nodes), $selector, $scopeNode, $psHandler);
+	}
+
+	/**
+	 * Match the given nodes themselves against a selector, returning them in the order given.
+	 *
+	 * @param SplObjectStorage $nodes
+	 * @param DOMNode[]|null   $list
+	 *   The order to count positional pseudo-classes in, or NULL for document order.
+	 * @param string           $selector
+	 * @param DOMNode|null     $scopeNode
+	 * @param PseudoClass|null $psHandler
+	 *
+	 * @return SplObjectStorage
+	 * @throws ParseException
+	 */
+	private static function filterInOrder(
+		SplObjectStorage $nodes,
+		?array $list,
+		$selector,
+		$scopeNode,
+		?PseudoClass $psHandler
 	): SplObjectStorage {
 		if (count($nodes) === 0) {
 			return new SplObjectStorage();
 		}
 
-		$traverser = new self($nodes, true, $scopeNode, $psHandler);
-		$groups    = $traverser->psHandler->parse($selector);
-		$matched   = $traverser->findGroups($groups)->matches();
+		$traverser                = new self($nodes, true, $scopeNode, $psHandler);
+		$traverser->candidateList = $list;
+		$groups                   = $traverser->psHandler->parse($selector);
+		$matched                  = $traverser->findGroups($groups)->matches();
 
 		// A single group without a set filter is matched in the order given. Otherwise the result
 		// is in document order, or a union of groups, and has to be put back in input order.
-		if (count($matched) < 2 || (count($groups) === 1 && ! $traverser->groupIsSetFiltered($groups->toArray()[0]))) {
+		$inOrder = $list === null
+			&& count($groups) === 1
+			&& ! $traverser->groupIsSetFiltered($groups->toArray()[0]);
+		if (count($matched) < 2 || $inOrder) {
 			return $matched;
 		}
 
@@ -330,7 +393,7 @@ protected function resolveSubject(array $selectorGroup, int $index): SplObjectSt
 		// Initialize matches if necessary. A caller-supplied set may hold non-element nodes,
 		// which can never match a selector.
 		if ($index === 0 && $this->initialized) {
-			$candidates = array_filter(iterator_to_array($this->matches, false), function ($node) {
+			$candidates = array_filter($this->candidateList ?? iterator_to_array($this->matches, false), function ($node) {
 				return $node instanceof DOMElement;
 			});
 		} else {
@@ -369,7 +432,9 @@ protected function resolveSubject(array $selectorGroup, int $index): SplObjectSt
 
 		$this->resolvingIndex = $previous;
 
-		$matched = Util::sortDocumentOrder($matched);
+		if ($index !== 0 || $this->candidateList === null) {
+			$matched = Util::sortDocumentOrder($matched);
+		}
 		foreach ($setFilters as $pseudoClass) {
 			$matched = $this->applySetFilter($matched, $pseudoClass);
 		}
@@ -398,7 +463,7 @@ protected function applySetFilter(array $nodes, array $pseudoClass): array
 		// :not() and :matches() whose argument uses a positional filter.
 		// The argument has to see the whole result set, exactly as jQuery's
 		// :not(:first) does, so run it over this set and keep or drop the result.
-		$inner = self::filterNodes($this->toMatches($nodes), $value, $this->scopeNode, $this->psHandler);
+		$inner = self::filterList($nodes, $value, $this->scopeNode, $this->psHandler);
 		if ($name !== 'not') {
 			return iterator_to_array($inner, false);
 		}

src/Helpers/QueryFilters.php

@@ -3,6 +3,7 @@
 namespace QueryPath\Helpers;
 
 use DOMElement;
+use QueryPath\CSS\DOMTraverser;
 use QueryPath\CSS\DOMTraverser\Util;
 use QueryPath\CSS\ParseException;
 use QueryPath\DOMQuery;
@@ -726,7 +727,8 @@ public function parent($selector = null): Query
 	/**
 	 * Get all ancestors of each element in the DOMQuery.
 	 *
-	 * If a selector is present, only matching ancestors will be retrieved.
+	 * If a selector is present, only matching ancestors will be retrieved. As in jQuery, a
+	 * positional selector counts from the nearest ancestor, so `parents(':first')` is the parent.
 	 *
 	 * @param string $selector
 	 *  A valid CSS 3 Selector.
@@ -741,7 +743,7 @@ public function parent($selector = null): Query
 	 */
 	public function parents($selector = null): Query
 	{
-		return $this->inReverseDocumentOrder($this->walk('parentNode', null, false), $selector);
+		return $this->inReverseDocumentOrder($this->walkAll('parentNode', $selector));
 	}
 
 	/**
@@ -793,7 +795,7 @@ public function next($selector = null): Query
 	 */
 	public function nextAll($selector = null): Query
 	{
-		return $this->inDocumentOrder($this->walk('nextSibling', null, false), $selector);
+		return $this->inDocumentOrder($this->walkAll('nextSibling', $selector));
 	}
 
 	/**
@@ -825,7 +827,8 @@ public function prev($selector = null): Query
 	 * Get the previous siblings for each element in the DOMQuery.
 	 *
 	 * For each element in the DOMQuery, get all previous siblings. If a
-	 * selector is provided, only matching siblings will be retrieved.
+	 * selector is provided, only matching siblings will be retrieved. As in jQuery, a
+	 * positional selector counts from the nearest sibling, so `prevAll(':first')` is prev().
 	 *
 	 * @param string $selector
 	 *  A valid CSS 3 selector.
@@ -842,7 +845,7 @@ public function prev($selector = null): Query
 	 */
 	public function prevAll($selector = null): Query
 	{
-		return $this->inReverseDocumentOrder($this->walk('previousSibling', null, false), $selector);
+		return $this->inReverseDocumentOrder($this->walkAll('previousSibling', $selector));
 	}
 
 	/**
@@ -1000,6 +1003,38 @@ private function walk(string $axis, $selector, bool $first, bool $inclusive = fa
 		return $found;
 	}
 
+	/**
+	 * Walk from each matched element to the end of a DOM axis, keeping the elements that match
+	 * an optional selector.
+	 *
+	 * As in jQuery, a positional selector counts along the walks joined in walk order, so
+	 * prevAll(':first') is the nearest previous sibling. An element reached from more than one
+	 * starting element is counted once per walk.
+	 *
+	 * @param string      $axis
+	 * @param string|null $selector
+	 *
+	 * @return SplObjectStorage
+	 * @throws ParseException
+	 */
+	private function walkAll(string $axis, $selector): SplObjectStorage
+	{
+		if (empty($selector) || ! Util::selectorHasPositionalPseudoClass($selector)) {
+			return $this->filterCandidates($this->walk($axis, null, false), $selector);
+		}
+
+		$walked = [];
+		foreach ($this->matches as $m) {
+			for ($n = $m->{$axis}; $n !== null && $n->nodeType !== XML_DOCUMENT_NODE; $n = $n->{$axis}) {
+				if ($n->nodeType === XML_ELEMENT_NODE) {
+					$walked[] = $n;
+				}
+			}
+		}
+
+		return DOMTraverser::filterList($walked, $selector);
+	}
+
 	/**
 	 * Wrap a traversal's results, optionally filtered by a selector, in document order.
 	 *

tests/Issues/Issue118Test.php

@@ -71,18 +71,18 @@ public function testNextPrevAndParentTestAPositionalSelectorPerNode()
 	}
 
 	/**
-	 * The set methods count a positional selector within the set, in document order
-	 * (issue #66), whatever order the results come back in (issue #117). jQuery counts
-	 * in walk order, so for prevAll() and parents() its :first is the nearest node.
+	 * The set methods count a positional selector along the walk, before the results are
+	 * sorted (issue #134), so for prevAll() and parents() :first is the nearest node.
+	 * siblings() still counts in document order.
 	 */
-	public function testSetMethodsCountPositionsInDocumentOrder()
+	public function testSetMethodsCountPositionsInWalkOrder()
 	{
-		$this->assertSame('a', $this->idList($this->selectIds(self::SIBLINGS, 'e')->prevAll(':first')));
-		$this->assertSame('d', $this->idList($this->selectIds(self::SIBLINGS, 'e')->prevAll(':last')));
-		$this->assertSame('b', $this->idList($this->selectIds(self::SIBLINGS, 'c', 'e')->prevAll(':eq(1)')));
-		$this->assertSame('b', $this->idList($this->selectIds(self::SIBLINGS, 'c', 'a')->nextAll(':first')));
+		$this->assertSame('d', $this->idList($this->selectIds(self::SIBLINGS, 'e')->prevAll(':first')));
+		$this->assertSame('a', $this->idList($this->selectIds(self::SIBLINGS, 'e')->prevAll(':last')));
+		$this->assertSame('a', $this->idList($this->selectIds(self::SIBLINGS, 'c', 'e')->prevAll(':eq(1)')));
+		$this->assertSame('b', $this->idList($this->selectIds(self::SIBLINGS, 'a', 'c')->nextAll(':first')));
 		$this->assertSame('a', $this->idList($this->selectIds(self::SIBLINGS, 'd', 'b')->siblings(':first')));
-		$this->assertSame('r', $this->idList($this->selectIds(self::ANCESTORS, 'u1', 'u2')->parents(':first')));
-		$this->assertSame('t2', $this->idList($this->selectIds(self::ANCESTORS, 'u1', 'u2')->parents(':last')));
+		$this->assertSame('t1', $this->idList($this->selectIds(self::ANCESTORS, 'u1', 'u2')->parents(':first')));
+		$this->assertSame('r', $this->idList($this->selectIds(self::ANCESTORS, 'u1', 'u2')->parents(':last')));
 	}
 }

tests/Issues/Issue134Test.php

@@ -0,0 +1,145 @@
+<?php
+/**
+ * @file
+ *
+ * Regression tests for https://github.com/GravityPDF/querypath/issues/134
+ *
+ * jQuery filters a traversal's results in walk order, before sorting them: it joins
+ * the walk from each starting element, repeats included, filters that list, then
+ * removes the repeats and sorts. A positional selector in prevAll(), parents() and
+ * nextAll() counts along that list. The expected values are what jQuery 3.7.1's
+ * algorithm gives for a set in the same order.
+ */
+
+namespace QueryPathTests;
+
+use DOMXPath;
+use QueryPath\CSS\DOMTraverser;
+
+class Issue134Test extends TestCase
+{
+	// Document order: r, a, b, c, d, e.
+	private const SIBLINGS = '<?xml version="1.0"?><r><a id="a"/><b id="b"/><c id="c"/><d id="d"/><e id="e"/></r>';
+
+	// Document order: r, s1, t1, u1, s2, t2, u2.
+	private const ANCESTORS = '<?xml version="1.0"?><r id="r"><s id="s1"><t id="t1"><u id="u1"/></t></s><s id="s2"><t id="t2"><u id="u2"/></t></s></r>';
+
+	/**
+	 * @testWith [":first", "d"]
+	 *           [":last", "a"]
+	 *           [":eq(0)", "d"]
+	 *           [":eq(1)", "c"]
+	 *           [":eq(-1)", "a"]
+	 *           [":lt(2)", "d,c"]
+	 *           [":gt(1)", "b,a"]
+	 *           [":odd", "c,a"]
+	 *           [":even", "d,b"]
+	 *           ["*:not(:first)", "c,b,a"]
+	 *           [":matches(:last)", "a"]
+	 *           ["b, :first", "d,b"]
+	 */
+	public function testPrevAllCountsFromTheNearestSibling($selector, $expected)
+	{
+		$this->assertSame($expected, $this->idList(qp(self::SIBLINGS, '#e')->prevAll($selector)));
+	}
+
+	/**
+	 * The walks from c and e join as b, a, d, c, b, a.
+	 *
+	 * @testWith [":first", "b"]
+	 *           [":last", "a"]
+	 *           [":eq(2)", "d"]
+	 *           [":eq(4)", "b"]
+	 *           [":lt(3)", "d,b,a"]
+	 *           [":gt(3)", "b,a"]
+	 *           [":odd", "c,a"]
+	 */
+	public function testPrevAllOnASetCountsAlongTheJoinedWalks($selector, $expected)
+	{
+		$this->assertSame($expected, $this->idList($this->selectIds(self::SIBLINGS, 'c', 'e')->prevAll($selector)));
+	}
+
+	/**
+	 * @testWith ["u2", ":first", "t2"]
+	 *           ["u2", ":last", "r"]
+	 *           ["u2", ":eq(1)", "s2"]
+	 *           ["u2", "s:first", "s2"]
+	 *           ["u1,u2", ":first", "t1"]
+	 *           ["u1,u2", ":eq(3)", "t2"]
+	 *           ["u1,u2", ":lt(2)", "t1,s1"]
+	 *           ["u1,u2", ":gt(3)", "s2,r"]
+	 *           ["u1,u2", "s:last", "s2"]
+	 */
+	public function testParentsCountsFromTheNearestAncestor($ids, $selector, $expected)
+	{
+		$query = $this->selectIds(self::ANCESTORS, ...explode(',', $ids));
+
+		$this->assertSame($expected, $this->idList($query->parents($selector)));
+	}
+
+	/**
+	 * The walks from a and c join as b, c, d, e, d, e.
+	 *
+	 * @testWith [":first", "b"]
+	 *           [":eq(1)", "c"]
+	 *           [":eq(4)", "d"]
+	 *           [":last", "e"]
+	 */
+	public function testNextAllOnASetCountsAlongTheJoinedWalks($selector, $expected)
+	{
+		$this->assertSame($expected, $this->idList($this->selectIds(self::SIBLINGS, 'a', 'c')->nextAll($selector)));
+	}
+
+	/**
+	 * The walks are joined in match-set order, as jQuery joins them in its own set's order. add()
+	 * keeps the order elements were added in, where jQuery's sorts them into document order.
+	 */
+	public function testWalksAreJoinedInMatchSetOrder()
+	{
+		$this->assertSame('d', $this->idList($this->selectIds(self::SIBLINGS, 'c', 'a')->nextAll(':first')));
+		$this->assertSame('d', $this->idList($this->selectIds(self::SIBLINGS, 'e', 'c')->prevAll(':first')));
+	}
+
+	public function testSelectorsWithoutAPositionAreUnchanged()
+	{
+		$this->assertSame('d,c,b,a', $this->idList(qp(self::SIBLINGS, '#e')->prevAll()));
+		$this->assertSame('c,a', $this->idList(qp(self::SIBLINGS, '#e')->prevAll('a, c')));
+		$this->assertSame('t2,s2,t1,s1', $this->idList($this->selectIds(self::ANCESTORS, 'u1', 'u2')->parents('s, t')));
+		$this->assertSame('d,e', $this->idList($this->selectIds(self::SIBLINGS, 'c', 'a')->nextAll('d, e')));
+	}
+
+	/**
+	 * The stop selector of prevUntil() and parentsUntil() already counts across the document
+	 * (issue #118), so walk order does not affect it.
+	 */
+	public function testUntilStopsAreUnaffected()
+	{
+		$this->assertSame('d,c', $this->idList(qp(self::SIBLINGS, '#e')->prevUntil(':eq(2)')));
+		$this->assertSame('t2', $this->idList(qp(self::ANCESTORS, '#u2')->parentsUntil(':eq(4)')));
+	}
+
+	public function testFilterListCountsInTheOrderGiven()
+	{
+		$xpath = new DOMXPath(qp(self::SIBLINGS)->document());
+		$nodes = [];
+		foreach (['d', 'c', 'b', 'c'] as $id) {
+			$nodes[] = $xpath->query("//*[@id='$id']")->item(0);
+		}
+
+		$ids = function ($storage) {
+			$found = [];
+			foreach ($storage as $node) {
+				$found[] = $node->getAttribute('id');
+			}
+
+			return implode(',', $found);
+		};
+
+		$this->assertSame('d', $ids(DOMTraverser::filterList($nodes, ':first')));
+		$this->assertSame('c', $ids(DOMTraverser::filterList($nodes, ':last')));
+		$this->assertSame('c', $ids(DOMTraverser::filterList($nodes, ':eq(3)')));
+		$this->assertSame('d,c,b', $ids(DOMTraverser::filterList($nodes, '*')));
+		$this->assertSame('c,b', $ids(DOMTraverser::filterList($nodes, ':gt(0)')));
+		$this->assertSame('', $ids(DOMTraverser::filterList([], ':first')));
+	}
+}