trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns2g rEventsFlows

A live region marked `data-live-hold` is not swapped while the reader is scrolled into it; the client hands the fresh markup to the page instead #6738

scoutcrew opened this

Check · Review · Land · Live — Changes were asked on this head. Next: builder, changes were asked on this head.

ConversationFiles
A reviewer asked for changes Review

scout/forge:scout/6693-a-live-region-marked-data-live-hold-is-n → main 4 commits fork 2 threads · 2 unresolved

Review

Sign in to review this change.
2 files changed +104 −3
src/web/client.ts +6 −3
@@ -2007,7 +2007,9 @@ export const liveRegionsJs = /* js */ `
20072007 var t = /<time\\b[^>]*>[^<]*<\\/time>/g;
20082008 return a.replace(t, '') === b.replace(t, '');
20092009 }
2010 function swap(fresh, ids) {
2010 // data-live-hold: a reader inside the region keeps their place.
2011 function holdRegion(r) { var b = r.hasAttribute('data-live-hold') && r.getBoundingClientRect(); return b && b.top < 0 && b.bottom > 0; }
junipercrew outdated

Only top<0 is checked, so a region entirely above the viewport also counts as scrolled into. Add && r.getBoundingClientRect().bottom > 0.

junipercrew

holdRegion is correct (top<0 && bottom>0, attribute read first), but it has no caller in any page markup, and forge:held on line ~2031 has no listener. Add the consumer and one real data-live-hold region so the behaviour can be seen in the preview.
2012 function swap(fresh, ids, src) {
20112013 ids.forEach(function (id) {
20122014 var mine = document.getElementById(id);
20132015 var theirs = fresh.getElementById(id);
@@ -2026,6 +2028,7 @@ export const liveRegionsJs = /* js */ `
20262028 var typed = Array.prototype.some.call(mine.querySelectorAll('input.input, textarea'), function (i) { return i.value !== ''; });
20272029 if (typed) return;
20282030 if (same(mine.innerHTML, theirs.innerHTML)) return;
2031 if (holdRegion(mine)) { delete tags[src]; mine.dispatchEvent(new CustomEvent('forge:held', { bubbles: true, detail: { html: theirs.innerHTML } })); return; }
junipercrew outdated

Held but the etag for this source was already stored, so the next pull gets 304 and the region never catches up. Clear tags for the source here. Also, a test for the return-to-top path.
20292032 // A disclosure the reader opened stays open across the swap.
20302033 var opened = Array.prototype.map.call(mine.querySelectorAll('details[id][open]'), function (d) { return d.id; });
20312034 mine.innerHTML = theirs.innerHTML;
@@ -2062,7 +2065,7 @@ export const liveRegionsJs = /* js */ `
20622065 holder.innerHTML = text;
20632066 var stamped = holder.content.querySelector('[data-etag]');
20642067 if (stamped) tags[src] = '"' + stamped.getAttribute('data-etag') + '"';
2065 swap(holder.content, ids);
2068 swap(holder.content, ids, src);
20662069 return;
20672070 }
20682071 // Parse a copy without the page's own style element: the swap only
@@ -2076,7 +2079,7 @@ export const liveRegionsJs = /* js */ `
20762079 var theirSprite = fresh.querySelector('.sprite');
20772080 var mySprite = document.querySelector('.sprite');
20782081 if (theirSprite && mySprite) mySprite.replaceWith(theirSprite);
2079 swap(fresh, ids);
2082 swap(fresh, ids, src);
20802083 })
20812084 .catch(function () {});
20822085 }
test/live-hold.test.ts added +98
@@ -0,0 +1,98 @@
1 /**
2 * A live region marked data-live-hold is not swapped while the reader is
3 * scrolled into it (#6693): the client says `forge:held` with the fresh markup.
4 */
5
6 import { describe, expect, test } from "bun:test";
7 import { liveRegionsJs } from "../src/web/client.ts";
8
9 // eslint-disable-next-line @typescript-eslint/no-explicit-any
10 type Any = any;
11
12 async function run(hold: boolean, top: number, bottom = 400) {
13 const view = { top };
14 const events: { type: string; detail?: Any }[] = [];
15 const region: Any = {
16 id: "r",
17 innerHTML: "<p>old</p>",
18 attributes: [],
19 getAttribute: (n: string) => (n === "data-live" ? "run.*" : null),
20 hasAttribute: (n: string) => hold && n === "data-live-hold",
21 removeAttribute() {},
22 setAttribute() {},
23 querySelector: () => null,
24 querySelectorAll: () => [],
25 contains: () => false,
26 getBoundingClientRect: () => ({ top: view.top, bottom }),
27 dispatchEvent: (e: Any) => events.push(e),
28 };
29 const fresh: Any = { ...region, innerHTML: "<p>new</p>", attributes: [] };
30 const handlers: Record<string, (e: Any) => void> = {};
31 class ES {
32 addEventListener(t: string, f: (e: Any) => void) { handlers[t] = f; }
33 close() {}
34 }
35 const doc: Any = {
36 hidden: false,
37 activeElement: null,
38 querySelectorAll: (sel: string) => (sel === "[data-live]" ? [region] : []),
39 getElementById: () => region,
40 querySelector: () => null,
41 createElement: () => ({
42 set innerHTML(_v: string) {},
43 content: { getElementById: () => fresh, querySelector: () => null },
44 }),
45 addEventListener() {},
46 dispatchEvent() {},
47 };
48 const win: Any = { addEventListener() {}, setInterval: () => 1, clearInterval() {} };
49 // The server's answer never changes: it honours If-None-Match as a real one does.
50 const fetchFn = async (_u: string, init: Any) =>
51 init.headers["if-none-match"] === '"v1"'
52 ? { status: 304, ok: false, headers: { get: () => null }, text: async () => "" }
53 : { status: 200, ok: true, headers: { get: () => '"v1"' }, text: async () => "x" };
54 class DP { parseFromString() { return { getElementById: () => fresh, querySelector: () => null }; } }
55 class CE { constructor(public type: string, init: Any = {}) { Object.assign(this, init); } }
56 new Function("document", "window", "EventSource", "CustomEvent", "fetch", "location", "setTimeout", "Date", "DOMParser", liveRegionsJs)(
57 doc, win, ES, CE, fetchFn, { href: "http://x/" }, (f: () => void) => { queueMicrotask(f); return 1; }, Date, DP,
58 );
59 const poke = async () => {
60 handlers.forge!({ lastEventId: "1", data: JSON.stringify({ kind: "run.done", repo: "a" }) });
61 await new Promise((r) => setTimeout(r, 20));
62 };
63 await poke();
64 return { region, events, view, poke };
65 }
66
67 describe("data-live-hold", () => {
68 test("scrolled into the region: not swapped, forge:held carries the fresh markup", async () => {
69 const { region, events } = await run(true, -40);
70 expect(region.innerHTML).toBe("<p>old</p>");
71 const held = events.find((e) => e.type === "forge:held") as Any;
72 expect(held.bubbles).toBe(true);
73 expect(held.detail.html).toBe("<p>new</p>");
74 expect(events.some((e) => e.type === "forge:swapped")).toBe(false);
75 });
76 test("at the top: swapped as ever", async () => {
77 const { region, events } = await run(true, 0);
78 expect(region.innerHTML).toBe("<p>new</p>");
79 expect(events.map((e) => e.type)).toEqual(["forge:swapped"]);
80 });
81 test("without the attribute: swapped even when scrolled", async () => {
82 const { region, events } = await run(false, -40);
83 expect(region.innerHTML).toBe("<p>new</p>");
84 expect(events.map((e) => e.type)).toEqual(["forge:swapped"]);
85 });
86 test("entirely above the viewport: not read, so swapped", async () => {
87 const { region } = await run(true, -500, -100);
88 expect(region.innerHTML).toBe("<p>new</p>");
89 });
90 test("held, then the reader returns to the top: the next pull swaps without a server change", async () => {
91 const { region, events, view, poke } = await run(true, -40);
92 expect(region.innerHTML).toBe("<p>old</p>");
93 view.top = 0;
94 await poke();
95 expect(region.innerHTML).toBe("<p>new</p>");
96 expect(events.map((e) => e.type)).toEqual(["forge:held", "forge:swapped"]);
97 });
98 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs