fix(morphdom): preserve checkbox/radio checked state across updates

[adnaan]

Summary

• Preserves checkbox and radio checked state in morphdom's onBeforeElUpdated hook so that user-toggled checkboxes survive server-driven re-renders (e.g. 2-second scan loops)
• Adds data-lvt-force-update escape hatch so the server can override preserved state when needed
• Fixes isEqualNode optimization to not short-circuit when data-lvt-force-update is present

Context

Without this fix, any morphdom diff cycle that touches a form with checkboxes resets user-checked state because checked is a DOM property (not an attribute) and morphdom's attribute-diffing replaces the element. This is the framework-level fix — consumers no longer need lvt-preserve-attrs workarounds on individual checkboxes.

Test plan

• Unit test: checkbox checked state preserved across morphdom updates
• Unit test: radio button checked state preserved across morphdom updates
• Unit test: data-lvt-force-update overrides checkbox preservation
• All 366 existing tests pass

🤖 Generated with Claude Code

[claude]

Review of PR #81 - preserve checkbox/radio checked state

The fix is correct and well-motivated. A few actionable points:

1. Potential gap: indeterminate state
The same root cause (DOM property vs HTML attribute) applies to the indeterminate property on checkboxes. If the codebase uses tri-state checkboxes (e.g. select-all patterns), that property will also be wiped on re-render. Consider copying it alongside checked in the preservation block.

2. Redundant cast in isEqualNode block
The toEl instanceof Element guard and (toEl as Element) cast are dead code -- toEl is already typed as Element per morphdom's onBeforeElUpdated contract. toEl.hasAttribute('data-lvt-force-update') alone is sufficient.

3. Test gap: radio mutual exclusion
No test covers the case where a user selects radio B, then a server update includes radio A pre-checked (without data-lvt-force-update). The per-element property copy should handle this correctly, but an explicit test would prevent regressions.

4. Subtle ordering note
The focused-element bail-out (return false) sits after the checkbox preservation block, so fromEl.checked = toEl.checked via data-lvt-force-update fires even when the element is focused. The checked property is silently updated without morphdom touching the rest of the element. Probably fine for checkboxes (they lose focus on click), but worth a comment if intentional.

The indeterminate gap is the only functionally meaningful concern; the rest are minor.

[Copilot]

Pull request overview

This PR updates the LiveTemplateClient’s morphdom integration to preserve user-toggled checkbox/radio checked state across server-driven re-renders, while providing an explicit escape hatch (data-lvt-force-update) to let the server override preserved state when needed.

Changes:

• Preserve checkbox/radio checked DOM property across morphdom updates (unless data-lvt-force-update is present).
• Adjust the isEqualNode short-circuit to not skip updates when data-lvt-force-update is present on the element.
• Add unit tests covering checkbox/radio preservation and the force-update override behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
livetemplate-client.ts	Extends onBeforeElUpdated to preserve checkbox/radio state and refines isEqualNode optimization to honor data-lvt-force-update.
tests/preserve.test.ts	Adds tests verifying preservation for checkboxes/radios and that data-lvt-force-update can override preservation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment about data-lvt-force-update says "sync fromEl to toEl instead", but the code path actually applies fromEl.checked = toEl.checked (syncing toEl → fromEl). Please update the comment to match the actual direction to avoid confusion for future maintainers.

Suggested change

	// wants to reset the checkbox, so we sync fromEl to toEl instead.
	// wants to reset the checkbox, so we sync toEl to fromEl instead.

[Copilot]

data-lvt-force-update is only checked on the current element in the isEqualNode short-circuit. If an ancestor element is isEqualNode(toEl) and does not have data-lvt-force-update, this hook returns false on that ancestor and morphdom won’t traverse into children—so a descendant checkbox/radio with data-lvt-force-update may never be processed, and its checked state can’t be reset when only the DOM property differs. Consider also bypassing the isEqualNode short-circuit when the incoming subtree contains a [data-lvt-force-update] descendant, or document/enforce that the attribute must be placed on an ancestor that will be visited (not only on the input).

Suggested change

	// to process this element unconditionally (e.g. resetting a
	// checkbox whose checked property differs from the attribute).
	if (
	fromEl.isEqualNode(toEl) &&
	!(
	toEl instanceof Element &&
	(toEl as Element).hasAttribute("data-lvt-force-update")
	)
	) {
	// to process this element or one of its descendants
	// unconditionally (e.g. resetting a checkbox whose checked
	// property differs from the attribute).
	const hasForcedUpdateInSubtree =
	toEl instanceof Element &&
	(toEl.hasAttribute("data-lvt-force-update") ||
	toEl.querySelector("[data-lvt-force-update]") !== null);
	if (fromEl.isEqualNode(toEl) && !hasForcedUpdateInSubtree) {

[claude]

Code Review

Overview: Preserves checkbox/radio checked state across morphdom scan-loop re-renders by copying DOM state onto the incoming element, with a data-lvt-force-update escape hatch for server-driven resets. The approach is sound and tests are thorough.

Potential Issues:

1. O(n^2) querySelector on every morphdom element - toEl.querySelector("[data-lvt-force-update]") runs a full subtree scan on every element during morphdom traversal. For large DOMs this is O(n^2). May be fine for typical template trees but worth flagging.

2. Radio mutual-exclusion comment may be inaccurate - The comment says syncing the checked attribute on toEl prevents radio mutual-exclusion side effects, but toEl is an out-of-DOM clone at this point in the morphdom callback, so the browser will not propagate mutual-exclusion across the radio group. The attribute sync still ensures morphdom attribute-diffing sees no delta (the real goal) - worth correcting the comment.

3. User selection silently wins over server checked attribute on radios - If the server sends checked on a different radio, the user selection wins anyway (unless data-lvt-force-update is present). Server authors may not expect this. A brief note explaining why user-wins is the right default would help future maintainers.

4. toEl instanceof Element check is redundant - At the hasForcedUpdateInSubtree callsite, toEl is already an HTMLElement per the morphdom API so the instanceof Element guard is always true.

Positives: Fix is well-scoped and does not disturb the existing focus-preservation path. Six targeted tests cover checkbox, radio, indeterminate, force-update, and the ancestor short-circuit case. The data-lvt-force-update escape hatch is a clean opt-in API.

The querySelector O(n^2) concern is the only one worth flagging before merging on pages with large frequently-updated DOMs. Otherwise this looks good.

[claude]

Review of PR #81 - Preserve checkbox/radio state across morphdom updates

Overall the approach is sound and well-tested. A few things worth considering:

Potential bug: force-update radio ordering

When data-lvt-force-update is on one radio and the server checks it, the code sets fromEl.checked = true mid-loop. This immediately unchecks sibling radios in the DOM via browser mutual-exclusion. If a sibling was already processed before the force-updated radio, its toEl.checked was set from fromEl.checked (true at that point) and will not be revisited. Both radios could end up unchecked after morphdom finishes. Low-impact in practice but worth noting as a known limitation, or handling the name-group atomically.

Performance: querySelector in the isEqualNode skip path

toEl.querySelector('[data-lvt-force-update]') runs for every element where isEqualNode returns true. For large lists with many stable rows this is a full subtree scan per morph cycle. Consider caching which containers have a force-update descendant, or skipping the scan for leaf elements.

Misleading test comment

In the 'data-lvt-force-update overrides checkbox preservation' test, the comment says the parent must differ so isEqualNode returns false on the parent. But the descendant querySelector check added to the isEqualNode path should bypass that requirement anyway. The v1/v2 span change makes the test pass via a different code path. The last test covers the intended path correctly; this comment is misleading.

Minor: No test for data-lvt-force-update on a radio input, relevant given the ordering note above.

[claude]

Review: Checkbox/radio state preservation

The overall approach is solid — intercepting onBeforeElUpdated before the focus check, syncing toEl attributes to prevent spurious morphdom mutations, and the data-lvt-force-update escape hatch are all sensible.

One functional concern

data-lvt-force-update is a persistent attribute — morphdom copies it from toEl to fromEl during its attribute pass, so it stays in the DOM until the server explicitly removes it. This means a server that wants a one-shot checkbox reset must remember to drop the attribute on the very next render or it will keep overriding user input every update. Nothing enforces this, and there's no test covering the add→remove lifecycle. Either:

• document this "sticky unless server removes it" contract explicitly, or
• strip the attribute from fromEl inside the force-update branch so it self-clears without server coordination.

Performance nit

toEl.querySelector("[data-lvt-force-update]") === null

This fires on every isEqualNode-true pair — intended as a fast-path skip. On a page with many large stable subtrees, that's an O(n) scan per stable node, making the skip O(n²) overall. Low risk today but worth an eye on it at scale.

Everything else looks good — logic is correct, the checked-attribute sync to prevent spurious radio mutual-exclusion is the right fix, indeterminate handling is correct, and test coverage is thorough (especially the isEqualNode bypass test).

[claude]

Good fix overall — logic is sound and test coverage is solid. Three issues worth addressing:

1. lvt-updated lifecycle hook fires spuriously on force-update ancestors

When fromEl.isEqualNode(toEl) is true but a descendant has data-lvt-force-update, the code no longer returns false, which means it falls through to this.executeLifecycleHook(fromEl, "lvt-updated"). That hook now fires on the ancestor even though the ancestor's own content didn't change — only its descendant was force-updated. If any lvt-updated hook has side effects (e.g. re-initialising a component), it'll trigger incorrectly. Consider skipping the hook call for this case (e.g. tracking whether the element itself changed vs. just traversed-for-a-descendant).

2. The querySelector test doesn't actually exercise the querySelector code path

The "data-lvt-force-update on descendant bypasses ancestor isEqualNode short-circuit" test uses the same tree object for both renders. After the first updateDOM, onElUpdated strips data-lvt-force-update from the live DOM's input. On the second render, fromInput has no attribute but toInput does — so fromEl.isEqualNode(toEl) is already false for the ancestor div, and the querySelector branch is never reached. The test passes, but for the wrong reason.

The actual scenario the querySelector is needed for is: an element with data-lvt-force-update arrives via onNodeAdded (never stripped), then appears again in the next render with the same attribute — isEqualNode returns true for both the element and its ancestor. A test covering that path is missing.

3. onNodeAdded doesn't strip data-lvt-force-update

onElUpdated strips the attribute (one-shot semantics), but onNodeAdded does not. Newly inserted nodes enter the live DOM with the attribute intact, which is why the querySelector fallback in issue #2 is needed at all. Adding a strip in onNodeAdded too would make the cleanup consistent and simplify the invariant: "after any morphdom pass, no live DOM node retains data-lvt-force-update."

[claude]

Review

Good overall — solid test coverage and the force-update escape hatch is a clean design. A few things worth addressing:

Bug: Misleading test comment (preserve.test.ts)

The test data-lvt-force-update on newly added node is stripped after first render has an opening comment that says:

"Subsequent renders treat the checkbox normally (user selection wins)."

But the test itself expects cbAfter.checked to be false (force-reset), and the inline comment confirms: "The checkbox block sees data-lvt-force-update on toEl and force-resets." The description comment directly contradicts the tested behavior — fix it to match what actually happens.

Performance: querySelector on every equal node

In the isEqualNode early-exit path:

toEl.children.length === 0 ||
  toEl.querySelector("[data-lvt-force-update]") === null

This runs a full subtree query for every unchanged element during each morphdom pass. For pages with large static subtrees this could be measurable. Consider whether a data-lvt-force-update ancestor flag (e.g. on the nearest scroll container or form) could narrow the scope, or at least add a comment acknowledging the trade-off.

Clarify "one-shot" semantics in comments

The inline comment says "Auto-strip data-lvt-force-update so it acts as a one-shot directive." This is only true if the server stops sending the attribute on the next render. If the server always includes it, the checkbox is force-reset on every render, which is probably fine — but the phrasing implies the attribute self-disables after one use regardless of server behavior. Worth a one-line clarification.

Acknowledged, no action needed

The radio mutual-exclusion limitation during a single morphdom pass is noted in the comment — that's fine to leave as-is given the complexity of fixing it.

[claude]

Review

Overall the logic is sound and the test coverage is thorough. A few things worth flagging:

Radio group force-update (documented limitation — worth a workaround note)

The comment acknowledges that data-lvt-force-update on one radio can silently uncheck a sibling that morphdom already processed earlier in the same pass, because browser mutual exclusion fires synchronously mid-loop. The current docs say nothing about the workaround. Consider adding a note that to safely reset a radio group the server should send data-lvt-force-update on all radios in the group, not just the one being checked.

toEl attribute mutation in the preservation block

When fromEl.checked = true (user-checked), the code does:

toEl.checked = fromEl.checked;           // property
toEl.setAttribute("checked", "");        // attribute

Adding the checked attribute to toEl means fromEl.isEqualNode(toEl) returns false for an otherwise-equal node (the user just toggled the property, the attribute was never set on fromEl). The isEqualNode fast-path is then bypassed and morphdom runs a normal diff, which syncs fromEl.defaultChecked. Not a correctness bug, but it means the optimization silently degrades for checked checkboxes on every equal-content render.

Missing edge-case test

No test covers a focused checkbox/radio with data-lvt-force-update. The focus guard (return false) runs after the checkbox block, so the checked state IS force-reset in step 1 even for focused elements — but other attribute changes are skipped. Worth a test to confirm the expected behavior is intentional and locked in.

Minor: querySelector on toEl children

Already noted in comments — just flagging it's worth a follow-up ticket if large mostly-static subtrees become perf-visible in practice.

---

No blocking issues. Approving contingent on either adding the radio-group workaround note to docs/comments or confirming the current behavior is acceptable.

[claude]

Review

Good approach overall — the preservation logic is correct, the force-update escape hatch is well-designed, and the test coverage is thorough.

Potential bug: data-lvt-force-update can get stuck on a focused element

The attribute is stripped in onElUpdated, but when a focused element has data-lvt-force-update, the focus guard returns false before onElUpdated fires. The attribute then stays on fromEl indefinitely while the element remains focused.

Consequence: on every subsequent render while focused, fromEl.isEqualNode(toEl) is false (attribute mismatch) even if no content changed, so morphdom processes the element, hits the focus guard, returns false again — and the attribute never clears. This is a performance regression (redundant processing per render) rather than a correctness bug, since the checkbox block correctly reads toEl.hasAttribute('data-lvt-force-update') (which is false when the server stops sending it) and falls back to preservation mode.

The test data-lvt-force-update resets checked state even when checkbox is focused correctly verifies the reset, but doesn't assert that the attribute is stripped afterward — worth adding a cleanup check there.

Suggested fix: strip data-lvt-force-update from fromEl directly in the onBeforeElUpdated force-update branch (after copying checked/indeterminate), rather than deferring to onElUpdated. This is safe because the property has already been applied:

if (toEl.hasAttribute('data-lvt-force-update')) {
  fromEl.checked = toEl.checked;
  if (fromEl.type === 'checkbox') fromEl.indeterminate = toEl.indeterminate;
  fromEl.removeAttribute('data-lvt-force-update'); // strip eagerly
}

Minor: The toEl.querySelector('[data-lvt-force-update]') call in the isEqualNode path is correctly guarded but only runs when isEqualNode is true — since the attribute is stripped from fromEl after each render, this condition (ancestor subtrees equal, but descendant has force-update) can only occur on the first render of a newly inserted subtree. After that, fromEl lacks the attribute and isEqualNode will be false, so morphdom reaches the descendant via normal diffing. Worth a note confirming this is intentional dead-code-in-steady-state.

[claude]

Review: checkbox/radio state preservation

The implementation is solid and the test coverage is thorough.

Potential issue — querySelector in the isEqualNode fast-path

The comment correctly notes this is a defensive fallback and rare in steady state (the attr is stripped post-render, so isEqualNode diverges). However, if a server bug causes data-lvt-force-update to appear on many nodes without ever being consumed, this becomes an O(subtree) scan on every equal-node pair morphdom visits. Worth adding a comment that the steady-state assumption is a precondition for acceptable performance.

Radio group partial force-update caveat

The known limitation (send force-update on ALL radios in the group) is documented in the source comment but not surfaced anywhere user-facing. A user who only sends the attribute on the radio they want checked will silently get wrong behavior. Consider repeating this warning in public docs for the attribute.

Logic looks correct

• Force-update strip in onBeforeElUpdated (not just onElUpdated) correctly handles focused elements where return false skips the updated callback — confirmed by the focused-checkbox test.
• Propagating fromEl.checked back to toEl plus syncing the checked attribute to prevent morphdom adding a spurious attribute is the right approach for the preservation path.
• indeterminate handling is correct; the property cannot be set via HTML so server-side toEl.indeterminate is always false, which is the right reset value.

No bugs found.