Skip to content

Psuedo-class selector :text producing errors and incorrect results - #50

Merged
jakejackson1 merged 6 commits into
mainfrom
issue-49
Aug 24, 2026
Merged

Psuedo-class selector :text producing errors and incorrect results#50
jakejackson1 merged 6 commits into
mainfrom
issue-49

Conversation

@jakejackson1

@jakejackson1 jakejackson1 commented Mar 21, 2024

Copy link
Copy Markdown
Member

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

  1. If doing ->is(':text') on a text node an error is produced.
  2. If doing ->find(':text') it won't match <input /> tags without a type (which are considered text inputs). See https://api.jquery.com/text-selector/

Issue Number: #49

Fixes #49

What is the new behavior?

1. Selectors no longer fatal on non-element nodes

The traverser assumed every node in a match set was a DOMElement and called element-only APIs
(getElementsByTagName(), tagName, getAttribute(), hasAttribute()) on it. A match set can legitimately hold
text, comment, CDATA and processing instruction nodes — contents() produces exactly that — so
$singleTextNode->is(':text') blew up with Call to undefined method DOMText::getElementsByTagName().

The fix is made once, at the boundary those nodes actually enter through. Every non-element reaches the traverser
as part of the initial match set, so initialMatchOnElement(), initialMatchOnID() and
initialMatchOnClasses() skip anything that is not an element. Everything downstream of initialMatch() is
therefore elements, which is why matchesSelector(), matchesSimpleSelector(), combine() and the
match*()/Util::matchesAttribute*() helpers all keep their existing DOMElement type hints — those hints are
the contract, and a caller passing a text node is better served by a TypeError at the boundary than a silent
false from four frames in. The reasoning is documented once, on initialMatch().

initialMatchOnElementNS() needs no guard of its own; it delegates to initialMatchOnElement().

The legacy engine (CSS\QueryPathEventHandler, still used by remove() and replaceAll()) already filtered
non-element nodes out in its constructor, so it needed no crash fix. I verified this rather than assuming it:
remove() was exercised against mixed and non-element-only match sets across the full selector battery on both
main and this branch, with no fatal on either.

2. find('a > b') no longer throws when the left side is the root element

combineDirectDescendant() was the one combinator with no element check — combineAdjacent(),
combineSibling() and combineAnyDescendant() all skip non-elements already. When the left-hand side of a
direct-descendant selector matched the root element, it handed the document to matchesSimpleSelector():

qp('<?xml version="1.0"?><root><a/><b><c/></b></root>', 'root')->find('* > *');
// main: TypeError: matchesSimpleSelector(): Argument #1 ($node) must be of type DOMElement,
//       QueryPath\Document given

This is a pre-existing bug independent of #49 — it needs no text nodes to reproduce — but it is the same class of
fault, so it is fixed here rather than left behind.

3. :text now means what it means in jQuery

:text was implemented as [type="text"], which matched any element carrying that attribute and missed a bare
<input />. It now matches jQuery: an input whose type attribute is absent (text is an input's default
type) or is text, compared case-insensitively. It does not, and never did, indicate whether a node is a text
node.

Both engines share one implementation, CSS\DOMTraverser\Util::isTextInput(), called directly from
PseudoClass::elementMatches() and QueryPathEventHandler::textInput(), so find(':text') and
remove(':text') / replaceAll(':text') cannot drift apart.

Tests

tests/Issues/Issue49Test.php — 7 tests, 91 assertions:

  • testCheckingForMatchingTextInputs, testCheckingForEmptyTextInputs — the committed spec from the issue.
  • testTextSelectorOnlyMatchesTextInputs:text matches <input type="text">, bare <input> and
    <input type="TeXt">, and not password / checkbox / submit / <textarea> / <button>.
  • testTextSelectorMatchesTheInputItself — the same eight inputs asserted through is() on the input itself.
  • testTextSelectorInTheLegacyEngineremove(':text') selects the same set as find(':text').
  • testSelectorsAgainstNonElementNodesDoNotThrow — one fixture holding a text, comment, CDATA and processing
    instruction node as siblings, each run through the same battery of nine is() selectors (element, wildcard,
    class, ID, attribute, attribute-value, :first-child, :text, descendant), five find() selectors, and
    filter('*').
  • testMixedNodeMatchSetStillMatchesItsElements — a match set mixing a text node with an element still matches the
    element it holds.

All 7 fail against main (3 errors, 4 failures). Full suite: 394 tests, 1267 assertions, 0 failures
(2 pre-existing skips for create_function on PHP 8). composer run lint and composer run lint:min-php clean.
CI covers PHP 7.1–8.5.

Does this introduce a breaking change?

  • Yes
  • No

:text changes meaning, deliberately: it no longer matches non-input elements that happen to carry
type="text", and it now matches <input> with no type. This is the point of the issue and brings the selector
in line with jQuery.

Additionally, a wildcard element selector now also considers the context node itself, not only its descendants, so
$el->find('*') includes $el. This is what makes $input->is(':text') true for the input itself, since is()
and filter() are both implemented on top of find() and a bare pseudo-class selector expands to *.

This hunk has a measurable cost and is the part most worth reviewing. Because a null element normalises to
*, it is not confined to a literal find('*') — every attribute-only or pseudo-only selector routes through it.
Measured with the hunk reverted vs. in place, best of 9 runs on a 16,882-node HTML5 document:

query without with delta result set
find('td')->find('*') 20.83 ms 30.09 ms +44% 7200 → 14400
find('td')->find('[href]') 21.80 ms 26.60 ms +22% 2400 → 2400 (identical)
find('td')->find(':text') 27.97 ms 36.78 ms +31% 2400 → 2400 (identical)
find('tr')->find('em') 9.27 ms 8.26 ms none 2400 → 2400 (identical)

The [href] and :text rows are the point: 7,200 extra candidates are pushed through the full match pipeline and
every one of them fails, for a result set that is byte-for-byte the same. The last row confirms the cost is
confined to wildcard-subject selectors — a named element is unaffected. Retained memory for
find('td')->find('*') goes from 4.64 MB to 5.11 MB (+10%).

The cost falls entirely on find(), which does not need the behaviour; only is()/filter() do, and there the
candidate set is a single node and the broadening is free. Reverting the hunk fails exactly two assertions, both
is(':text').

Other information

Overlap with #72 and #73. The wildcard self-match above is a workaround for is()/filter() being built on
find(), and both of those PRs address that properly:

Whichever of those lands after this PR should drop the wildcard self-match from initialMatchOnElement();
testTextSelectorMatchesTheInputItself and testMixedNodeMatchSetStillMatchesItsElements depend on it and move
with it. Nothing else here overlaps: the crash fixes live in the traverser's initial match and combinators, and
:text is a pseudo-class evaluation.

The earlier revision of this description flagged a conflict with #51; that PR no longer exists, and the
assertion it warned about ($q->is(':text') relying on descendant semantics) has since been replaced on this
branch by $q->has(':text'), which is correct under either semantics.

The other jQuery input pseudo-classes (:radio, :checkbox, :password, :submit, :button, …) still use the
old [type=x] implementation and have the same divergence from jQuery in a milder form — they are case-sensitive
and do not check the tag name, so :radio misses <input type="RADIO"> and matches <span type="radio">. Left
alone deliberately: out of scope for #49, and worth its own issue.

🤖 Generated with Claude Code

@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 90.18%. Comparing base (807384f) to head (745a4e2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main      #50      +/-   ##
============================================
+ Coverage     90.11%   90.18%   +0.06%     
- Complexity     1421     1433      +12     
============================================
  Files            26       26              
  Lines          3198     3219      +21     
============================================
+ Hits           2882     2903      +21     
  Misses          316      316              

☔ 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 and others added 5 commits August 24, 2026 20:02
Two defects, per issue #49.

1. Running any selector against a match set that contained a non-element
   node (text, comment, CDATA, processing instruction) fataled, because
   the traverser assumed every node was a DOMElement and called
   element-only methods such as getElementsByTagName() and tagName on it.
   Non-element nodes now simply do not match an element selector:

   - DOMTraverser::matchesSimpleSelector() returns FALSE for any node
     that is not a DOMElement. matchesSelector(), matchesSimpleSelector()
     and combine() take a DOMNode rather than a DOMElement so they can
     make that decision instead of raising a TypeError.
   - initialMatchOnElement(), initialMatchOnID() and
     initialMatchOnClasses() skip nodes that cannot hold elements.
   - PseudoClass::elementMatches() and Util::matchesAttribute[NS]() guard
     against non-elements as well, since they are reachable directly.

   initialMatchOnElement() also now captures the node itself when the
   element selector is the wildcard, which is what initialMatchOnID() and
   initialMatchOnClasses() already do for their own selectors.

2. The :text pseudo-class matched anything with type="text". It now
   follows jQuery, matching an input whose type attribute is absent
   (text is an input's default type) or is text, compared
   case-insensitively. It has never indicated whether a node is a text
   node. Fixed in both the current engine (CSS\DOMTraverser) and the
   legacy engine (CSS\QueryPathEventHandler) that remove() and
   replaceAll() still use, so the two agree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
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>
The rule — an input whose type is absent or case-insensitively "text" — was
spelled out once per engine. The two are meant to agree, which is why this PR
has a test asserting they do; sharing the definition is what actually keeps them
agreeing. Util is the established home for this: 4.1.0 moved parseAnB() there
for the same reason.

Also in this commit, none of it behavioural:

- Drop .phpunit.result.cache, which was committed despite being in .gitignore.
- Restore the six blank lines the diff had stripped from released CHANGELOG
  sections. All five open PRs edit that file, so unrelated whitespace churn in it
  buys four conflicts for nothing.
- Record the find('*') self-match change in the CHANGELOG. It was needed to make
  is(':text') work on an element under the current is(), but it is a behaviour
  change that was going in unmentioned, and #73 supersedes it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jakejackson1
jakejackson1 marked this pull request as ready for review August 24, 2026 17:37
The guards were spread over seven call sites, three of which no caller could
reach: PseudoClass::elementMatches() and Util::matchesAttribute[NS]() are
only ever entered through matchAttributes(DOMElement $node, ...) and
matchPseudoClasses(DOMElement $node, ...), whose type hints already say what
the guards were re-checking. Those three are gone.

Every non-element that reaches the traverser arrives through the initial
match set, so the three initialMatchOn* shortcuts are the boundary and the
only place the check belongs. The reasoning is stated once, on initialMatch(),
instead of six times in six different wordings.

combineDirectDescendant() was reaching matchesSimpleSelector() with the
document itself whenever the left side of "a > b" matched the root element,
which is why the signatures had been widened from DOMElement to DOMNode. It
now skips non-elements the way combineAdjacent(), combineSibling() and
combineAnyDescendant() already did, so matchesSelector(),
matchesSimpleSelector() and combine() keep their DOMElement hints. Those are
public and cannot be re-narrowed later without a break, and a caller passing
a text node is better served by a TypeError at the boundary than a silent
false from four frames in.

PseudoClass::isTextInput() was a protected method whose body was a single
call to Util::isTextInput(), carrying a copy of that method's docblock. The
neighbouring switch arms call Util::matchesAttribute() directly; ':text' now
does the same.

Fold the three near-duplicate "selectors against a <kind> node do not throw"
tests into one loop over a fixture holding all four node kinds. The copies
had drifted -- only the text-node one checked ':first-child' and a descendant
selector, only the comment one checked ':text' -- so the shared battery is
the union of what they each asserted, and every kind is now held to the same
standard. Assertions go from 46 to 91.
@jakejackson1
jakejackson1 merged commit 91bd1e7 into main Aug 24, 2026
14 checks passed
@jakejackson1
jakejackson1 deleted the issue-49 branch August 24, 2026 18:33
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.

$singleTextNode->is(':text') throws

1 participant