Count and replace from one set of instances - #136
Merged
Merged
Conversation
Counting measured an element's innerHTML as a string; replacing drove a TreeWalker. They disagreed, and the disagreement was visible: a page with a match in a script and one in a style reported 2 matches and replaced 4, so Replace Next went 2 -> 1 -> 1 -> 1 -> 0, appearing to stall on the matches the count had never admitted to. The two cannot be reconciled by patching either side. Making the count read the unfiltered original — so it stops missing the script and the style — takes the invariant suite from 4 failures to 13, because the count then disagrees about hidden text and input values instead. An innerHTML string cannot express scope: the text of a hidden element, of a script and of an input are all in it, whether or not any of them is in scope for this search. So collect the matches once, from nodes rather than strings, and have both operations read the list: text a text node, which covers prose and script and style contents attribute an attribute value, in scope only when replacing HTML markup a match that exists only in the tags, e.g. searching for "<p" count is the size of the list; Replace All walks it; Replace Next takes the first entry and replaces exactly one occurrence. The count cannot drift from what replace will do because there is one source of truth. Writing through the node holding the match fixes two further things: - A replacement no longer rewrites an ancestor's innerHTML, which destroyed and recreated every node beneath it, taking the page's listeners, selection, focus and framework DOM identity with it. - "Hidden content" now means something in HTML mode. The known limitation recorded in the invariant tests — that Replace HTML rewrote hidden text regardless — is fixed, and the test that pinned it now asserts the opposite. Markup matches keep working. Anything in an element's innerHTML that the text and attribute instances cannot explain is charged as a surplus to the innermost element that can account for it, walking in reverse document order. An element whose subtree holds anything out of scope is never given one, since its innerHTML describes that content too and rewriting it would drag it back in. Deletes replaceInner, nodesUnder, isIgnored, equivalentInIgnoredElements, containsAncestor and getHiddenElements: the clone-and-filter machinery existed only to keep unwanted subtrees out of an innerHTML rewrite, and nothing is rewritten wholesale any more. 787 tests, 16 of them new. The invariant matrix gains "each Replace Next takes exactly one match off the count", which now holds for every page shape and option set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
forgetso
force-pushed
the
count-replace-instances
branch
from
September 21, 2026 22:20
0ae2102 to
4b270ba
Compare
The Cypress suite caught what the unit tests did not: searching "<div" on the e2e fixture reported 2 where it expects 18. Charging markup matches to an element's innerHTML was wrong twice over. An ancestor's innerHTML repeats every descendant's tag, so the arithmetic to avoid double counting depended on the taint check, and on a real page almost every container holds a script or a hidden element — so almost every markup match was dropped. Matches in markup now belong to the element whose tags they are. An element's own tags are `element.cloneNode(false).outerHTML`, children excluded, so each element accounts for itself and nesting cannot double count. Attribute matches come off the total, since attribute values sit inside the opening tag. Replacing one rebuilds the element from the rewritten tag and moves the existing children across rather than assigning to outerHTML, which would reparse them and hand back new nodes. Descendants keep their identity. If the rewritten tag does not parse to exactly one element the element is left alone, rather than replacing the page with rubble. The innerHTML fallback stays for terms that span a tag boundary, which no single tag, attribute or text node holds — `<span>x</span>` is the existing test. That is the only case still using the destructive path, and the taint check still guards it. The root element's own tags are excluded: the search is of what is inside the root, as innerHTML always was. Counting them means "<b" matches "<body>", and replacing that swapped out the body element. Two Cypress expectations pinned behaviour this work deliberately changes, and now assert the corrected version: script contents are in scope for Replace HTML wherever the script sits, and text hidden by a stylesheet is out of scope unless Hidden content is set. Both previously depended on where the element happened to sit in the tree rather than on the options. Adds searchreplace.fixture.test.ts, which counts the Cypress fixture from `npm test`. The e2e specs only run in CI, so this regression was invisible until the pull request went red. 792 unit tests and all 30 end-to-end tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #135.
Counting measured an element's
innerHTMLas a string; replacing drove aTreeWalker. They disagreed, and the disagreement was visible.Note
Based on #133, not
master, because both touchsearchreplace.ts. Sibling of #134 rather than stacked on it.The measurement
Page with four occurrences — visible text, a JSON
<script>, a<style>rule, anhref:All four were always replaced; only the count was wrong. Two presses in a row left the number unchanged, which reads as "I pressed it and nothing happened".
Why patching one side doesn't work
I tried the one-line version first: make the count read the unfiltered original so it stops missing the script and the style. That takes the invariant suite from 4 failures to 13 — the count then disagrees about hidden text and input values instead.
An
innerHTMLstring cannot express scope. The text of a hidden element, of a script and of an input are all in it, whether or not any of them is in scope for this search. So the fix has to move off strings.One list, three kinds
collectInstances()walks the page once and returns where the term was found:textnode.dataattributesetAttributemarkupinnerHTML<pcountis the size of the list, Replace All walks it, Replace Next takes the first entry and replaces exactly one occurrence. The count cannot drift from what replace will do because there is one source of truth.Two more fixes that fall out
Replacements stop destroying subtrees. Writing through the node holding the match means an ancestor's
innerHTMLis never reassigned, so the page keeps its listeners, selection, focus and framework DOM identity. New invariant: a match in one element does not recreate its siblings."Hidden content" now means something in HTML mode. The known limitation recorded in the invariant tests — that Replace HTML rewrote hidden text regardless of the option — is fixed. The test that pinned it now asserts the opposite, with a companion showing hidden text is rewritten when the option is on.
Markup matches still work
Searching
<p, or<span>x</span>→<b>y</b>, is a real Replace HTML capability and two existing tests cover it. Anything in an element'sinnerHTMLthat the text and attribute instances cannot explain is charged as a surplus to the innermost element that can account for it, walking in reverse document order so a descendant is considered before its ancestor.An element whose subtree holds anything out of scope never gets one: its
innerHTMLdescribes that hidden text or that script too, and rewriting it would drag them back into the replacement. That taint check is what stops the markup fallback reintroducing the bug it sits next to.Application order matters for the same reason — a markup instance recreates its subtree and detaches the text nodes other instances hold, so everything else is written first and markup instances go deepest-first.
Deletions
replaceInner,nodesUnder,isIgnored,equivalentInIgnoredElements,containsAncestor,getHiddenElements. The clone-and-filter machinery existed only to keep unwanted subtrees out of aninnerHTMLrewrite; nothing is rewritten wholesale any more. Net −279 lines of the old path.Checks
npm run checkspasses — typecheck, lint, format, 787 tests, build. 16 new, including the invariant matrix gaining "each Replace Next takes exactly one match off the count", which now holds for every page shape × option set.Known edge
Escaped characters. A text node's data is unescaped where
innerHTMLis not, so searching for a bare&or<can make the markup surplus arithmetic over- or under-count by one on an element containing escaped entities. It is clamped at zero so it cannot produce a negative count. Worth a follow-up if anyone hits it; I did not want to widen this change further.🤖 Generated with Claude Code