Skip to content

Make filter() narrow the match set instead of keeping elements that contain a match - #71

Closed
jakejackson1 wants to merge 2 commits into
issue-62from
fix-filter-selector
Closed

Make filter() narrow the match set instead of keeping elements that contain a match#71
jakejackson1 wants to merge 2 commits into
issue-62from
fix-filter-selector

Conversation

@jakejackson1

Copy link
Copy Markdown
Member

Fixes the containment bug in filter() — the one all three of the open selector PRs ran into
and left alone because fixing it means deliberately rebaselining an existing test.

Based on issue-62 (#69), which is based on issue-51 (#67). Merge order: #67#69 → this.
It needs QueryPath\Helpers\NodeMatcher, introduced in #67.

The bug

filter() ran a descendant search per node, so it kept any element that merely contained a
match rather than narrowing the set to the elements that match:

$html = '<div><ul><li class="x">a1</li><li>a2</li></ul><p class="x">p1</p><span>s1</span></div>';

html5qp($html)->find('ul, p, span')->filter('.x');
// before: ul, p     <- ul only contains .x
// after:  p         <- jQuery

That is what has() does, and has() keeps doing it — it is the migration path for anyone
relying on the old behaviour.

The author's comment

filter() carried this 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, which asserts the containment result:

$this->assertEquals(1, qp($file)->filter('li')->count());          // <root> contains li
$this->assertEquals(2, qp($file, 'inner')->filter('li')->count()); // <inner> contains li

Under jQuery semantics both are 0. The test is rebaselined, and two tests are added: one
pinning the filter()/has() difference so the migration path stays covered, one covering
set-level evaluation.

Filtering the set, not the node

Both filter() and children($selector) now filter their candidates in one pass instead of
one node at a time. A per-node pass cannot evaluate a selector describing a position within the
set — each node would be the only member of its own one-element set.

This also drops the per-node scope node, fixing the same :scope bug that #69 fixed in the
traversal methods:

html5qp($html)->find('div')->children(':scope')->count();
// before: 3 (every child)   after: 0

children() did not have the containment bug — it already used the correct traverser mode.
Only its scope handling was wrong.

Interaction with #66

#66 documents a known limitation: because children() and filter() built a traverser per
node, a positional pseudo-class saw a one-element set, so children('li:first') returned every
li child. Filtering set-at-a-time resolves it. Verified by merging #66 into this branch
locally — 382 tests pass, and:

$q->find('li')->filter(':first')  // a1        (was: every li)
$q->find('li')->filter(':eq(3)')  // b1
$q->find('li')->filter(':odd')    // a2,b1
$q->find('ul')->children('li:first') // x1

No change is needed in #66; this branch just removes the reason for its caveat.

Verification

🤖 Generated with Claude Code

jakejackson1 and others added 2 commits August 22, 2026 04:09
filter() kept any element that *contained* a match, because it ran a descendant
search per node. jQuery's filter() narrows the set to the members that match, so
qp($file, 'inner')->filter('li') returned both <inner> elements where jQuery
returns none. has() already provides the old behaviour.

The original author left a comment on the line saying the correct traverser mode
"fails unit tests". It fails exactly one, DOMQueryTest::testFilter, which asserts
the containment result; that test is rebaselined here.

Both methods now filter their candidates as a single set rather than one node at
a time. A per-node pass cannot evaluate a selector that describes a position
within the set, because each node is the only member of its own one-element set.
This also drops the per-node scope node, so children(':scope') no longer matches
every child.

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

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.71%. Comparing base (6ccf341) to head (aa85469).

Additional details and impacted files
@@              Coverage Diff               @@
##             issue-62      #71      +/-   ##
==============================================
- Coverage       89.74%   89.71%   -0.03%     
  Complexity       1363     1363              
==============================================
  Files              27       27              
  Lines            3071     3064       -7     
==============================================
- Hits             2756     2749       -7     
  Misses            315      315              

☔ 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

Copy link
Copy Markdown
Member Author

Collapsed into #72, which carries all of this work unchanged — same tree, one branch. Closing to keep the review in one place; reopen if the split turns out to be preferable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant