[BUG] Attribute/reveal route still walks a revealed node as its own walk root inside a translated paragraph (#2185 for the notranslate family)
Summary
PR #2186 stops handleMutationRecords from walking an added node as its own walk root when that node sits inside a registered bilingual source — the #2185 defect. The sibling attribute/reveal branch was left unguarded, deliberately, and it reproduces the same defect for one family of blockers: an element that stops matching isDontWalkIntoButTranslateAsChildElement (the notranslate family) while inside an already-translated paragraph.
The naive fix — applying the same guard there — is wrong and would strand content. The correct fix is narrower. This issue records both.
References are against cdcee15c (head of #2186).
Where
findEnclosingBilingualLayoutSource is imported and called in exactly one place in src: page-translation.ts:1192, inside the rec.type === "childList" branch. The sibling branch has no such check:
} else if (this.isWalkabilityAttributeMutation(rec)) {
const el = rec.target
if (!isHTMLElement(el)) continue
for (const unblocked of this.collectNewlyWalkableSubtrees(el, config)) {
void this.observeTopLevelParagraphs(unblocked, config) // ← fresh walk rootobserveTopLevelParagraphs is exactly the "fresh walk root" primitive #2186 identifies as the root cause: it computes "top-level paragraph" relative to its walk root, and walkNode (traversal.ts:185-190) labels any element with a non-blank direct Text child, inline spans included.
Nothing downstream rejects the promoted unit:
observeParagraphUnit(page-translation.ts:752-800) does no enclosing-source check.- The walker's already-translated guard (
translation-walker.ts:62-66) iselement.querySelector(...)— self and descendants only. The enclosing wrapper is a sibling of the revealed span, so it does not fire. - The run-level wrapper lookup at
translation-modes.ts:1060isparentNode.closest(...), which is null for a span whose sibling holds the wrapper.
Verified by execution (temporary jsdom probe against page-translation-mutations.test.ts, since removed): revealing a cached-blocked span inside a registered source produces a fresh walk root on the span, sets data-read-frog-paragraph on it, calls IntersectionObserver.observe(span), and schedules zero retranslations of the enclosing source.
Why the enclosing source never rescues it
This is the part that makes the obvious fix wrong.
collectHostText (translation-state.ts:277) counts hidden and notranslate text — it only excludes translation wrappers. But collectRawSource (paragraph-segmentation.ts:173-188) drops isDontWalkIntoAndDontTranslateAsChildElement children entirely.
So revealing a blocked element changes what is translatable without changing collectHostText. The enclosing source stays current, findStaleBilingualLayoutSource never resolves it, and retranslateChangedSource is never scheduled.
That cuts both ways, and the two blocker families need opposite treatment:
| revealed element | its text before the reveal | walking it is |
|---|---|---|
display:none / hidden family (isDontWalkIntoAndDontTranslateAsChildElement) |
never in the enclosing translation | a rescue — must keep happening |
notranslate family (isDontWalkIntoButTranslateAsChildElement) |
already rendered by the enclosing wrapper, verbatim, as a preserved atomic chunk (paragraph-segmentation.ts:184-188) |
a duplicate — the bug |
Applying findEnclosingBilingualLayoutSource wholesale here would kill the first row — revealed accordion / "show more" content would be stranded permanently, exactly the dead zone #2186's own comments warn about for translationOnly.
Reproduction
<p id="summary">Original <span id="brand" class="notranslate">Acme Corp</span> summary</p>- Translate the page in bilingual mode.
#summarygets one wrapper; "Acme Corp" appears verbatim inside it (preserved atomic chunk). document.getElementById('brand').classList.remove('notranslate')#brandtransitions blocked → walkable,collectNewlyWalkableSubtreesreturns it, and it is walked as its own root.- Count
document.querySelectorAll('#summary .read-frog-translated-content-wrapper').length— expected 1, observed 2, the second inside#summaryat#brand, re-translating a fragment the first wrapper already rendered.
Note #brand must be in walkBlockedElementsCache for didChangeToWalkable (page-translation.ts:907-919) to return true. That happens on the initial walk via onBlockedElement (page-translation.ts:699), so a pre-existing blocked descendant qualifies — the addition does not need to be new.
Not confirmed in a real browser. Step 4's wrapper count is inferred from the guard analysis above; the jsdom probe could not prove it because translateNodesBilingualMode is mocked in that suite. Worth confirming before closing.
Proposed fix
Inside the collectNewlyWalkableSubtrees loop, skip observeTopLevelParagraphs(unblocked, config) when both:
findEnclosingBilingualLayoutSource(unblocked)resolves, and- the element was blocked by the
isDontWalkIntoButTranslateAsChildElementfamily — i.e. its text was already folded into the enclosing source's chunk stream.
Keep observeIsolatedDescendantsMutations(unblocked, config) either way, as the childList branch does.
Condition (2) needs the reason an element was cached as blocked, which is not currently stored — walkBlockedElementsCache (page-translation.ts:145) is a plain WeakSet. Note cacheWalkBlockedElement has five call sites (:699, :913, :928, :1080, :1375), and two of them (:928, :1080) decide "blocked" through the collapsed isWalkBlockedElement boolean with no reason available to thread. So threading a reason through onBlockedElement only covers one call site; recomputing the family at cache time is the only option that covers all five.
An alternative worth weighing (not validated): force the enclosing source stale on this transition and let retranslateChangedSource re-run it. That would repair placement too, but its interaction with the #1831 tamper/churn caps was not checked, and retranslateChangedSource's behavior when collectHostText is unchanged was not traced.
Suggested tests
In page-translation-mutations.test.ts — its mock walk now mirrors the real labelling rule, so an inline span can be promoted under it:
- Registered source containing a blocked span, span revealed → assert
observer.observeis never called with the span, and the enclosing source is not retranslated. - Same shape with a
hidden-family blocker → assert the span is walked (the rescue must survive).
Two hazards:
- Prefer a distinct class (e.g.
np) over the literalnotranslatewhen configuringmockIsDontWalkIntoButTranslateAsChildElement; two existing tests do key on the literal and compensate with amockHasNoWalkAncestorimplementation, which is easy to get subtly wrong. - The existing
retains blocked stale sources until %scase reveals an ancestor of a registered source, not a descendant inside one — it is not a counterexample to "no coverage for a reveal inside a registered source".
Confidence
The code chain is verified line by line at cdcee15c, plus one executed jsdom probe (above). Real-world frequency is unmeasured — I have no site that demonstrably removes a notranslate class mid-session. The display:none/hidden family is far more common and is not affected, so this is plausibly rarer than #2185 itself. Prioritize accordingly.
Follow-up to #2186 / #2185.
Source: mengxi-ream/read-frog