From 4c4e4b56ddf7dfc95e2e576341d9881c87760101 Mon Sep 17 00:00:00 2001 From: Adnaan Badr Date: Tue, 14 Jul 2026 19:05:41 +0000 Subject: [PATCH 1/3] fix(lvt-el): preserve client-applied class/attr state across morphs (#147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A class applied by lvt-el:toggleClass/addClass was stripped by the next morph. The server never emits it, so the incoming toEl carried the server's original class and morphdom's attribute diff overwrote the live one. A dropdown held open with lvt-el:toggleClass:on:click="open" closed under the user on any unrelated re-render — in prereview's --agent mode, a background status render slammed the View menu shut with no user action at all. There was no escape hatch: lvt-ignore-attrs only copies attributes toEl LACKS, and any element using toggleClass has a server-emitted base class, so class was always skipped. lvt-ignore freezes the whole subtree and stops content updates. setAttr/toggleAttr had the identical bug — same directive, same executeAction switch, no protection — so all five state-mutating actions are fixed together through one record rather than three ad-hoc guards. dom/client-owned-state.ts records what the client applied, in a WeakMap (the morph-immune pattern already used by animatedElements/scrollResetPriors). The record is required, not just tidy: at morph time fromEl.classList is the UNION of the server's tokens and the client's, so copying it onto toEl would resurrect classes the server deliberately dropped. The record answers the one question morphdom cannot — which tokens does the client own? onBeforeElUpdated re-applies that state onto toEl, which morphdom then stamps onto the live element (the hook runs before morphAttrs, and class has no special handler). The shared options object covers both morph call sites. Semantics: the client overlay wins on CONFLICT (so removeClass of a class the server still emits also survives — click-away works), and the server reowns on AGREEMENT (the entry self-cleans, which is also what retires an entry when the client toggles back). Deliberately NOT gated on the directive still being present, unlike the one-shot iv-done/autofocused guards: this is persistent state, not a latch, and a data-lvt-target'd binding lands on an element that carries no lvt-el:* attribute at all. Out of scope by design: replaceWith/replaceChildren paths destroy element identity, so client state legitimately dies with the node. Verified in a real browser (chromedp + the built bundle): a real click opens the menu, a background render lands, and the menu stays open while the server's update still applies. Against the unfixed bundle the same run closes the menu. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01DBWL3SvGmFQTLH7ezW4xTz --- dom/client-owned-state.ts | 112 ++++++++++++++++++++ dom/reactive-attributes.ts | 16 ++- livetemplate-client.ts | 7 ++ tests/client-owned-state.test.ts | 171 +++++++++++++++++++++++++++++++ 4 files changed, 304 insertions(+), 2 deletions(-) create mode 100644 dom/client-owned-state.ts create mode 100644 tests/client-owned-state.test.ts diff --git a/dom/client-owned-state.ts b/dom/client-owned-state.ts new file mode 100644 index 0000000..0fa0a9e --- /dev/null +++ b/dom/client-owned-state.ts @@ -0,0 +1,112 @@ +/** + * Client-owned DOM state — a morph-surviving overlay for `lvt-el:` actions. + * + * `lvt-el:toggleClass` / `addClass` / `removeClass` / `setAttr` / `toggleAttr` exist to hold + * client-side UI state (a dropdown's `open`, a disclosure's `expanded`). The server never emits + * that state, so the incoming `toEl` carries the server's original `class`/attrs and morphdom's + * attribute diff overwrites the live element — the menu closes under the user on any unrelated + * re-render. + * + * Copying `fromEl`'s classes onto `toEl` cannot fix this: at morph time `fromEl.classList` is the + * UNION of the server's tokens and the client's, so copying it would also resurrect classes the + * server deliberately dropped this render. Hence a record — it answers the one question morphdom + * cannot: *which of these tokens does the client own?* + * + * A WeakMap (not a data-* attribute) is what makes the record itself morph-immune, following + * `animatedElements` / `scrollResetPriors` in dom/directives.ts. It also leaves no DOM residue and + * is collected with the element. + * + * Semantics — the client overlay wins on conflict, the server reowns on agreement: + * + * - Conflict (client wants a class the server omits, or vice versa) → re-apply onto `toEl`, so + * morphdom stamps it onto the live element. The overlay persists until the client reverses it + * or the element is discarded. + * - Agreement (client and server now want the same thing) → drop the entry; the overlay is + * redundant and the server reowns the value. + * + * The agreement rule is also what retires an entry when the client toggles back to the server's + * value — no separate bookkeeping needed. It does mean that once the server emits a value the + * client had been overlaying, the client's claim is retired for good: if the server later stops + * emitting it, the value goes away rather than reverting to an overlay. That is intended — once the + * server has spoken about a value it owns it — and it keeps records from leaking forever. + * + * Note this is deliberately NOT gated on the element still carrying its `lvt-el:*` directive, which + * is how the one-shot guards (`data-lvt-iv-done`, `data-lvt-autofocused`) work. Those are latches + * that need to re-arm; this is state that needs to persist. A directive gate would also break every + * `data-lvt-target` binding, whose resolved target carries no `lvt-el:*` attribute at all. + */ + +interface OwnedState { + /** class → true if the client added it, false if the client removed it. */ + classes: Map; + /** attribute name → the value the client set, or null if the client removed it. */ + attrs: Map; +} + +const ownedState = new WeakMap(); + +function stateFor(element: Element): OwnedState { + let state = ownedState.get(element); + if (!state) { + state = { classes: new Map(), attrs: new Map() }; + ownedState.set(element, state); + } + return state; +} + +/** Record that the client added (or removed) a class, so it survives the next morph. */ +export function recordClass(element: Element, className: string, added: boolean): void { + stateFor(element).classes.set(className, added); +} + +/** + * Record that the client set an attribute to `value`, or removed it when `value` is null. + * Boolean attributes set via toggleAttr are recorded with their DOM value, the empty string. + */ +export function recordAttr(element: Element, name: string, value: string | null): void { + stateFor(element).attrs.set(name, value); +} + +/** + * Re-apply an element's client-owned state onto the incoming `toEl`, from onBeforeElUpdated. + * + * Writing to `toEl` rather than `fromEl` is the whole trick: morphdom calls onBeforeElUpdated + * BEFORE morphAttrs(fromEl, toEl), and morphAttrs copies toEl's attributes ONTO the live element. + * So whatever we put on `toEl` here is what morphdom stamps onto the live DOM. + * + * Entries that the server has come to agree with are dropped as we go (see the file header). + */ +export function reapplyClientOwnedState(fromEl: Element, toEl: Element): void { + const state = ownedState.get(fromEl); + if (!state) return; + + for (const [className, added] of state.classes) { + if (toEl.classList.contains(className) === added) { + state.classes.delete(className); // server agrees — it reowns the class + continue; + } + if (added) { + toEl.classList.add(className); + } else { + toEl.classList.remove(className); + } + } + + for (const [name, value] of state.attrs) { + // getAttribute already returns null for an absent attribute, which is exactly how a + // client-side removal is recorded — so the two representations compare directly. + if (toEl.getAttribute(name) === value) { + state.attrs.delete(name); // server agrees — it reowns the attribute + continue; + } + if (value === null) { + toEl.removeAttribute(name); + } else { + toEl.setAttribute(name, value); + } + } + + if (state.classes.size === 0 && state.attrs.size === 0) { + ownedState.delete(fromEl); + } +} diff --git a/dom/reactive-attributes.ts b/dom/reactive-attributes.ts index 9b8a8cc..c033cec 100644 --- a/dom/reactive-attributes.ts +++ b/dom/reactive-attributes.ts @@ -24,8 +24,15 @@ * - toggleClass: Toggles CSS class(es) * - setAttr: Sets an attribute (name:value format) * - toggleAttr: Toggles a boolean attribute + * + * Every method except `reset` mutates state the server doesn't know about, so each records what it + * did in dom/client-owned-state.ts. Without that record the next morph strips it (the server's + * `class`/attrs win), and a dropdown held open with `lvt-el:toggleClass` closes under the user on + * any unrelated re-render. */ +import { recordAttr, recordClass } from "./client-owned-state"; + export type ReactiveAction = | "reset" | "addClass" @@ -148,6 +155,7 @@ export function executeAction( if (param) { const classes = param.split(/\s+/).filter(Boolean); element.classList.add(...classes); + classes.forEach((c) => recordClass(element, c, true)); } break; @@ -155,13 +163,14 @@ export function executeAction( if (param) { const classes = param.split(/\s+/).filter(Boolean); element.classList.remove(...classes); + classes.forEach((c) => recordClass(element, c, false)); } break; case "toggleClass": if (param) { const classes = param.split(/\s+/).filter(Boolean); - classes.forEach((c) => element.classList.toggle(c)); + classes.forEach((c) => recordClass(element, c, element.classList.toggle(c))); } break; @@ -172,13 +181,16 @@ export function executeAction( const name = param.substring(0, colonIndex); const value = param.substring(colonIndex + 1); element.setAttribute(name, value); + recordAttr(element, name, value); } } break; case "toggleAttr": if (param) { - element.toggleAttribute(param); + // toggleAttribute returns true if the attribute is now present. Present boolean + // attributes have the empty string as their DOM value. + recordAttr(element, param, element.toggleAttribute(param) ? "" : null); } break; } diff --git a/livetemplate-client.ts b/livetemplate-client.ts index 06979d9..7bac11f 100644 --- a/livetemplate-client.ts +++ b/livetemplate-client.ts @@ -44,6 +44,7 @@ import { ObserverManager } from "./dom/observer-manager"; import { LoadingIndicator } from "./dom/loading-indicator"; import { FormDisabler } from "./dom/form-disabler"; import { setupReactiveAttributeListeners } from "./dom/reactive-attributes"; +import { reapplyClientOwnedState } from "./dom/client-owned-state"; import { setupInvokerPolyfill } from "./dom/invoker-polyfill"; import { setupHashLink, teardownHashLink, openFromHash, safeMatchesPopoverOpen } from "./dom/hash-link"; import { setupScrollAway, teardownScrollAway } from "./dom/scroll-away"; @@ -1883,6 +1884,12 @@ export class LiveTemplateClient { if (f.getAttribute("data-lvt-autofocused") === "true" && t.hasAttribute("lvt-autofocus")) { t.setAttribute("data-lvt-autofocused", "true"); } + // Re-apply state the client owns (lvt-el:toggleClass/addClass/setAttr/...) onto toEl, + // so morphdom stamps it back onto the live element instead of overwriting it with the + // server's class/attrs. Unlike the guards above this is NOT gated on the directive + // still being present — it's persistent state, not a one-shot latch, and a + // data-lvt-target'd binding lands on an element that has no lvt-el:* attribute. + reapplyClientOwnedState(f, t); } // Track newly-introduced directive attributes so the post-render diff --git a/tests/client-owned-state.test.ts b/tests/client-owned-state.test.ts new file mode 100644 index 0000000..856ba90 --- /dev/null +++ b/tests/client-owned-state.test.ts @@ -0,0 +1,171 @@ +/** + * Client-owned `lvt-el:` state must survive morphdom (#147). + * + * `lvt-el:toggleClass` exists to hold client-side UI state, but the server never emits that class, + * so the incoming `toEl` carries the server's original `class` and morphdom's attribute diff used to + * overwrite the live one — a dropdown held open with `lvt-el:toggleClass:on:click="open"` closed + * under the user on any unrelated re-render. Same bug for `setAttr` / `toggleAttr`. + * + * These tests drive the REAL pipeline: `executeAction` (exactly what the click/click-away delegation + * calls, with the target already resolved) followed by `client.updateDOM`, which runs a real morph. + * The browser e2e only reproduces ~1 run in 4 — this is the deterministic gate. + * + * Semantics under test: the client overlay wins on CONFLICT, the server reowns on AGREEMENT. + */ + +import { LiveTemplateClient } from "../livetemplate-client"; +import { executeAction, resolveTarget } from "../dom/reactive-attributes"; +import { reapplyClientOwnedState } from "../dom/client-owned-state"; + +describe("client-owned lvt-el state survives morphdom", () => { + let client: LiveTemplateClient; + let wrapper: HTMLElement; + + beforeEach(() => { + client = new LiveTemplateClient(); + document.body.replaceChildren(); + wrapper = document.createElement("div"); + wrapper.setAttribute("data-lvt-id", "test-owned"); + document.body.appendChild(wrapper); + }); + + it("keeps a toggled class across an unrelated re-render (the #147 repro)", () => { + client.updateDOM(wrapper, { + s: [ + `
`, + ``, + ], + 0: "a", + }); + const dropdown = wrapper.querySelector(".tb-dropdown") as HTMLElement; + executeAction(dropdown, "toggleClass", "open"); // user clicks the trigger + expect(dropdown.classList.contains("open")).toBe(true); + + // Any unrelated render — in prereview, a sibling action whose data feeds the panel. + client.updateDOM(wrapper, { 0: "b" }); + + const after = wrapper.querySelector(".tb-dropdown") as HTMLElement; + expect(after).toBe(dropdown); // morphdom reused the node + expect(after.classList.contains("open")).toBe(true); // the menu stays open — the fix + expect(after.classList.contains("tb-dropdown")).toBe(true); // server's base class intact + expect(wrapper.querySelector(".sib")!.textContent).toBe("b"); // and the real update landed + }); + + it("keeps a class the client REMOVED from a class the server still emits", () => { + // click-away on a panel the server rendered open. An add-only record would not fix this: + // morphdom would re-add `open` from toEl on the very next render. + client.updateDOM(wrapper, { + s: [`
`, ``], + 0: "a", + }); + const panel = wrapper.querySelector(".panel") as HTMLElement; + expect(panel.classList.contains("open")).toBe(true); // server opened it + + executeAction(panel, "removeClass", "open"); // user clicks away + client.updateDOM(wrapper, { 0: "b" }); + + const after = wrapper.querySelector(".panel") as HTMLElement; + expect(after).toBe(panel); + expect(after.classList.contains("open")).toBe(false); // stays closed + expect(wrapper.querySelector(".sib")!.textContent).toBe("b"); + }); + + it("does not resurrect a class after the client toggles back to the server's state", () => { + client.updateDOM(wrapper, { + s: [`
`, ``], + 0: "a", + }); + const dropdown = wrapper.querySelector(".tb-dropdown") as HTMLElement; + + executeAction(dropdown, "toggleClass", "open"); // open + executeAction(dropdown, "toggleClass", "open"); // close again + expect(dropdown.classList.contains("open")).toBe(false); + + client.updateDOM(wrapper, { 0: "b" }); + + expect((wrapper.querySelector(".tb-dropdown") as HTMLElement).classList.contains("open")).toBe(false); + }); + + it("keeps setAttr and toggleAttr state across a morph, including a toggle-OFF", () => { + client.updateDOM(wrapper, { + s: [``, ``], + 0: "a", + }); + const row = wrapper.querySelector(".row") as HTMLElement; + + executeAction(row, "setAttr", "aria-expanded:true"); // conflicts with the server's "false" + executeAction(row, "toggleAttr", "hidden"); // toggles OFF an attr the server emits + + client.updateDOM(wrapper, { 0: "b" }); + + const after = wrapper.querySelector(".row") as HTMLElement; + expect(after).toBe(row); + expect(after.getAttribute("aria-expanded")).toBe("true"); // client value survives + expect(after.hasAttribute("hidden")).toBe(false); // toggle-off survives + expect(wrapper.querySelector(".sib")!.textContent).toBe("b"); + }); + + it("survives on the element data-lvt-target resolves to, which carries no lvt-el attribute", () => { + client.updateDOM(wrapper, { + s: [ + `
`, + ``, + ], + 0: "a", + }); + const button = wrapper.querySelector("button") as HTMLElement; + const menu = wrapper.querySelector(".menu") as HTMLElement; + + // Both call sites in reactive-attributes.ts pass the RESOLVED target, so the record keys on it. + const target = resolveTarget(button); + expect(target).toBe(menu); + expect(menu.hasAttribute("lvt-el:addClass:on:click")).toBe(false); // no directive on the target + executeAction(target, "addClass", "open"); + + client.updateDOM(wrapper, { 0: "b" }); + + expect((wrapper.querySelector(".menu") as HTMLElement).classList.contains("open")).toBe(true); + expect(wrapper.querySelector(".sib")!.textContent).toBe("b"); + }); + + it("hands the class back to the server once the server starts emitting it", () => { + client.updateDOM(wrapper, { + s: [`
`, ``], + 0: "a", + }); + const dropdown = wrapper.querySelector(".tb-dropdown") as HTMLElement; + executeAction(dropdown, "toggleClass", "open"); + + // Server now emits `open` too — client and server agree, so the overlay retires. + client.updateDOM(wrapper, { + s: [`
`, ``], + 0: "a", + }); + expect((wrapper.querySelector(".tb-dropdown") as HTMLElement).classList.contains("open")).toBe(true); + + // Server drops it again. The class goes away — it did NOT linger as a client overlay. + client.updateDOM(wrapper, { + s: [`
`, ``], + 0: "a", + }); + expect((wrapper.querySelector(".tb-dropdown") as HTMLElement).classList.contains("open")).toBe(false); + }); + + it("does not leak one element's state onto another", () => { + const recorded = document.createElement("div"); + executeAction(recorded, "addClass", "open"); + + // A different element that never had an action run on it must come back untouched. + const fresh = document.createElement("div"); + const incoming = document.createElement("div"); + incoming.className = "base"; + reapplyClientOwnedState(fresh, incoming); + expect(incoming.className).toBe("base"); + + // The recorded element's state, by contrast, lands on its own incoming node. + const ownIncoming = document.createElement("div"); + ownIncoming.className = "base"; + reapplyClientOwnedState(recorded, ownIncoming); + expect(ownIncoming.classList.contains("open")).toBe(true); + }); +}); From 84440274400fcff7bb6c03e805430ff623baabd5 Mon Sep 17 00:00:00 2001 From: Adnaan Badr Date: Tue, 14 Jul 2026 19:06:42 +0000 Subject: [PATCH 2/3] fix(lvt-fx): preserve the bottom-sticky guard across morphs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit data-lvt-scroll-sticky is a runtime-only guard the server never emits, and the preservation block in onBeforeElUpdated copied only its two siblings (data-lvt-iv-done, data-lvt-autofocused). So morphdom stripped it on every render: `initialized` was false every time, and lvt-fx:scroll="bottom-sticky" re-ran its first-encounter branch — an unconditional scrollTo(bottom, behavior:"instant") — yanking back any user who had scrolled up to read history and making the near-bottom threshold check dead code. Copy it onto toEl exactly like the other two, gated on lvt-fx:scroll still being present. Unlike the lvt-el overlay in the previous commit, the directive gate IS correct here: this is a one-shot latch that must re-arm when the directive is re-added. Found while fixing #147 — same family of bug (client-created state with no protection from the morph), different directive. The existing bottom-sticky tests call handleScrollDirectives in isolation, so they never saw a morph, which is how this survived. The new test drives a real morph via updateDOM. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01DBWL3SvGmFQTLH7ezW4xTz --- dom/client-owned-state.ts | 41 +++++++++++++++++++++++++++++ livetemplate-client.ts | 35 +++++++----------------- tests/scroll-guard-preserve.test.ts | 30 +++++++++++++++++++++ 3 files changed, 81 insertions(+), 25 deletions(-) diff --git a/dom/client-owned-state.ts b/dom/client-owned-state.ts index 0fa0a9e..f923227 100644 --- a/dom/client-owned-state.ts +++ b/dom/client-owned-state.ts @@ -36,6 +36,47 @@ * `data-lvt-target` binding, whose resolved target carries no `lvt-el:*` attribute at all. */ +/** + * One-shot latch guards: runtime-only attributes a directive stamps on the live DOM to record that + * it has already fired once. The server never emits them, so morphdom strips them on every render + * unless we copy them onto `toEl` — and the directive then re-fires on every unrelated re-render. + * + * Unlike the overlay below, these ARE gated on their directive still being present: they are + * latches, so when the server drops the directive the guard must fall away and the directive + * re-arm on re-add (deliberately re-navigating to the same element should scroll again). + * + * This is a table rather than an if-chain because the chain was how `data-lvt-scroll-sticky` came + * to ship broken: it enumerated two of the three guards, and the third was simply forgotten. A new + * one-shot directive registers a row here instead of hand-writing another branch. + * + * (`lvt-fx:animate` needs no row — it latches via a WeakSet, which morphdom cannot touch.) + */ +const ONE_SHOT_GUARDS: ReadonlyArray<{ + /** the runtime-only attribute the directive stamps */ + guard: string; + /** the value it stamps, and the value we restore */ + value: string; + /** preserve the guard only while this directive is still on the element */ + directive: string; +}> = [ + // lvt-fx:scroll="into-view" — else a background poll re-scrolls the viewport to a stale target. + { guard: "data-lvt-iv-done", value: "1", directive: "lvt-fx:scroll" }, + // lvt-autofocus — else any render re-steals focus. + { guard: "data-lvt-autofocused", value: "true", directive: "lvt-autofocus" }, + // lvt-fx:scroll="bottom-sticky" — else every render force-jumps a scrolled-up reader to the + // bottom (behavior:"instant"), making the near-bottom threshold check dead code. + { guard: "data-lvt-scroll-sticky", value: "1", directive: "lvt-fx:scroll" }, +]; + +/** Copy any still-armed one-shot guards onto `toEl` so morphdom keeps them. */ +export function preserveOneShotGuards(fromEl: Element, toEl: Element): void { + for (const { guard, value, directive } of ONE_SHOT_GUARDS) { + if (fromEl.getAttribute(guard) === value && toEl.hasAttribute(directive)) { + toEl.setAttribute(guard, value); + } + } +} + interface OwnedState { /** class → true if the client added it, false if the client removed it. */ classes: Map; diff --git a/livetemplate-client.ts b/livetemplate-client.ts index 7bac11f..fc77117 100644 --- a/livetemplate-client.ts +++ b/livetemplate-client.ts @@ -44,7 +44,7 @@ import { ObserverManager } from "./dom/observer-manager"; import { LoadingIndicator } from "./dom/loading-indicator"; import { FormDisabler } from "./dom/form-disabler"; import { setupReactiveAttributeListeners } from "./dom/reactive-attributes"; -import { reapplyClientOwnedState } from "./dom/client-owned-state"; +import { preserveOneShotGuards, reapplyClientOwnedState } from "./dom/client-owned-state"; import { setupInvokerPolyfill } from "./dom/invoker-polyfill"; import { setupHashLink, teardownHashLink, openFromHash, safeMatchesPopoverOpen } from "./dom/hash-link"; import { setupScrollAway, teardownScrollAway } from "./dom/scroll-away"; @@ -1859,36 +1859,21 @@ export class LiveTemplateClient { return false; } - // Preserve the one-shot scroll/autofocus guards across morphdom. The - // guards — data-lvt-iv-done (for lvt-fx:scroll="into-view") and - // data-lvt-autofocused (for lvt-autofocus) — are runtime-only attributes - // the server never emits, so morphdom would strip them on every render - // (toEl lacks them), making BOTH directives re-fire on every unrelated - // re-render: re-scrolling the page / re-stealing focus to a stale target - // (e.g. a 750ms status poll yanking the viewport back to the last-jumped - // comment). Copy the guard onto toEl WHILE its directive is still present - // so morphdom keeps it; if the server dropped the directive the guard - // falls away naturally and the directive re-arms when re-applied (so - // deliberately re-navigating to the same element still scrolls). This is - // why lvt-fx:animate is immune — it guards via a WeakSet that survives - // morphdom; these two guard via attributes, which do not. + // Carry client-created DOM state across the morph. The server never emits any of it, so + // toEl lacks it and morphdom's attribute diff would strip it on every render. Both helpers + // write onto toEl — the hook runs before morphAttrs, which stamps toEl onto the live + // element — and both are registered in dom/client-owned-state.ts: + // - one-shot latch guards (lvt-fx:scroll, lvt-autofocus), gated on their directive so + // they re-arm if the server drops it; + // - the lvt-el: class/attr overlay (toggleClass, setAttr, ...), which is persistent + // state and deliberately NOT directive-gated. if ( fromEl.nodeType === Node.ELEMENT_NODE && toEl.nodeType === Node.ELEMENT_NODE ) { const f = fromEl as Element; const t = toEl as Element; - if (f.getAttribute("data-lvt-iv-done") === "1" && t.hasAttribute("lvt-fx:scroll")) { - t.setAttribute("data-lvt-iv-done", "1"); - } - if (f.getAttribute("data-lvt-autofocused") === "true" && t.hasAttribute("lvt-autofocus")) { - t.setAttribute("data-lvt-autofocused", "true"); - } - // Re-apply state the client owns (lvt-el:toggleClass/addClass/setAttr/...) onto toEl, - // so morphdom stamps it back onto the live element instead of overwriting it with the - // server's class/attrs. Unlike the guards above this is NOT gated on the directive - // still being present — it's persistent state, not a one-shot latch, and a - // data-lvt-target'd binding lands on an element that has no lvt-el:* attribute. + preserveOneShotGuards(f, t); reapplyClientOwnedState(f, t); } diff --git a/tests/scroll-guard-preserve.test.ts b/tests/scroll-guard-preserve.test.ts index b000997..575ee95 100644 --- a/tests/scroll-guard-preserve.test.ts +++ b/tests/scroll-guard-preserve.test.ts @@ -69,6 +69,36 @@ describe("one-shot scroll/autofocus guards survive morphdom", () => { expect(scrollSpy).toHaveBeenCalledTimes(2); }); + it("bottom-sticky does NOT force-jump a scrolled-up user on a later render (guard preserved)", () => { + // data-lvt-scroll-sticky is the same kind of one-shot guard. Unpreserved, `initialized` was + // false on every render, so bottom-sticky re-ran its first-encounter branch — an unconditional + // scrollTo(bottom, behavior:"instant") — yanking back any user who had scrolled up to read + // history, and making the near-bottom threshold check dead code. + const scrollToSpy = jest.fn(); + (Element.prototype as any).scrollTo = scrollToSpy; + // jsdom has no layout: a viewport far from the bottom (500 - 0 - 100 = 400 > 100 threshold). + Object.defineProperty(Element.prototype, "scrollHeight", { value: 500, configurable: true }); + Object.defineProperty(Element.prototype, "clientHeight", { value: 100, configurable: true }); + Object.defineProperty(Element.prototype, "scrollTop", { value: 0, configurable: true }); + + client.updateDOM(wrapper, { + s: [`
`, `
`], + 0: "a", + }); + const log = wrapper.querySelector(".log") as HTMLElement; + expect(scrollToSpy).toHaveBeenCalledTimes(1); // first paint pins to the bottom + expect(scrollToSpy).toHaveBeenLastCalledWith({ top: 500, behavior: "instant" }); + expect(log.dataset.lvtScrollSticky).toBe("1"); + + // The user has scrolled up. A later render must leave them where they are. + client.updateDOM(wrapper, { 0: "b" }); + + expect(wrapper.querySelector(".log")).toBe(log); // node reused + expect(log.dataset.lvtScrollSticky).toBe("1"); // guard PRESERVED across the morph + expect(scrollToSpy).toHaveBeenCalledTimes(1); // NOT force-jumped — the fix + expect(wrapper.querySelector(".sib")!.textContent).toBe("b"); // real update still landed + }); + it("preserves data-lvt-autofocused while lvt-autofocus stays present", () => { client.updateDOM(wrapper, { s: [``, ``], From 8fbfd564167668743638a684fbdca39dcef07a0f Mon Sep 17 00:00:00 2001 From: Adnaan Badr Date: Tue, 14 Jul 2026 20:52:05 +0000 Subject: [PATCH 3/3] fix: address Claude review comments on #148 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real edge cases the bot caught, both now pinned by tests that fail without the fix: 1. One-shot guards matched on directive PRESENCE, not the mode its value selects. data-lvt-iv-done and data-lvt-scroll-sticky both gate on lvt-fx:scroll, but the mode lives in that attribute's VALUE (directives.ts: `const mode = config`). So if a node switched into-view -> bottom-sticky, the stale into-view latch survived, and when the node came back to into-view it never scrolled again. Guard rows now carry an optional directiveValue and match on it. The bot is right that the presence-match predates this branch for iv-done — but the guard table doubled the surface, so it gets fixed here. 2. lvt-el:setAttr with name `class` stomped the class overlay: setAttribute writes the class list wholesale, and the attrs loop ran AFTER the classes loop, so it clobbered tokens the overlay had just restored. Attributes are now applied BEFORE classes, so a wholesale write lands first and per-token classes layer on top. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01DBWL3SvGmFQTLH7ezW4xTz --- dom/client-owned-state.ts | 51 +++++++++++++++++++---------- tests/client-owned-state.test.ts | 20 +++++++++++ tests/scroll-guard-preserve.test.ts | 18 ++++++++++ 3 files changed, 72 insertions(+), 17 deletions(-) diff --git a/dom/client-owned-state.ts b/dom/client-owned-state.ts index f923227..8d7b240 100644 --- a/dom/client-owned-state.ts +++ b/dom/client-owned-state.ts @@ -58,20 +58,32 @@ const ONE_SHOT_GUARDS: ReadonlyArray<{ value: string; /** preserve the guard only while this directive is still on the element */ directive: string; + /** + * ...and, when the directive's VALUE selects a mode, only while that mode is still selected. + * Two lvt-fx:scroll modes latch separately (into-view and bottom-sticky), so presence of + * `lvt-fx:scroll` alone is not enough: if a node switched modes, matching on presence would + * carry the old mode's guard over and stop the directive re-arming when the mode came back. + */ + directiveValue?: string; }> = [ // lvt-fx:scroll="into-view" — else a background poll re-scrolls the viewport to a stale target. - { guard: "data-lvt-iv-done", value: "1", directive: "lvt-fx:scroll" }, - // lvt-autofocus — else any render re-steals focus. + { guard: "data-lvt-iv-done", value: "1", directive: "lvt-fx:scroll", directiveValue: "into-view" }, + // lvt-autofocus — else any render re-steals focus. Valueless: presence is the whole directive. { guard: "data-lvt-autofocused", value: "true", directive: "lvt-autofocus" }, // lvt-fx:scroll="bottom-sticky" — else every render force-jumps a scrolled-up reader to the // bottom (behavior:"instant"), making the near-bottom threshold check dead code. - { guard: "data-lvt-scroll-sticky", value: "1", directive: "lvt-fx:scroll" }, + { guard: "data-lvt-scroll-sticky", value: "1", directive: "lvt-fx:scroll", directiveValue: "bottom-sticky" }, ]; /** Copy any still-armed one-shot guards onto `toEl` so morphdom keeps them. */ export function preserveOneShotGuards(fromEl: Element, toEl: Element): void { - for (const { guard, value, directive } of ONE_SHOT_GUARDS) { - if (fromEl.getAttribute(guard) === value && toEl.hasAttribute(directive)) { + for (const { guard, value, directive, directiveValue } of ONE_SHOT_GUARDS) { + if (fromEl.getAttribute(guard) !== value) continue; + const stillArmed = + directiveValue === undefined + ? toEl.hasAttribute(directive) + : toEl.getAttribute(directive) === directiveValue; + if (stillArmed) { toEl.setAttribute(guard, value); } } @@ -116,23 +128,16 @@ export function recordAttr(element: Element, name: string, value: string | null) * So whatever we put on `toEl` here is what morphdom stamps onto the live DOM. * * Entries that the server has come to agree with are dropped as we go (see the file header). + * + * Attributes are applied BEFORE classes on purpose: a whole-attribute write (`lvt-el:setAttr` + * with name `class`) replaces the class list wholesale, so it has to land first and let the + * per-token class overlay layer on top. The other order would let setAttr stomp the classes the + * overlay had just restored. */ export function reapplyClientOwnedState(fromEl: Element, toEl: Element): void { const state = ownedState.get(fromEl); if (!state) return; - for (const [className, added] of state.classes) { - if (toEl.classList.contains(className) === added) { - state.classes.delete(className); // server agrees — it reowns the class - continue; - } - if (added) { - toEl.classList.add(className); - } else { - toEl.classList.remove(className); - } - } - for (const [name, value] of state.attrs) { // getAttribute already returns null for an absent attribute, which is exactly how a // client-side removal is recorded — so the two representations compare directly. @@ -147,6 +152,18 @@ export function reapplyClientOwnedState(fromEl: Element, toEl: Element): void { } } + for (const [className, added] of state.classes) { + if (toEl.classList.contains(className) === added) { + state.classes.delete(className); // server agrees — it reowns the class + continue; + } + if (added) { + toEl.classList.add(className); + } else { + toEl.classList.remove(className); + } + } + if (state.classes.size === 0 && state.attrs.size === 0) { ownedState.delete(fromEl); } diff --git a/tests/client-owned-state.test.ts b/tests/client-owned-state.test.ts index 856ba90..b1470f7 100644 --- a/tests/client-owned-state.test.ts +++ b/tests/client-owned-state.test.ts @@ -151,6 +151,26 @@ describe("client-owned lvt-el state survives morphdom", () => { expect((wrapper.querySelector(".tb-dropdown") as HTMLElement).classList.contains("open")).toBe(false); }); + it("lets setAttr:class and toggleClass coexist without stomping each other", () => { + // setAttr writes the whole class attribute; toggleClass writes one token. The overlay applies + // attrs before classes so the wholesale write lands first and the token layers on top. + client.updateDOM(wrapper, { + s: [`
`, ``], + 0: "a", + }); + const row = wrapper.querySelector(".row") as HTMLElement; + + executeAction(row, "setAttr", "class:row selected"); + executeAction(row, "toggleClass", "open"); + + client.updateDOM(wrapper, { 0: "b" }); + + const after = wrapper.querySelector("div") as HTMLElement; + expect(after.classList.contains("selected")).toBe(true); // setAttr's value survives + expect(after.classList.contains("open")).toBe(true); // and the toggled class wasn't stomped + expect(wrapper.querySelector(".sib")!.textContent).toBe("b"); + }); + it("does not leak one element's state onto another", () => { const recorded = document.createElement("div"); executeAction(recorded, "addClass", "open"); diff --git a/tests/scroll-guard-preserve.test.ts b/tests/scroll-guard-preserve.test.ts index 575ee95..1d63474 100644 --- a/tests/scroll-guard-preserve.test.ts +++ b/tests/scroll-guard-preserve.test.ts @@ -99,6 +99,24 @@ describe("one-shot scroll/autofocus guards survive morphdom", () => { expect(wrapper.querySelector(".sib")!.textContent).toBe("b"); // real update still landed }); + it("re-arms into-view when the SAME node switches scroll mode and comes back", () => { + // into-view and bottom-sticky latch separately but share the lvt-fx:scroll attribute — the + // MODE lives in its value (directives.ts: `const mode = config`). Preserving a guard on mere + // presence of lvt-fx:scroll would carry data-lvt-iv-done across a switch to bottom-sticky, so + // when the node returned to into-view it would never scroll again. + client.updateDOM(wrapper, { s: [`
`, `
`], 0: "x" }); + expect(scrollSpy).toHaveBeenCalledTimes(1); + expect((wrapper.querySelector(".target") as HTMLElement).dataset.lvtIvDone).toBe("1"); + + // Same node, different mode → the into-view latch must NOT survive. + client.updateDOM(wrapper, { s: [`
`, `
`], 0: "x" }); + expect((wrapper.querySelector(".target") as HTMLElement).dataset.lvtIvDone).toBeUndefined(); + + // Back to into-view → it must scroll again. + client.updateDOM(wrapper, { s: [`
`, `
`], 0: "x" }); + expect(scrollSpy).toHaveBeenCalledTimes(2); + }); + it("preserves data-lvt-autofocused while lvt-autofocus stays present", () => { client.updateDOM(wrapper, { s: [``, ``],