Skip to content

Match selectors against the nodes in hand, not their descendants - #72

Merged
jakejackson1 merged 7 commits into
mainfrom
fix-selector-matching
Oct 26, 2026
Merged

jakejackson1 merged 7 commits into
mainfrom
fix-selector-matching

Conversation

@jakejackson1

@jakejackson1 jakejackson1 commented Aug 21, 2026 •

Copy link
Copy Markdown
Member

Collapses #67, #69 and #71 into one branch. They were three PRs because there was one agent per issue, but they are one defect at three layers. Those PRs are closed in favour of this one.

Fixes #51. Fixes #62. Also fixes the filter() bug that had no issue.

The defect

is(), filter() and the selector-filtered traversal methods asked "does this node contain a match?" where jQuery asks "is this node a match?". Each ran a descendant search against the candidate, so anything matching below it kept it:

html5qp('<p><span>foo</span></p>', 'p')->is('span');   // true  — expected false
qp($xml, 'Demographics > Age > Name')->parents('Demographics');
// <Demographics>, <AmplifyReturn>, <ns1:AmplifyResponse>  — expected just <Demographics>
qp($file, 'inner')->filter('li')->count();             // 2     — expected 0

has() does exactly what these used to do and is unchanged. Anyone who depends on the old behaviour can switch to it.

The fix

The test is extracted into QueryPath\Helpers\NodeMatcher, so every call path shares it. A matcher is built once per call with new NodeMatcher($selector):

  • $matcher->filter($nodes) / matching($nodes) / matchesAny($nodes) evaluate a whole set in one pass. filter(), children(), not(), is(), siblings(), nextAll(), prevAll() and parents() use these. A per-node pass cannot evaluate a selector describing a position within the set (:first, :eq(n), :odd), because each node is the only member of its own one-element set.
  • $matcher->matches($node) answers the per-node question for the walk-and-stop methods (closest(), parent(), next(), prev(), nextUntil(), prevUntil(), parentsUntil()), memoizing each answer for the duration of the call.

It builds DOMTraverser in "initialized" mode, which takes the supplied nodes as the candidates instead of seeding them with a descendant search. The only engine change is that DOMTraverser::resolveSubject() drops non-element nodes from a caller-supplied set, so is() no longer fatals on a set holding text nodes (Call to undefined method DOMText::getElementsByTagName()).

It also leaves the scope node at its default. Passing a candidate as the scope node made every candidate match :scope, so the selector silently stopped filtering:

qp($xml, 'c')->parents(':scope')     // before: b, a, root   after: root
find('div')->children(':scope')      // before: 3            after: 0

parents()/parentsUntil() now return reverse document order with duplicates removed, per the jQuery spec quoted in #62.

The old per-node path also rebuilt a DOMQuery for every visited node. On a 1,000-item list, nextUntil() drops from about 1,000 ms to 26 ms.

Breaking changes

Method Before After
is($sel) true if a descendant matched true if a set member matches
filter($sel) kept elements containing a match keeps elements that match
parents(), parentsUntil(), closest(), parent(), next*(), prev*(), siblings(), not() selector matched containment selector matches the element
parents(), parentsUntil() ordering grouped by starting element reverse document order, deduplicated

The author's comment on filter()

filter() had carried this comment since 2009:

// Seems like this should be right... but it fails unit
// tests. Need to compare to jQuery.
// $query = new \QueryPath\CSS\DOMTraverser($tmp, TRUE, $m);

It fails exactly one test, DOMQueryTest::testFilter, whose two meaningful assertions both pin the containment result. That test is rebaselined here, and two tests are added: one pins the filter()/has() difference so the has() replacement stays covered, and one covers set-level evaluation. No other existing test encoded the old behaviour.

Interaction with merged work

Rebased onto current main, which now includes #70 (positional pseudo-classes, #66), #50 (:text), #106 (not() with SplObjectStorage) and #122 (nested pseudo-class arguments). It merges cleanly. The not() fix from #106 is kept, and its SplObjectStorage branch now shares the selector branch's loop. #70's caveat that children('li:first') returned every li child no longer applies, since filtering is now set-level. Issue49Test passes unchanged.

Follow-ups (out of scope)

Verification

  • vendor/bin/phpunit: 548 tests, 1653 assertions, 0 failures (1 skipped)
  • composer run lint and composer run lint:min-php: clean, PHP 7.1+
  • New tests: tests/Issues/Issue51Test.php, tests/Issues/Issue62Test.php, and additions to DOMQueryTest, including a ParseException case for every changed method and for filter(''), is('') and not('')

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.92473% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.07%. Comparing base (6ae6acf) to head (f8196cf).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/Helpers/NodeMatcher.php 95.65% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #72      +/-   ##
============================================
+ Coverage     90.69%   91.07%   +0.38%     
- Complexity     1424     1433       +9     
============================================
  Files            25       26       +1     
  Lines          3170     3172       +2     
============================================
+ Hits           2875     2889      +14     
+ Misses          295      283      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jakejackson1 added a commit that referenced this pull request Aug 21, 2026
The assertion held the <div> and expected is(':text') to be true, which only
worked because is() ran a descendant search. #72 makes is() test the elements in
the match set, as jQuery does, so that assertion would flip to false.

Rewritten so it does not depend on which semantics are in force: the containment
question is asked with has(), which is what it always meant, and is() is asked of
the inputs themselves. It passes both with and without #72.

Also renamed $textNode to $firstInput in this test. contents()->eq(0) here is the
first <input> element, not a text node — the name is accurate in the sibling test
below, where the fixture really does hold text.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1 added a commit that referenced this pull request Aug 21, 2026
Two assertions reached find() for a node that was already in their own match set,
which only worked because find() self-matched. #73 makes find() search
descendants only, as jQuery does.

Rewritten to ask each question of the method that answers it: find() of a real
descendant, filter()/is() of the elements in the set. The mixed-node fixture gains
a nested <em> so find() still has something to reach, which keeps the point of the
test — that a set holding a text node does not cause a fatal — intact on both
sides of the selector.

Passes with and without #72/#73.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1 added a commit that referenced this pull request Aug 24, 2026
The assertion held the <div> and expected is(':text') to be true, which only
worked because is() ran a descendant search. #72 makes is() test the elements in
the match set, as jQuery does, so that assertion would flip to false.

Rewritten so it does not depend on which semantics are in force: the containment
question is asked with has(), which is what it always meant, and is() is asked of
the inputs themselves. It passes both with and without #72.

Also renamed $textNode to $firstInput in this test. contents()->eq(0) here is the
first <input> element, not a text node — the name is accurate in the sibling test
below, where the fixture really does hold text.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1 added a commit that referenced this pull request Aug 24, 2026
Two assertions reached find() for a node that was already in their own match set,
which only worked because find() self-matched. #73 makes find() search
descendants only, as jQuery does.

Rewritten to ask each question of the method that answers it: find() of a real
descendant, filter()/is() of the elements in the set. The mixed-node fixture gains
a nested <em> so find() still has something to reach, which keeps the point of the
test — that a set holding a text node does not cause a fatal — intact on both
sides of the selector.

Passes with and without #72/#73.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jakejackson1

Copy link
Copy Markdown
Member Author

Note from #50: this PR should delete the wildcard self-match, and NodeMatcher's element filter is load-bearing

Three things found while rebasing and reviewing #50, verified against a trial merge of the two branches.

1. Delete #50's wildcard self-match once this lands.

#50 makes DOMTraverser::initialMatchOnElement() attach the node itself when the element selector is the
wildcard, so that $input->is(':text') can be true for the input itself. That is a workaround for is()/filter()
being built on find() — exactly what this PR fixes properly with NodeMatcher. Its changelog entry says as much.

I merged the two branches locally and deleted the hunk. All 421 tests / 1358 assertions pass with it gone,
including #50's own is(':text') assertions — NodeMatcher::matchesNode() covers everything it was standing in for.

It is worth deleting rather than leaving, because it is not confined to a literal find('*'): a null element
normalises to *, so every attribute-only and pseudo-only selector routes through it. Measured on the merged
branches, best of 9 on a 16,882-node document:

query hunk present hunk removed result set
find('td')->find('*') 32.21 ms 22.70 ms (−30%) 14400 → 7200
find('td')->find('[href]') 30.23 ms 22.71 ms (−25%) 2400 → 2400 (identical)
find('td')->find(':text') 41.19 ms 27.96 ms (−32%) 2400 → 2400 (identical)
find('tr')->find('em') 9.16 ms 9.20 ms (unchanged) 2400 → 2400 (identical)

Retained memory for find('td')->find('*') drops from 5.11 MB to 4.64 MB. The last row confirms named-element
selectors were never affected. Removing it also restores jQuery's descendant-only find('*'), which is what #73 wants.

2. NodeMatcher's instanceof DOMElement filter must not be removed as redundant.

NodeMatcher::filter() screens non-elements before building the traverser:

foreach ($nodes as $node) {
    if ($node instanceof DOMElement) {
        $candidates->offsetSet($node);
    }
}

That looks like it duplicates the guards #50 adds to initialMatchOnID() / initialMatchOnClasses() /
initialMatchOnElement(). It does not. Those guards live in initialMatch(), and
DOMTraverser::resolveSubject() skips initialMatch() entirely when $initialized is true — which is the
mode NodeMatcher uses:

if ($index === 0 && $this->initialized) {
    $candidates = $this->matches;      // no initialMatch(), so no guard
} else {
    $candidates = $this->initialMatch($selectorGroup[$index], $this->matches);
}

I confirmed this by switching QueryFilters::filter() to initialized mode without an element filter:

TypeError: QueryPath\CSS\DOMTraverser::matchesSimpleSelector(): Argument #1 ($node)
           must be of type DOMElement, DOMText given

That is issue #49's crash coming back. The two filters guard two different paths and both are needed.

3. This branch needs a rebase onto main before it merges.

It predates #70, so its Util.php re-adds sortDocumentOrder(), documentOrderPath(), siblingOffset() and
comparePaths(), which main now already has. A straight merge fails with
Cannot redeclare QueryPath\CSS\DOMTraverser\Util::sortDocumentOrder(). #70's copies are the supersets, so the
resolution is to drop this branch's versions and keep main's. The other conflicts are just the use block and
the CHANGELOG unreleased section.

jakejackson1 and others added 4 commits October 26, 2026 12:11
is(), filter(), and the selector-filtered traversal methods all asked "does this
node contain a match?" where jQuery asks "is this node a match?". They ran a
descendant search against each candidate, so a selector that matched anything
below a candidate kept that candidate.

    html5qp('<p><span>foo</span></p>', 'p')->is('span');        // true, expected false
    qp($xml, 'Demographics > Age > Name')->parents('Demographics');
                                        // <Demographics>, <AmplifyReturn>, <ns1:AmplifyResponse>
    qp($file, 'inner')->filter('li')->count();                  // 2, expected 0

has() does what these used to do, and is unchanged: it remains the migration path
for callers that want the containment behaviour.

The single-node test is extracted into QueryPath\Helpers\NodeMatcher so the three
call paths cannot drift apart. It builds the traverser in "initialized" mode,
which treats the supplied nodes as the candidates rather than seeding a
descendant search, and leaves the scope node at its default so :scope resolves
against the document element exactly as it does in find(). Passing a candidate as
the scope node made every candidate match :scope, which is why parents(':scope')
returned every ancestor and children(':scope') returned every child.

filter() and children() filter their candidates as one set rather than one node
at a time. A per-node pass cannot evaluate a selector describing a position
within the set, because each node is the only member of its own one-element set.

parents() and parentsUntil() now return reverse document order with duplicates
removed, as jQuery does. Previously a set built from more than one starting
element was grouped by starting element.

DOMQueryTest::testFilter asserted the containment result and is rebaselined. It
is the test the original author's "fails unit tests" comment on filter() referred
to; it was the only one.

Fixes #51
Fixes #62

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems, both in the machinery this PR introduced.

The selector was still evaluated per node in 12 places. The changelog states the
rule — a per-node pass cannot answer a selector describing a position within the
set, because each node is the only member of its own one-element set — and then
filter() and children() honour it while every other method routes through
matchesNodeSelector(). It does not show up yet, because :first and friends are
still sibling-positional on main. Once #70 lands, not(':first') returns the empty
set, siblings(':first') returns every sibling, and nextAll(':first') returns all
of them. Verified against #70 merged locally.

not(), siblings(), nextAll(), prevAll() and the parents() path now collect their
candidates and filter the set once. That also drops a selector parse and four
SplObjectStorage allocations per candidate: parents('div') on a 20-deep tree of
100 leaves parsed 'div' 2000 times for one call.

matchesNodeSelector() stays for the loops that genuinely need a per-node answer —
nextUntil(), prevUntil(), parentsUntil(), closest(), next(), prev() and parent(),
which stop at the first match rather than collecting a set.

The document-order sort was quadratic on document width. documentOrderPath()
walked previousSibling per node, so sorting n siblings cost n^2/2 pointer hops,
and array_unshift() per level made it quadratic on depth too. Sorting is now
Util::sortDocumentOrder(), which indexes each child list once and memoizes for
the duration of the call. parents() over a 4000-wide document: 253ms -> 33ms,
linear again.

The sorter is named and shaped to match the one #70 adds to the same file, so
merging the two is a single "defined twice, keep one" rather than a reconcile.

NodeMatcher::filter() now returns its result in the caller's order, so the
order-restoring walk filter() and children() each carried is gone, and
matchesAny() folds into matchesNode().

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Skip non-element candidates in DOMTraverser::resolveSubject(), so an
  initialized traverser no longer needs its caller to pre-filter the set.
- NodeMatcher gains matchesAny() for is(), and an instance form that
  remembers each node's answer for the duration of one call. closest(),
  parent(), next(), prev() and the *Until() methods revisit the same nodes
  from every starting element; nextUntil('.stop') over 300 siblings drops
  from ~1000ms to ~25ms with the same per-node semantics.
- Drop the matchesNodeSelector() pass-through and the orphaned docblock,
  route children() through filterCandidates(), and collapse the duplicated
  ancestor walk in getParentElements().
- Tests: a real positional filter() case, a shared Issue62 fixture, and
  drop the parents() test duplicated between Issue51Test and Issue62Test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rents()

- DOMTraverser filters a caller-supplied set to elements where it is adopted,
  instead of guarding both candidate loops.
- not() shares one exclusion loop for SplObjectStorage and selector arguments,
  and uses NodeMatcher::matching(), which skips filter()'s re-ordering pass.
- sortReverseDocumentOrder() returns early for a single starting element,
  whose ancestor walk is already in reverse document order.
- Remove an unused import and shorten the NodeMatcher docblock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@gravitypdf-bot
gravitypdf-bot marked this pull request as ready for review October 26, 2026 01:16
gravitypdf-bot pushed a commit that referenced this pull request Oct 26, 2026
find() treated the nodes already in the match set as candidates for their own
selector, so qp($xml, '#a1')->find('a') returned #a1 itself alongside any nested
<a>. jQuery's .find() searches descendants; filter() is what asks whether the
nodes in hand match.

The document element keeps its self-match. qp() seeds the match set with the
document element rather than with the document node, so it stands in for
$(document), and without the exception qp($xml)->find('root') could never match.

This depends on #72. Before it, the selector-filtered traversal methods reach
is(), which reached find(), so they relied on the self-match to test a node
against a selector; changing find() on its own breaks nine of them. Once those
go through NodeMatcher they no longer care.

testBefore searched for the node it was standing on. Rewritten to search from
top(), which is the shape the next two assertions in that test already use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gravitypdf-bot and others added 2 commits October 26, 2026 12:28
The set methods were static and took the selector on every call, while the
per-node matches() needed an instance for its memo. Every method is now an
instance method on a NodeMatcher built with the selector, so there is one way
to use it and one place for the parsed selector to live later (#116).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ocblocks

- nextAll() and prevAll() break at the first sibling already collected, since
  every sibling beyond it was collected with it. Linear instead of quadratic
  in the number of starting siblings.
- The walk-and-stop methods now document ParseException, not the Exception
  that QueryPath::with() used to throw.
- NodeMatcher::matches() takes a DOMNode, as SplObjectStorage requires.
- Drop the stale "Is there any case where parent node is not an element?"
  comments that the code beneath them answers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gravitypdf-bot pushed a commit that referenced this pull request Oct 26, 2026
find() treated the nodes already in the match set as candidates for their own
selector, so qp($xml, '#a1')->find('a') returned #a1 itself alongside any nested
<a>. jQuery's .find() searches descendants; filter() is what asks whether the
nodes in hand match.

The document element keeps its self-match. qp() seeds the match set with the
document element rather than with the document node, so it stands in for
$(document), and without the exception qp($xml)->find('root') could never match.

This depends on #72. Before it, the selector-filtered traversal methods reach
is(), which reached find(), so they relied on the self-match to test a node
against a selector; changing find() on its own breaks nine of them. Once those
go through NodeMatcher they no longer care.

testBefore searched for the node it was standing on. Rewritten to search from
top(), which is the shape the next two assertions in that test already use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each method rewritten onto NodeMatcher now documents ParseException, but
nothing asserted it. One case per method, on a node that gives each of them a
candidate to test, plus filter()/is()/not() with an empty selector, which
they have no "no selector" form for.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gravitypdf-bot pushed a commit that referenced this pull request Oct 26, 2026
find() treated the nodes already in the match set as candidates for their own
selector, so qp($xml, '#a1')->find('a') returned #a1 itself alongside any nested
<a>. jQuery's .find() searches descendants; filter() is what asks whether the
nodes in hand match.

The document element keeps its self-match. qp() seeds the match set with the
document element rather than with the document node, so it stands in for
$(document), and without the exception qp($xml)->find('root') could never match.

This depends on #72. Before it, the selector-filtered traversal methods reach
is(), which reached find(), so they relied on the self-match to test a node
against a selector; changing find() on its own breaks nine of them. Once those
go through NodeMatcher they no longer care.

testBefore searched for the node it was standing on. Rewritten to search from
top(), which is the shape the next two assertions in that test already use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jakejackson1
jakejackson1 merged commit abdfc24 into main Oct 26, 2026
14 checks passed
@jakejackson1
jakejackson1 deleted the fix-selector-matching branch October 26, 2026 01:49
jakejackson1 added a commit that referenced this pull request Oct 26, 2026
find() treated the nodes already in the match set as candidates for their own
selector, so qp($xml, '#a1')->find('a') returned #a1 itself alongside any nested
<a>. jQuery's .find() searches descendants; filter() is what asks whether the
nodes in hand match.

The document element keeps its self-match. qp() seeds the match set with the
document element rather than with the document node, so it stands in for
$(document), and without the exception qp($xml)->find('root') could never match.

This depends on #72. Before it, the selector-filtered traversal methods reach
is(), which reached find(), so they relied on the self-match to test a node
against a selector; changing find() on its own breaks nine of them. Once those
go through NodeMatcher they no longer care.

testBefore searched for the node it was standing on. Rewritten to search from
top(), which is the shape the next two assertions in that test already use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1 added a commit that referenced this pull request Oct 26, 2026
* Make find() search descendants only, as jQuery does

find() treated the nodes already in the match set as candidates for their own
selector, so qp($xml, '#a1')->find('a') returned #a1 itself alongside any nested
<a>. jQuery's .find() searches descendants; filter() is what asks whether the
nodes in hand match.

The document element keeps its self-match. qp() seeds the match set with the
document element rather than with the document node, so it stands in for
$(document), and without the exception qp($xml)->find('root') could never match.

This depends on #72. Before it, the selector-filtered traversal methods reach
is(), which reached find(), so they relied on the self-match to test a node
against a selector; changing find() on its own breaks nine of them. Once those
go through NodeMatcher they no longer care.

testBefore searched for the node it was standing on. Rewritten to search from
top(), which is the shape the next two assertions in that test already use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Make the ID and class initial matchers descendant-only too

Only the element matcher was changed, so find('a') stopped self-matching while
find('#a1') and find('.c') still did. Three initial matchers, three separate
self-tests, and the selector you happened to write decided the semantics.

All three now apply the same rule, with the same document-element exception.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Give the document-element exception one home

The rule and its four-line explanation were pasted into all three initial
matchers. They have to agree — find('#a1') matching a node that find('a') does
not is the divergence this PR exists to remove — so they should not be three
independent copies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Pin detach($selector) to find()'s descendant-only matching

remove() still runs on the legacy engine, which lets a selector match the
nodes in hand, so the remove()/detach() agreement test now covers only the
selectors that cannot match them. Tracked in #126.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Leave the document-element exception to the element matcher alone

The ID and class matchers' self-match branches only ever fired for the
document element, which initialXpathQuery() already matches by running their
query from the document. Removing them leaves one caller for
isDocumentElement(), so its check is inlined there, using the same
isSameNode() test as initialXpathQuery().

testBefore() now passes its selector straight to top().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Test find() from mixed, leaf and empty sets, and with malformed selectors

The element, ID and class matchers each skip text and comment nodes in the
match set, exclude the elements in it, and still find their descendants.
Finding from a leaf or an empty set returns nothing, and starting below the
document element still reports a malformed selector.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: gravitypdf-bot <support@gravitypdf.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parents() doesn't match jQuery functionality when using selector html5qp('<p><span>foo</span></p>')->is('span') returns true

2 participants