diff --git a/CHANGELOG.md b/CHANGELOG.md index ac1c5e7..c9c4dc6 100644 --- a/CHANGELOG.md +++ b/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 `` came back out of `html()`, `innerHTML()`, `innerXML()`, `innerXHTML()`, `xml()`, `html5()`, `innerHTML5()`, and `writeXML()` as ``. 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 `` 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 diff --git a/src/CSS/DOMTraverser.php b/src/CSS/DOMTraverser.php index 7c6f09c..c42662d 100644 --- a/src/CSS/DOMTraverser.php +++ b/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); diff --git a/src/CSS/DOMTraverser/PseudoClass.php b/src/CSS/DOMTraverser/PseudoClass.php index a36d346..e8f8c25 100644 --- a/src/CSS/DOMTraverser/PseudoClass.php +++ b/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': diff --git a/src/CSS/DOMTraverser/Util.php b/src/CSS/DOMTraverser/Util.php index 6a3ddac..cd2f542 100644 --- a/src/CSS/DOMTraverser/Util.php +++ b/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'; + } } diff --git a/src/CSS/QueryPathEventHandler.php b/src/CSS/QueryPathEventHandler.php index 13e6b43..bbb73c9 100644 --- a/src/CSS/QueryPathEventHandler.php +++ b/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': diff --git a/tests/Issues/Issue49Test.php b/tests/Issues/Issue49Test.php new file mode 100644 index 0000000..ad9f8bc --- /dev/null +++ b/tests/Issues/Issue49Test.php @@ -0,0 +1,191 @@ +' + . '' + . '' + . '' + . '' + . '' + . '' + . '' + . '' + . ''; + + /** + * Every kind of non-element node the DOM can hand back from contents(), as siblings. + */ + protected const MIXED_XML = '' + . '' + . 'Sample' + . '' + . '' + . '' + . 'Child' + . ''; + + /** + * 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'); + + /* + * The collection holds the
. 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 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 elements, not text nodes */ + $firstInput = $q->contents()->eq(0); + $this->assertTrue($firstInput->is(':text')); + } + + public function testCheckingForEmptyTextInputs(): void + { + $q = html5qp('
Sample
', '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('
SampleChild
', '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')); + } +}