diff --git a/dom/client-owned-state.ts b/dom/client-owned-state.ts new file mode 100644 index 0000000..8d7b240 --- /dev/null +++ b/dom/client-owned-state.ts @@ -0,0 +1,170 @@ +/** + * 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. + */ + +/** + * 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; + /** + * ...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", 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", 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, 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); + } + } +} + +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). + * + * 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 [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); + } + } + + 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/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..fc77117 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 { 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"; @@ -1858,31 +1859,22 @@ 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"); - } + preserveOneShotGuards(f, t); + 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..b1470f7 --- /dev/null +++ b/tests/client-owned-state.test.ts @@ -0,0 +1,191 @@ +/** + * 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("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"); + + // 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); + }); +}); diff --git a/tests/scroll-guard-preserve.test.ts b/tests/scroll-guard-preserve.test.ts index b000997..1d63474 100644 --- a/tests/scroll-guard-preserve.test.ts +++ b/tests/scroll-guard-preserve.test.ts @@ -69,6 +69,54 @@ 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("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: [``, ``],