Skip to content

Make search and count a single class over one set of instances #135

Closed

Description

@forgetso

Counting and replacing are two independent walks of the DOM today, and they disagree. They should be one pass over one set of instances, held in a dict that both read from.

The divergence

replaceInner counts with countOccurrences() against an element's innerHTML string; nodesUnder replaces with a TreeWalker. Different filters, different targets, and different regexes (searchPattern vs globalSearchPattern). Measured on a page with four occurrences of the term — visible text, a JSON <script>, a <style> rule and an href:

<p>European</p>
<script type="application/json">{"region":"European"}</script>
<style>.European { color: red; }</style>
<a href="/European-news">link</a>
actual expected
count, Replace HTML on 2 4
count, Replace HTML off 1 1 ✓
remaining after each Replace Next 2 → 1 → 1 → 1 → 0 → 0 4 → 3 → 2 → 1 → 0

All four do get replaced — the final DOM has the replacement in all four places — so the replacing is right and the counting is wrong. Two presses in a row leave the number unchanged, which reads to the user as "I pressed it and nothing happened".

Proposal

Collect the matches once into a keyed structure — element (or node) → its matches — and have both operations read from it:

  • count is the size of the collection
  • Replace All walks it
  • Replace Next takes the first outstanding entry and marks it done

The count then cannot drift from what replace will do, because there is one source of truth, and each press provably takes exactly one entry off.

Fixes a second bug for free

Key each instance to the innermost element containing it. Today the walker visits elements in document order with Replace HTML, so the first hit is a large ancestor and we rewrite its entire innerHTML — on a React page that destroys and recreates every node in the subtree, losing focus, selection, listeners and framework DOM identity. Scoping the rewrite to the innermost element means one small text change touches one small element.

Not in scope: excluding script and style

Worth recording, because it is tempting and wrong. Measured:

inline <script> re-ran? external <script src> re-fetched? <style> applied? text changed in DOM?
rewrite ancestor innerHTML no no yes yes
set script.textContent no no yes yes

Scripts never re-execute — per spec, script elements created by the fragment-parsing algorithm are flagged "already started". <style> content, by contrast, applies immediately. So replacing inside a <style> is a working feature, and replacing inside a <script> is a legitimate way to edit an embedded config; both should keep working. Replace HTML means the HTML, scripts and styles included — and the count must say so.

Acceptance

Extend searchreplace.invariants.test.ts, which already runs a page × options matrix:

  • the remaining count falls by exactly one per Replace Next, on every page shape and option set
  • with Replace HTML on, matches in <script>, <style> and attributes are counted
  • replacing an instance leaves the rest of the subtree's nodes identical (same object identity), so frameworks are not disturbed

Background: #133, and the store report about Jira changes only appearing after clicking save.

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

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions