Psuedo-class selector :text producing errors and incorrect results

CHANGELOG.md

@@ -19,6 +19,10 @@ QueryPath Changelog
 - Add `composer run test:examples` (and `test:examples:network`) to run the examples locally
 - Fix processing instructions gaining an extra `?` each time an HTML-parsed document was serialized, so `<?php echo $title; ?>` came back out of `html()`, `innerHTML()`, `innerXML()`, `innerXHTML()`, `xml()`, `html5()`, `innerHTML5()`, and `writeXML()` as `<?php echo $title; ??>`. libxml's HTML parser keeps the closing `?` as part of the node's data, unlike its XML parser and the Masterminds HTML5 parser, so QueryPath now strips it on load. `DOMProcessingInstruction::$data` consequently no longer has a stray `?` on the end for documents read with `htmlqp()` or `qp()` on an `.html` file. Note that this normalisation applies only to documents QueryPath parses itself, and that taking the underlying `DOMDocument` out of QueryPath and calling libxml's own `saveHTML()` on it will emit `<?php ... >` without the terminator, since libxml's HTML serializer never adds one
 - Add `QueryPath\Document`, a `DOMDocument` subclass QueryPath parses into. The type is how QueryPath records that a document's processing instruction data does not carry the closing `?`, which cannot be determined by inspecting the document afterwards. It travels with the document, so every route to a second `DOMQuery` over one document -- iteration, `add()`, `remove()`, `replaceAll()`, `branch()`, `QueryPath::with()`, and the bundled extensions -- serializes it correctly. A `DOMDocument` supplied by the caller is a plain `DOMDocument`, makes no such promise, and is still serialized exactly as it was handed over
+- Fix fatal error when running a CSS selector against a match set that contains non-element nodes (text, comment, CDATA or processing instruction), as `contents()` returns. Those nodes now simply do not match, instead of calling element-only DOM methods on them
+- Fix `find('a > b')` throwing a `TypeError` when the left-hand side matched the root element, whose parent is the document rather than an element
+- Fix the `:text` pseudo-class so it matches jQuery: it selects `input` elements whose `type` attribute is absent or is `text` (case-insensitively). It never indicated, and still does not indicate, whether a node is a text node
+- `find('*')` now matches the nodes in the match set as well as their descendants, so that a selector can be tested against an element already in hand
 
 # 4.1.0
 

src/CSS/DOMTraverser.php

@@ -565,7 +565,10 @@ public function combineSibling($node, $selectors, $index)
 	public function combineDirectDescendant($node, $selectors, $index)
 	{
 		$parent = $node->parentNode;
-		if (empty($parent)) {
+		// The parent of the root element is the document, which is not an element and can never
+		// match. combineAdjacent(), combineSibling() and combineAnyDescendant() all skip
+		// non-elements the same way.
+		if (empty($parent) || $parent->nodeType !== XML_ELEMENT_NODE) {
 			return false;
 		}
 
@@ -613,6 +616,12 @@ public function combineAnyDescendant($node, $selectors, $index)
 	 * This should only be executed when not working with
 	 * an existing match set.
 	 *
+	 * A match set handed in from outside may legitimately contain non-element nodes -- text,
+	 * comment, CDATA or processing instruction -- because contents() returns them. Selectors only
+	 * ever match elements, so the three shortcuts below skip anything that is not one rather than
+	 * calling the element-only DOM API on it. Everything downstream of here is therefore elements,
+	 * which is why matchesSimpleSelector() and friends can keep their DOMElement type hints.
+	 *
 	 * @param \QueryPath\CSS\SimpleSelector $selector
 	 * @param SplObjectStorage              $matches
 	 *
@@ -682,8 +691,11 @@ protected function initialMatchOnID(SimpleSelector $selector, SplObjectStorage $
 		$xpath     = new DOMXPath($this->dom);
 
 		// Now we try to find any matching IDs.
-		/** @var DOMElement $node */
 		foreach ($matches as $node) {
+			if (! $node instanceof DOMElement) {
+				continue;
+			}
+
 			if ($node->getAttribute('id') === $id) {
 				$found->offsetSet($node);
 			}
@@ -726,8 +738,11 @@ protected function initialMatchOnClasses(SimpleSelector $selector, SplObjectStor
 		$xpath     = new DOMXPath($this->dom);
 
 		// Now we try to find any matching IDs.
-		/** @var DOMElement $node */
 		foreach ($matches as $node) {
+			if (! $node instanceof DOMElement) {
+				continue;
+			}
+
 			// Refactor me!
 			if ($node->hasAttribute('class')) {
 				$intersect = array_intersect($selector->classes, explode(' ', $node->getAttribute('class')));
@@ -793,11 +808,14 @@ protected function initialMatchOnElement(SimpleSelector $selector, SplObjectStor
 			$element = '*';
 		}
 		$found = $this->newMatches();
-		/** @var DOMDocument $node */
 		foreach ($matches as $node) {
-			// Capture the case where the initial element is the root element.
-			if ($node->tagName === $element
-				|| ($element === '*' && $node->parentNode instanceof DOMDocument)) {
+			if (! $node instanceof DOMElement && ! $node instanceof DOMDocument) {
+				continue;
+			}
+
+			// Capture the case where the node itself matches the element.
+			if ($node instanceof DOMElement
+				&& ($element === '*' || $node->tagName === $element)) {
 				$found->offsetSet($node);
 			}
 			$nl = $node->getElementsByTagName($element);

src/CSS/DOMTraverser/PseudoClass.php

@@ -12,6 +12,7 @@
 
 namespace QueryPath\CSS\DOMTraverser;
 
+use DOMElement;
 use QueryPath\CSS\DOMTraverser;
 use QueryPath\CSS\NotImplementedException;
 use QueryPath\CSS\EventHandler;
@@ -154,6 +155,8 @@ public function elementMatches($pseudoclass, $node, $scope, $value = null)
 			case 'checked':
 				return Util::matchesAttribute($node, $name);
 			case 'text':
+				return Util::isTextInput($node);
+
 			case 'radio':
 			case 'checkbox':
 			case 'file':

src/CSS/DOMTraverser/Util.php

@@ -7,6 +7,7 @@
 
 namespace QueryPath\CSS\DOMTraverser;
 
+use DOMElement;
 use DOMNode;
 use QueryPath\CSS\EventHandler;
 use QueryPath\CSS\ParseException;
@@ -504,4 +505,27 @@ public static function parseAnB($rule): array
 
 		return [$aVal, $bVal];
 	}
+
+	/**
+	 * Does this node match jQuery's :text pseudo-class?
+	 *
+	 * jQuery's :text selects input elements whose type attribute is absent, or is "text"
+	 * regardless of case. It says nothing about whether a node is a text node.
+	 *
+	 * Both selector engines ask this question, so they share one answer — they are meant to
+	 * agree, and two copies of the rule would be free to drift apart.
+	 *
+	 * @param mixed $node
+	 *
+	 * @return bool
+	 */
+	public static function isTextInput($node): bool
+	{
+		if (! $node instanceof DOMElement || strtolower($node->localName) !== 'input') {
+			return false;
+		}
+
+		// An input with no type attribute defaults to a text input.
+		return ! $node->hasAttribute('type') || strtolower($node->getAttribute('type')) === 'text';
+	}
 }

src/CSS/QueryPathEventHandler.php

@@ -357,6 +357,30 @@ public function attribute($name, $value = null, $operation = EventHandler::IS_EX
 		$this->findAnyElement = false;
 	}
 
+	/**
+	 * Helper function for the jQuery ':text' pseudo-class.
+	 *
+	 * As in jQuery, ':text' selects `input` elements of type text -- that is, an
+	 * `input` whose `type` attribute is either absent (`text` is the default
+	 * type of an `input`) or is `text`, matched case-insensitively. It does NOT
+	 * indicate whether the node is a text node.
+	 *
+	 * @see https://api.jquery.com/text-selector/
+	 */
+	protected function textInput()
+	{
+		$found   = new SplObjectStorage();
+		$matches = $this->candidateList();
+		foreach ($matches as $item) {
+			if (Util::isTextInput($item)) {
+				$found->offsetSet($item);
+			}
+		}
+
+		$this->matches        = $found;
+		$this->findAnyElement = false;
+	}
+
 	/**
 	 * Helper function to find all elements with exact matches.
 	 *
@@ -553,6 +577,8 @@ public function pseudoClass($name, $value = null)
 				$this->attribute($name);
 				break;
 			case 'text':
+				$this->textInput();
+				break;
 			case 'radio':
 			case 'checkbox':
 			case 'file':

tests/Issues/Issue49Test.php

@@ -0,0 +1,191 @@
+<?php
+
+namespace QueryPathTests;
+
+use DOMCdataSection;
+use DOMComment;
+use DOMProcessingInstruction;
+use DOMText;
+
+class Issue49Test extends TestCase
+{
+	protected const INPUT_HTML = '<div>'
+								 . '<input id="a" type="text" />'
+								 . '<input id="b" />'
+								 . '<input id="c" type="TeXt" />'
+								 . '<input id="d" type="password" />'
+								 . '<input id="e" type="checkbox" />'
+								 . '<input id="f" type="submit" />'
+								 . '<textarea id="g"></textarea>'
+								 . '<button id="h">Go</button>'
+								 . '</div>';
+
+	/**
+	 * Every kind of non-element node the DOM can hand back from contents(), as siblings.
+	 */
+	protected const MIXED_XML = '<?xml version="1.0"?>'
+							   . '<root class="wrap" id="wrap">'
+							   . 'Sample'
+							   . '<!-- A comment -->'
+							   . '<![CDATA[Some data]]>'
+							   . '<?target instruction?>'
+							   . '<span class="wrap" id="child">Child</span>'
+							   . '</root>';
+
+	/**
+	 * Get the ID of every element in the match set.
+	 *
+	 * @param \QueryPath\DOMQuery $query
+	 *
+	 * @return array
+	 */
+	protected function ids($query): array
+	{
+		$ids = [];
+		foreach ($query as $item) {
+			$ids[] = $item->attr('id');
+		}
+		sort($ids);
+
+		return $ids;
+	}
+
+	public function testCheckingForMatchingTextInputs(): void
+	{
+		$q = html5qp('<div><input name="text1" type="text" /><input name="text2" /></div>', 'div');
+
+		/*
+		 * The collection holds the <div>. It is not itself a text input, but it contains two,
+		 * so the containment question is asked with has() and the matches with find().
+		 */
+		$this->assertCount(1, $q->has(':text'));
+		$this->assertCount(2, $q->find(':text'));
+
+		/* The inputs themselves match: an explicit type="text", and an <input> with no type */
+		$this->assertTrue($q->find('input')->is(':text'));
+		$this->assertTrue($q->find('[name="text1"]')->is(':text'));
+		$this->assertTrue($q->find('[name="text2"]')->is(':text'));
+
+		/* contents() here holds the two <input> elements, not text nodes */
+		$firstInput = $q->contents()->eq(0);
+		$this->assertTrue($firstInput->is(':text'));
+	}
+
+	public function testCheckingForEmptyTextInputs(): void
+	{
+		$q = html5qp('<div>Sample</div>', 'div');
+
+		/* Check if the DOMNode or its children matches */
+		$this->assertFalse($q->is(':text'));
+		$this->assertCount(0, $q->find(':text'));
+
+		/* check if a text node matches */
+		$textNode = $q->find('div')->contents()->eq(0);
+		$this->assertFalse($textNode->is(':text'));
+	}
+
+	/**
+	 * As in jQuery, ':text' matches an `input` whose type is absent or 'text'
+	 * (case-insensitively), and nothing else.
+	 *
+	 * @see https://api.jquery.com/text-selector/
+	 */
+	public function testTextSelectorOnlyMatchesTextInputs(): void
+	{
+		$q = html5qp(self::INPUT_HTML, 'div');
+
+		$this->assertSame(['a', 'b', 'c'], $this->ids($q->find(':text')));
+	}
+
+	public function testTextSelectorMatchesTheInputItself(): void
+	{
+		$q = html5qp(self::INPUT_HTML, 'div');
+
+		// Explicit type="text", no type at all, and mixed-case type.
+		foreach (['a', 'b', 'c'] as $id) {
+			$this->assertTrue($q->find('#' . $id)->is(':text'), $id);
+		}
+
+		// password, checkbox, submit, textarea, button.
+		foreach (['d', 'e', 'f', 'g', 'h'] as $id) {
+			$this->assertFalse($q->find('#' . $id)->is(':text'), $id);
+		}
+	}
+
+	/**
+	 * remove() runs the selector through the legacy CSS engine, which must agree
+	 * with find().
+	 */
+	public function testTextSelectorInTheLegacyEngine(): void
+	{
+		$q = html5qp(self::INPUT_HTML, 'div');
+
+		$this->assertSame(['a', 'b', 'c'], $this->ids($q->remove(':text')));
+		$this->assertCount(0, $q->find(':text'));
+	}
+
+	/**
+	 * Any selector run against a match set holding a non-element node must return a sane result
+	 * rather than fataling on the element-only DOM API.
+	 */
+	public function testSelectorsAgainstNonElementNodesDoNotThrow(): void
+	{
+		$contents = qp(self::MIXED_XML, 'root')->contents();
+
+		$kinds = [
+			DOMText::class,
+			DOMComment::class,
+			DOMCdataSection::class,
+			DOMProcessingInstruction::class,
+		];
+
+		foreach ($kinds as $index => $class) {
+			$node = $contents->eq($index);
+			$this->assertInstanceOf($class, $node->get(0));
+			$this->assertMatchesNothing($node, $class);
+		}
+	}
+
+	/**
+	 * The full battery, so every node kind is held to the same standard.
+	 *
+	 * @param \QueryPath\DOMQuery $node
+	 * @param string               $kind
+	 */
+	private function assertMatchesNothing($node, $kind): void
+	{
+		$selectors = ['*', 'span', '.wrap', '#wrap', '[class]', '[class="wrap"]', ':first-child', ':text', 'root span'];
+		foreach ($selectors as $selector) {
+			$this->assertFalse($node->is($selector), sprintf('%s must not match is(%s)', $kind, $selector));
+		}
+
+		foreach (['*', 'span', '.wrap', '#wrap', '[class]'] as $selector) {
+			$this->assertCount(0, $node->find($selector), sprintf('%s must not match find(%s)', $kind, $selector));
+		}
+
+		$this->assertCount(0, $node->filter('*'), sprintf('%s must not match filter(*)', $kind));
+	}
+
+	/**
+	 * A match set mixing elements with non-element nodes must still match the
+	 * elements it holds.
+	 */
+	public function testMixedNodeMatchSetStillMatchesItsElements(): void
+	{
+		$contents = html5qp('<div>Sample<span class="x" id="s"><em id="e">Child</em></span></div>', 'div')
+			->contents();
+
+		/* The set holds a text node and an element; neither may cause a fatal. */
+		$this->assertCount(2, $contents);
+
+		/* find() reaches the descendants of the elements in the set */
+		$this->assertSame(['e'], $this->ids($contents->find('em')));
+		$this->assertSame(['e'], $this->ids($contents->find('#e')));
+
+		/* filter() and is() ask about the elements in the set itself */
+		$this->assertSame(['s'], $this->ids($contents->filter('span')));
+		$this->assertSame(['s'], $this->ids($contents->filter('.x')));
+		$this->assertSame(['s'], $this->ids($contents->filter('#s')));
+		$this->assertTrue($contents->is('.x'));
+	}
+}