diff --git a/app/composer/tests/overlays.test.js b/app/composer/tests/overlays.test.js index 851dd378..c6366728 100644 --- a/app/composer/tests/overlays.test.js +++ b/app/composer/tests/overlays.test.js @@ -6,7 +6,7 @@ import assert from "node:assert/strict"; import { setupDom, teardownDom, key, fire } from "./_dom.js"; import { EDGES, sectorAt, sectorInArc } from "../elements/wheel.js"; -let composer; +let composer, liveCount, LADDER; let entries; let DEMOS; let ROOT_ID; @@ -16,7 +16,7 @@ test.before(async () => { ({ composer, entries } = await import("../composer.js")); ({ DEMOS } = await import("../../catalogue/demos.js")); ({ ROOT_ID } = await import("../internals/portal.js")); - ({ baseOf } = await import("../internals/zlayer.js")); + ({ baseOf, liveCount, LADDER } = await import("../internals/zlayer.js")); }); test.after(() => teardownDom()); @@ -498,8 +498,21 @@ test("every overlay keeps its z-index bound, not just windows", () => { } // Closing the oldest compacts every rung it was in; nothing may go stale. - const before = opened.map(({ handle }) => handle.node.style.zIndex); - if (opened.length > 1) opened.shift().handle.close(); + // + // `before` used to be computed and then discarded with `void before`, so this + // checked only that survivors do not collide -- which is trivially true when + // nothing moves at all. It would have passed with zlayer's notify() removed, + // and notify() is the whole mechanism this names. + const before = new Map(opened.map(({ name, handle }) => [name, handle.node.style.zIndex])); + const closed = opened.length > 1 ? opened.shift() : null; + + // Some demos close themselves as they are built (a wheel that resolves on + // creation), so calling close() on one is a no-op and nothing SHOULD move. + // Counting live claims says whether a slot was actually freed; asserting + // compaction after a no-op close failed on correct code. + const rungOf = (z) => LADDER[Math.floor(Number(z) / 100) - 1]; + const closedRung = closed ? rungOf(before.get(closed.name)) : null; + const liveBefore = closedRung ? liveCount(closedRung) : 0; const seen = new Map(); for (const { name, handle } of opened) { @@ -507,6 +520,49 @@ test("every overlay keeps its z-index bound, not just windows", () => { assert.ok(!seen.has(z), `${name} collides with ${seen.get(z)} at z ${z}`); seen.set(z, name); } - void before; + + const freed = Boolean(closed && closedRung && liveCount(closedRung) < liveBefore); + if (freed) { + // What "nothing goes stale" means, stated exactly: everything that sat + // ABOVE the closed overlay in its rung moved down by one, and everything + // below did not move at all. + // + // Not "the rung is contiguous from its base" -- earlier tests in this file + // leave live claims, so this test does not own the whole rung and a gap at + // the base is somebody else's slot, not a fault. That version failed on + // correct code. + const closedZ = Number(before.get(closed.name)); + const rung = (z) => Math.floor(z / 100); + + for (const { name, handle } of opened) { + const was = Number(before.get(name)); + const now = Number(handle.node.style.zIndex); + if (rung(was) !== rung(closedZ)) { + assert.equal(now, was, `${name} moved although it is in another rung`); + } else if (was > closedZ) { + assert.equal(now, was - 1, + `${name} was above ${closed.name} and did not compact down`); + } else { + assert.equal(now, was, `${name} was below ${closed.name} and moved anyway`); + } + } + } for (const { handle } of opened) handle.destroy(); + + // The sweep above only checks compaction when a demo happened to leave a live + // claim, which is not something to rely on. Two overlays this test opens + // itself, so the invariant is exercised every run whatever the demos do. + const first = composer.popover.anchored({ + anchor: anchor.node, body: composer.text.md({ text: "a" }) }); + const second = composer.popover.anchored({ + anchor: anchor.node, body: composer.text.md({ text: "b" }) }); + const firstZ = Number(first.node.style.zIndex); + const secondZ = Number(second.node.style.zIndex); + assert.equal(secondZ, firstZ + 1, "two peers did not stack"); + + first.close(); + assert.equal(Number(second.node.style.zIndex), firstZ, + "the survivor did not compact into the freed slot"); + second.close(); + anchor.destroy(); }); diff --git a/app/composer/tests/rule_unknown_element_hides.test.js b/app/composer/tests/rule_unknown_element_hides.test.js index f56b1710..54ec28fc 100644 --- a/app/composer/tests/rule_unknown_element_hides.test.js +++ b/app/composer/tests/rule_unknown_element_hides.test.js @@ -112,10 +112,32 @@ test("a module is handed no way to touch the document", async () => { { load: async () => ({ setup: (ctx) => { context = ctx; } }) }, ); - assert.deepEqual(Object.keys(context).sort(), ["composer", "on", "shell", "values"]); - for (const value of Object.values(context)) { - assert.equal(value instanceof globalThis.Node, false, "no DOM node is reachable from a module"); + assert.deepEqual(Object.keys(context).sort(), ["on", "shell", "values"]); + + // Not "no value IS a node" -- that was the old assertion, and it passed while + // `composer` sat in this object handing out handles whose .node is a real + // element, reachable to the whole document through node.ownerDocument. The + // property that matters is that nothing here PRODUCES one either. + for (const [name, value] of Object.entries(context)) { + assert.equal(value instanceof globalThis.Node, false, `${name} is a DOM node`); } + assert.equal("composer" in context, false, + "an element factory is a way to obtain a DOM node"); + + // `on` is callable, and must hand back an unsubscribe rather than anything + // that leads to the page. + const off = context.on(() => {}); + assert.equal(typeof off, "function"); + assert.equal(off instanceof globalThis.Node, false); + + // Nothing one level down is a node either -- the services are the other place + // a handle could have been tucked away. + for (const [name, value] of Object.entries(context.shell)) { + for (const [inner, member] of Object.entries(value)) { + assert.equal(member instanceof globalThis.Node, false, `shell.${name}.${inner}`); + } + } + // The service surface is named one by one, so it can be reviewed. assert.deepEqual(Object.keys(context.shell).sort(), ["density", "theme"]); }); diff --git a/app/shell/panels.js b/app/shell/panels.js index 59ff651d..595fba75 100644 --- a/app/shell/panels.js +++ b/app/shell/panels.js @@ -1,8 +1,22 @@ // Turning announcements into panels. // // Everything a module can reach is decided here, and it is deliberately little: -// { composer, values, on }. No DOM node, no document, no kit internals. A module -// cannot style anything because it is never handed anything that could. +// { values, on, shell }. No DOM node, no document, no element factory. +// +// `composer` used to be in that list, and it was the hole in the guarantee: a +// factory returns a handle whose `.node` is a real element, so a module could +// take one, style it, and reach the whole document through `node.ownerDocument` +// -- no lint to evade, just the intended API. No module ever used it. Panels are +// rendered from the DECLARATION, and a module that wants to draw something is a +// module that should be declaring it instead. +// +// What is true, stated exactly: the kit hands a module nothing that touches the +// DOM. What is NOT true, and must not be claimed: that a module CANNOT reach the +// DOM. A ui.js is an ES module in the page realm and `document` is a global. +// tools/kit_lint raises the cost of doing that accidentally; it is a discipline +// check and not a boundary, and only real isolation (a worker, another realm) +// would make it one. First-party modules in this repo are reviewed; a +// third-party module would need that isolation before it could be trusted. import { composer } from "../composer/composer.js"; import { renderPanel, defaultsOf } from "../composer/panel.js"; @@ -56,7 +70,6 @@ export async function mountAll(manifest, { load = (path) => import(path) } = {}) // Kept, not discarded: setup() returns the unsubscribe from on(), and // dropping it means every re-mount leaves another live listener behind. teardown = ui.setup({ - composer, values: { get: () => values.valuesOf(spec.id), set: (key, value) => values.set(spec.id, key, value), diff --git a/tools/kit_lint/rules.js b/tools/kit_lint/rules.js index 1e1981f7..718ebbdb 100644 --- a/tools/kit_lint/rules.js +++ b/tools/kit_lint/rules.js @@ -1,21 +1,32 @@ // What a module may not do, and what the kit may not do. // -// These are the static half of rules 1 and 4. The runtime half is stronger -- -// a module is never handed anything that could style or parse -- but a scanner -// catches the attempt in the file rather than at the moment it would have run, -// which is the difference between a failing test and a support ticket. +// A DISCIPLINE CHECK, NOT A BOUNDARY. Say it plainly, because the difference +// matters: a module's ui.js is an ES module in the page realm, so `document` is +// a global it can reach and no regex can stop it. `globalThis["docu"+"ment"]` +// defeats every rule below and always will; only real isolation -- a worker, +// another realm -- would be a boundary. +// +// What this does buy: the accidental and the casual are caught in the file +// rather than at the moment they would have run, which is the difference +// between a failing test and a support ticket. The bracket-access spellings are +// here because they were the first thing tried and they went straight through. +// +// The runtime half is the real work: the kit hands a module nothing that +// touches the DOM, so following the contract requires no restraint. export const MODULE_RULES = [ { name: "inline-style", why: "presentation belongs to the kit", - test: /\.style\s*[.[]|setAttribute\s*\(\s*['"`]style/ }, + test: /\.style\s*[.[]|\[\s*['"`]style['"`]\s*\]|setAttribute\s*\(\s*['"`]style/ }, { name: "style-attribute", why: "presentation belongs to the kit", test: /\bstyle\s*=\s*['"`]/ }, { name: "class-manipulation", why: "kit classes are the kit's; a module names elements, not classes", test: /\bclassList\b|setAttribute\s*\(\s*['"`]class/ }, - { name: "raw-element", why: "every element comes from composer..()", - test: /document\s*\.\s*createElement|createElementNS/ }, + { name: "raw-element", why: "every element comes from the kit, not from a module", + test: /document\s*[.[]|createElementNS|\[\s*['"`]createElement['"`]\s*\]/ }, + { name: "reaching-for-globals", why: "a module is handed what it may use; the rest is not for it", + test: /\bglobalThis\b|\bwindow\s*[.[]|\bself\s*\[/ }, { name: "markup", why: "content is data; the kit renders it as text", - test: /\binnerHTML\b|\bouterHTML\b|insertAdjacentHTML|document\s*\.\s*write/ }, + test: /\binnerHTML\b|\bouterHTML\b|insertAdjacentHTML|\[\s*['"`](inner|outer)HTML['"`]\s*\]/ }, { name: "stylesheet", why: "a module cannot ship styling, and cannot inject it either", test: /adoptedStyleSheets|document\s*\.\s*head|<\s*(link|style)\b/ }, { name: "hardcoded-colour", why: "colour comes from the theme tokens", @@ -23,7 +34,7 @@ export const MODULE_RULES = [ { name: "hardcoded-size", why: "sizes come from the spacing and control scales", test: /\b\d+(\.\d+)?px\b/ }, { name: "eval", why: "nothing in a module is compiled at runtime", - test: /\beval\s*\(|new\s+Function\s*\(/ }, + test: /\beval\s*[([]|new\s+Function\s*\(|\[\s*['"`](eval|Function)['"`]\s*\]/ }, ]; // The kit itself may style and may build elements -- that is its job -- but it