Skip to content

fix(core): a variable-bound title split into words shows once - #5844

Open
miguel-heygen wants to merge 4 commits into
mainfrom
scrubfix/vartextsplit
Open

miguel-heygen wants to merge 4 commits into
mainfrom
scrubfix/vartextsplit

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

What

A title bound with data-var-text that the composition's own script splits into word or character spans (for a word-by-word reveal) shows once. Before this, snapshot, preview and render showed the full text twice: Launch day is hereLaunch day is here, the second copy being the animated words.

Why

applyVariableBindings ran at init, which is after the composition's inline scripts. By then the script had already replaced the span's text with word spans, so the span had element children and no text node of its own. setOwnTextPreservingChildren then inserted the full value in front of the word spans. With an override (--variables '{"title":"…"}') it was worse: the script split the authored text, and the override was added in front of it.

How

  • installEarlyTextBindings() (runtime entry, next to installAuthoredOpacityCapture, which uses the same pattern) binds data-var-text from a MutationObserver while the page parses. The parser runs a microtask checkpoint before each inline script, so the binding lands before the composition script reads the text, and the script splits the variable's value. The observer disconnects at DOMContentLoaded.
  • Each element remembers the text it was bound to. The init pass, and the re-pass after sub-compositions are inlined, skip an element already bound to the same value, so they no longer write over or beside a split. A changed value is still written as before.
  • When the composition loader empties a bound host to mount its scene (resetCompositionHost), it forgets that host's bound value (forgetBoundText), so the next binding pass writes the text again beside the mounted scene, as on main.
  • Variable overrides are already set before any composition script, by the engine (evaluateOnNewDocument) and by the preview (injectPreviewVariables), so the early binding sees them.

Related work

#5158 also edits applyVariableBindings.ts (per-mount variable values for sub-compositions). Different change, and it may need a rebase after either lands.

Test plan

  • Unit test added: applyVariableBindings.test.ts "does not write a bound text again after a script split it into word spans" fails on main and passes with the fix.
  • Browser test added: coreRuntimeBrowser.test.ts "shows a bound title once when the composition splits it into words", with the default value and with an override. The page is loaded by navigation, because setContent writes through document.write with no checkpoint before scripts. Both cases fail on main (Launch day is hereLaunch day is here), and the 55 browser tests pass 3 runs in a row with the fix. The core runtime suite passes (2111).
  • Loader test added: compositionLoader.test.ts "restores a host's bound text after mounting its composition clears the host" (bind, mount an inline template into the same host, re-bind; expects both the bound text and the scene). It passes on main, fails at 752990a7, and passes with the fix. Runtime suite 2112 and browser tests 58 pass 3 runs in a row.
  • Manual testing performed: a project whose script splits a bound title into words. snapshot on the current release shows the title twice. From this branch, snapshot shows it once, and render --variables '{"title":"Doors open at nine"}' shows the override once, split into words.
  • Documentation updated (if applicable): not applicable.
  • Comments follow CONTRIBUTING.md "Comments": one doc line on the new function, one line on why the bound value is remembered, one in the test on why it navigates.

Comment thread packages/core/src/runtime/applyVariableBindings.test.ts Fixed
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 177 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • seqheldtwice-tlhold-px-r0-nested-z100: tracking 0.41, pressJump 60.41, drop 0.39, reload 0.39, render 0.02, renderKey -, undo true, teleport true / tracking 0.09, pressJump 0.11, drop 0.39, reload 0.39, render 0.02, renderKey -, undo true, teleport true / tracking 0.09, pressJump 0.11, drop 0.39, reload 0.39, render 0.02, renderKey -, undo true, teleport true

data-var-text was applied at init, after composition scripts ran, so a span
the script had split into word spans got the full text again beside them.
Text bindings now apply as the page parses, before those scripts, so the
script splits the variable's value, and a later pass skips an element
already bound to the same value.

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source-only re-review of HEAD 752990a7a65e80fbb8bf252cd0df489be8ec07ca against API/effective BASE ab2e939558ba073ab725530d98f47a791ea6942d. All four changed BASE files were read fully before all four changed HEAD files; the loader, variable resolution and init/recovery call sites were also traced. Both independent reviewer finals were received. No tests, browser/native execution or CI queries were performed.

The split-span unit regression now constructs elements safely (applyVariableBindings.test.ts:58–73), addressing the historical CodeQL test-construction concern without repeating it as a new finding.

[P2] Invalidate the text memo when a persistent composition host is reset

packages/core/src/runtime/applyVariableBindings.ts:200–203: equality with the last applied scalar does not mean the host still contains that text. With a top-level title = "Bound", an empty <div data-composition-id="scene" data-var-text="title"></div> and its template#scene-template containing an inner composition with a <span>child</span>, the first init binding writes and memoizes Bound. The inline loader still accepts this text-only host (compositionLoader.ts:674–680), then resetCompositionHost removes all its children while retaining the same Element and binding attribute (213–218, 692). Mounting appends the composition to that same host. With no scoped defaults/overrides, both passes resolve the same top-level value.

When settleSceneDom reapplies bindings (init.ts:3584), HEAD returns at the memo check, leaving only child; BASE's own-text writer restores Bound before the mounted child, preserving both. This is ordinary runtime mounting, not intentional word splitting and not a parser-checkpoint hypothesis. Invalidate/reconcile this memo at destructive runtime resets (while preserving the split-span behavior), and cover that loader/rebind sequence with a regression test.

The partly parsed-element/scoped-initialization timing question remains unverified and is not counted as another finding. This finding is a source-derived transition witness, not an executed browser reproduction.

Verdict: REQUEST CHANGES
Reasoning: The new same-value memo suppresses an existing recovery binding after the runtime clears and reuses the bound host, causing previously restored visible text to disappear.

— Review by tai (pr-review)

Fresh public-state round at the same immutable pins: prior full source reads were explicitly rebound to identical tree paths, Git blob SHAs and raw byte counts, and the reset/rebind witness was rechecked. The prior no-post round remains preserved. The changed bot status is not a code defect or release hold.

The same-value memo skipped re-binding a host the loader had just emptied to
mount its composition, so the host's bound text disappeared. Resetting a
composition host now forgets its bound text, so the next binding pass writes
it again; a script's word split still is not re-written.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

@terencecho Fixed at bc4c7400. Confirmed your witness first: a host with data-var-text, bound at init, then emptied by the loader to mount its inline template, kept only the scene after the re-bind at 752990a7, while main shows both.

The fix is at the destructive reset you pointed to. resetCompositionHost is the one place all three mount paths empty a host, and it now calls forgetBoundText(host) (exported next to the memo in applyVariableBindings.ts). The next binding pass then writes the bound text again before the mounted scene, as on main. A composition script's word split is not a reset, so it still isn't written over.

Regression test: compositionLoader.test.ts "restores a host's bound text after mounting its composition clears the host" runs bind, loadCompositions and re-bind through the real loader, and expects both Bound and child. It passes on main, fails at 752990a7 (expected 'child' to contain 'Bound'), and passes now. The runtime suite (2112) and the browser tests (58, including both split-title cases) pass 3 runs in a row.

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact HEAD bc4c7400d6d7d8ab80c942456a50eb147d83ddaa against API/effective BASE ab2e939558ba073ab725530d98f47a791ea6942d.

The previous retained-host reset finding is repaired: resetCompositionHost now calls forgetBoundText(host) before clearing children, covering the inline mount, external mount and external-failure cleanup paths. The new real-loader test exercises bind → mount into the same Element → rebind. The split-span test also retains the safe element construction. I am not carrying either historical concern forward as a current defect.

[P2] Do not finalize the text memo while the parser is still populating the element

packages/core/src/runtime/applyVariableBindings.ts:205–208 memoizes a scalar during the new parsing observer and makes all subsequent same-value passes no-ops. This is not limited to elements that a composition script intentionally split.

Consider an ordinary navigation with the runtime installed synchronously in <head>, a declared string variable title defaulting to Bound, and this body content inside a normal composition root:

<h1 data-var-text="title">Fallback<script type="application/json">{}</script> suffix</h1>

At the nested script's end, the HTML parser performs a microtask checkpoint before preparing the script; the unsupported application/json type aborts preparation only afterward. With the usual empty JS execution-context stack and a non-reentrant checkpoint, the observer therefore visits the unfinished h1, writes/memoizes Bound, and preserves its script child. Parsing then resumes and inserts suffix after that child. The observer scans only addedNodes, accepts only Elements (216–226), and never revisits the bound parent for that Text addition. The later init and scene-settlement bindings both return at the scalar-equality check. The visible own text is consequently Bound suffix, whereas BASE's late own-text normalization clears the extra direct Text node and displays Bound. (textContent also includes the inert script's {}; it is not the visible-text assertion.)

This needs neither a text-splitting author script nor a composition-host remount. The root-only/no-template case does not reach the new reset invalidation. The compiled-document path also inserts the runtime in the head and does not classify this no-src JSON script as executable for hoisting/coalescing; this trace assumes ordinary DOM parsing/serialization preserves that nested script/Text structure.

Distinguish parser completion from intentional post-binding DOM ownership before treating equal scalar values as sufficient to skip normalization. Add a navigation regression for an inert script nested inside a bound element, retaining both the existing word-split and loader-reset regressions. Merely rescanning the mutation target is insufficient if the same-value check still prevents the corrective write.

Evidence and limits: Source/spec-derived BASE-relative transition witness, not an executed browser reproduction. The checkpoint-before-prepare rule was inspected in immutable historical W3C HTML source (2ef7a579bcfd2693b98df35f20e51dff60e3fbef, sections/syntax.include:5263–5277), together with script type rejection, microtask draining and DOM observer delivery. This is not a current Chromium implementation certification. The transitive linkedom parser/serializer was not audited; ordinary preservation of this structure is an explicit dependency contract assumption, not a measured compiled-output result. Parent FULL coverage of every existing changed BASE blob preceded changed HEAD reading; all changed HEAD blobs are FULL, with exact immutable prior-own reuse recorded where applicable. Both independent source passes and the focused checkpoint follow-up were received. No tests, builds, browser/native processes or CI queries were run; author test/gate claims are not independently certified.

Verdict: REQUEST CHANGES
Reasoning: The new same-value memo can preserve parser-appended fallback text that BASE removed, independently of the repaired retained-host reset lifecycle.

— Review by tai (pr-review)

The memo skipped every later pass with the same value, so text the parser
appended inside a bound element after an inert nested script stayed visible.
The memo now keeps the text node it wrote: a later pass skips only when that
node no longer holds the value (a composition script took the text over, as
when it splits words), and otherwise writes again, clearing the extra text.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

@terencecho Fixed at 69424335. Reproduced your witness first in real Chrome (navigation from a file, runtime in <head>): <h1 data-var-text="title">Fallback<script type="application/json">{}</script> suffix</h1> showed own text Bound suffix at bc4c7400.

The fix tells parser completion from script ownership by the node, not the value: the memo now keeps the text node it wrote (setOwnTextPreservingChildren returns it). A later same-value pass skips only when that node no longer holds the value under the element, which is what a word-splitting script does (it empties or removes the text it was given). While the node still holds the value, the pass writes again, and the existing writer clears any later direct text, as on main. A host reset still forgets the memo.

New navigation regression in coreRuntimeBrowser.test.ts: "shows only the bound title when the parser adds text after an inert script inside it" expects own text Bound. It fails at bc4c7400 and passes now. Both word-split cases and the loader-reset test still pass; disabling the skip fails both word-split cases. Browser tests 59 and the core runtime suite 2112 pass 3 runs in a row.

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed HEAD 694243355ec5f2404ab21cf5292e2f828eddc6d3 against effective BASE ab2e939558ba073ab725530d98f47a791ea6942d.

The node-aware check fixes the retained inert-script/separate-text-node case covered in packages/producer/src/services/coreRuntimeBrowser.test.ts:453–481. The shared reset invalidation (packages/core/src/runtime/compositionLoader.ts:214–220) and safe span construction (packages/core/src/runtime/applyVariableBindings.test.ts:58–73) also address the earlier concerns; I am not carrying those findings forward.

[P2] Parser coalescing can still be mistaken for script-owned text

packages/core/src/runtime/applyVariableBindings.ts:205–210

The same-value guard also skips when the parser, rather than a text-splitting script, changes the remembered Text node. With the runtime loaded synchronously in <head> and the declared title default set to Bound, use this inside an ordinary root composition:

<h1 id="title" data-var-text="title">Fallback<script>document.currentScript.remove();</script> suffix</h1>

The source/spec transition is:

  1. The closing-script microtask checkpoint lets the early observer bind the unfinished heading. Since the nested script still exists, the writer preserves it, changes the first direct Text to Bound, and remembers that Text node (applyVariableBindings.ts:105–116,215–229).
  2. The inline script removes only itself; it does not split or otherwise edit the heading's text.
  3. Parsing resumes at the end of the heading. Without the script separator, the parser appends suffix into the same remembered Text node, rather than creating a second one.
  4. At init, prior.text === text is still true, but prior.node.nodeValue !== text. The guard labels this scriptOwnsText and returns. Visible text remains Bound suffix; BASE's late binding writes Bound. Subsequent same-value binding passes skip too. This fixture needs no external/template host mount, so host-reset invalidation does not rescue it.

The parser rule and explicit self-removing-script AB example are in the immutable HTML parsing source, lines 3978–4012; the pre-script checkpoint is at lines 5263–5277.

Please distinguish parser continuation from actual script-owned text before skipping the late bind, and cover this self-removing-separator variant alongside the retained inert-script case. The current inert-script regression leaves its separator in place, so it exercises a different Text-node layout.

Scope: source-only review of all six changed files at both pins, with all BASE files read before HEAD, plus two completed independent passes. This finding is a source/spec-derived transition, not an executed browser reproduction or current Chromium implementation certification. No tests, browser, builds, or CI queries were run; author/bot pass claims were not independently verified.

Verdict: REQUEST CHANGES
Reasoning: The repaired inert-script case is clear, but same-node parser continuation can still leave authored fallback suffix text in the bound title instead of the scalar value that BASE supplies.

— Review by tai (pr-review)

This branch has not been deployed

No deployments
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.

3 participants