diff --git a/scripts/following-in-the-console.js b/scripts/following-in-the-console.js new file mode 100644 index 00000000..790d1227 --- /dev/null +++ b/scripts/following-in-the-console.js @@ -0,0 +1,201 @@ +/* + * Following, reported from the reader's own browser. + * + * Paste this into DevTools on a pull request's Files page, with the extension + * running. It presses names the way a reader does and says what happened. + * + * It exists because the probes cannot get here. A pull request is drawn from + * routes that answer to a session, so a fresh profile gets the signed-out card — + * measured on a public pull request, not assumed — and the machine this was + * written on has no display to sign one in with. The browser already looking at + * the page has both. + * + * The first run of this found that a press *does* open the panel on a diff, for + * a name written in the file being read — which is the opposite of what was + * reported. So this one watches what happens next: the panel closes on a press + * outside it and on any scroll that did not start inside it, and that second + * rule is listening in the capture phase on `window`, where it hears a scroll + * from anywhere on the page. A panel that opens and is taken away a moment + * later is, to a reader, a click that did nothing. + * + * Edit NAMES, paste, read. Nothing is sent anywhere; it prints to the console. + */ +;(async () => { + /** The names to press, as a reader would, and the line each is on. */ + const NAMES = [ + { word: "readBodyWithinLimit", line: 12 }, + { word: "BodyReadFailure", line: 5 } + ] + + /** How long to watch the panel after it opens, for it to be taken away. */ + const WATCH = 6000 + + const sleep = (ms) => new Promise((go) => setTimeout(go, ms)) + const panelNow = () => document.querySelector('[aria-label^="Uses of "]') + + const panes = () => { + const found = [] + const walk = (node) => { + for (const el of node.querySelectorAll("*")) { + if (!el.shadowRoot) continue + if (el.tagName.toLowerCase() === "diffs-container") found.push(el.shadowRoot) + else walk(el.shadowRoot) + } + } + walk(document) + return found + } + + const drawn = panes() + if (drawn.length === 0) { + console.log("%cno pane drew at all", "color:#f85149") + return + } + + /* + * Everything that could take the panel away, in the order it happens. + * + * The same two events the panel itself listens for, on the same targets and + * in the same phase, so what is logged here is what it heard. + */ + const heard = [] + const note = (what) => (event) => { + const target = event.target + const named = + target === document ? "document" + : target === window ? "window" + : target && target.nodeType === 1 + ? target.tagName.toLowerCase() + (target.id ? "#" + target.id : "") + + (typeof target.className === "string" && target.className ? "." + target.className.split(" ")[0] : "") + : String(target) + heard.push({ at: Math.round(performance.now()), what, from: named }) + } + const onScroll = note("scroll") + const onDown = note("pointerdown") + window.addEventListener("scroll", onScroll, true) + document.addEventListener("pointerdown", onDown, true) + + const edge = "[^A-Za-z0-9_$]" + + const press = async ({ word, line }) => { + const out = { name: `${word} @ L${line}` } + + let row = null, token = null, inside = -1 + for (const root of drawn) { + for (const one of root.querySelectorAll(`[data-line="${line}"]`)) { + for (const span of one.querySelectorAll("span")) { + if (span.querySelector("span")) continue + const text = span.textContent || "" + const at = text.search(new RegExp(`(^|${edge})${word}(${edge}|$)`)) + if (at === -1) continue + row = one; token = span; inside = text.indexOf(word, at) + break + } + if (token) break + } + if (token) break + } + if (!token) { + out.result = "the renderer drew no token holding that word on that line" + return out + } + + row.scrollIntoView({ block: "center", behavior: "instant" }) + await sleep(600) + + const box = token.getBoundingClientRect() + const text = token.textContent || "" + const wide = box.width / Math.max(1, text.length) + const where = { + bubbles: true, composed: true, cancelable: true, + clientX: box.left + (inside + word.length / 2) * wide, + clientY: box.top + box.height / 2, + metaKey: true, ctrlKey: true, button: 0, buttons: 1, + pointerId: 1, isPrimary: true, pointerType: "mouse" + } + + document.dispatchEvent(new KeyboardEvent("keydown", { key: "Meta", bubbles: true })) + token.dispatchEvent(new PointerEvent("pointerover", where)) + token.dispatchEvent(new PointerEvent("pointermove", where)) + + let underlined = false + for (let waited = 0; waited < 15000; waited += 50) { + const marked = (el) => (el.style.textDecoration || "") !== "" + if (marked(token) || [...token.querySelectorAll("span")].some(marked)) { underlined = true; break } + await sleep(50) + } + out.underlined = underlined + if (!underlined) { + out.result = "held the key and it never underlined" + return out + } + + /* + * The whole sequence a real mouse sends, not the three events a script + * usually bothers with. `mousedown` is the one a script leaves out and a + * hand never does, and it is what the drawing underneath acts on — a line + * marked, a selection begun, and whatever scrolling either causes. + */ + heard.length = 0 + const started = Math.round(performance.now()) + token.dispatchEvent(new PointerEvent("pointerdown", where)) + token.dispatchEvent(new MouseEvent("mousedown", where)) + token.dispatchEvent(new PointerEvent("pointerup", { ...where, buttons: 0 })) + token.dispatchEvent(new MouseEvent("mouseup", { ...where, buttons: 0 })) + token.dispatchEvent(new MouseEvent("click", { ...where, buttons: 0 })) + + let opened = null + for (let waited = 0; waited < 8000; waited += 50) { + const found = panelNow() + if (found) { opened = Math.round(performance.now()) - started; break } + await sleep(50) + } + if (opened === null) { + out.result = "no panel ever appeared" + out.heard = heard.slice(0, 6) + return out + } + + // It opened. Does it survive being looked at? + let gone = null + for (let waited = 0; waited < WATCH; waited += 50) { + if (!panelNow()) { gone = Math.round(performance.now()) - started; break } + await sleep(50) + } + + out.result = gone === null + ? `opened after ${opened}ms and stayed for ${WATCH}ms` + : `opened after ${opened}ms and was GONE by ${gone}ms` + out.heard = heard + .filter((one) => one.at - started >= 0 && (gone === null || one.at - started <= gone + 100)) + .map((one) => `${one.at - started}ms ${one.what} from ${one.from}`) + .slice(0, 10) + + document.dispatchEvent(new KeyboardEvent("keyup", { key: "Meta", bubbles: true })) + document.body.dispatchEvent( + new PointerEvent("pointerdown", { bubbles: true, composed: true, pointerId: 1, isPrimary: true, pointerType: "mouse" }) + ) + await sleep(400) + return out + } + + try { + const seen = [] + for (const one of NAMES) seen.push(await press(one)) + for (const one of seen) { + console.log(`%c${one.name}`, "font-weight:bold") + console.log(` underlined: ${one.underlined}`) + console.log(` result: ${one.result}`) + if (one.heard && one.heard.length > 0) { + console.log(` heard while it was up:`) + for (const line of one.heard) console.log(` ${line}`) + } else if (one.heard) { + console.log(` heard while it was up: nothing`) + } + } + console.log(seen) + } finally { + window.removeEventListener("scroll", onScroll, true) + document.removeEventListener("pointerdown", onDown, true) + } +})() diff --git a/scripts/probe-diff-following.ts b/scripts/probe-diff-following.ts new file mode 100644 index 00000000..ec79e5d6 --- /dev/null +++ b/scripts/probe-diff-following.ts @@ -0,0 +1,182 @@ +/** + * Following on a pull request, reported in full rather than as pass or fail. + * + * Every live check of Following so far was made on a blob page, because that is + * the only page this can reach on its own: a pull request is drawn from routes + * that answer to a session, and a fresh profile has none — measured, not + * assumed, on a *public* pull request, which showed our own signed-out card with + * `user-login` empty and only anonymous cookies. A diff is a different drawing — + * two halves, a marker column, line numbers belonging to one side — so none of + * what a blob page proves carries over on its own. + * + * So this needs a browser that is signed in, which means a profile that has been + * signed in once and kept: + * + * GITQUIET_CDP_PROFILE=~/.gitquiet-qa bun scripts/probe-diff-following.ts \ + * --page 'https://github.com/OWNER/REPO/pull/N/files' \ + * --names 'readBodyWithinLimit@12,AsyncResult@1,BodyReadFailure@5' + * + * Sign in once in that profile and it stays signed in between runs. Nothing here + * reads a cookie or moves one anywhere. + * + * For each name it says what the renderer handed over, what the underline did, + * and what the press did — a panel, a move, or nothing — because "nothing + * happens" is three different faults wearing one coat, and which one it is + * cannot be told from the outside. + */ +import { PANES, withExtension } from "./chrome" + +const argued = (flag: string): string | undefined => { + const at = Bun.argv.indexOf(flag) + return at === -1 ? undefined : Bun.argv[at + 1] +} + +const PAGE = argued("--page") ?? "https://github.com/flazouh/gitquiet/pull/82/files" + +/** `name@line` a few times over, which is what a reader would press. */ +const NAMES = (argued("--names") ?? "Secret@11").split(",").map((one) => { + const [word, line] = one.trim().split("@") + return { word: word ?? "", line: Number(line ?? 1) } +}) + +const session = await withExtension(PAGE, `${import.meta.dir}/../.output/chrome-mv3`) +const sleep = (ms: number) => new Promise((go) => setTimeout(go, ms)) + +type Report = { + readonly drew: boolean + readonly row?: string + readonly token?: { char: string | null; text: string; inside: number } + readonly underlined?: boolean + readonly press?: string + readonly why?: string +} + +const look = (word: string, line: number): Promise => + session.evaluate(` + (async () => { + ${PANES} + const sleep = (ms) => new Promise((go) => setTimeout(go, ms)) + + let drawn = [] + for (let tries = 0; tries < 60; tries++) { + drawn = panes() + if (drawn.length > 0) break + await sleep(500) + } + if (drawn.length === 0) return { drew: false, why: "no pane drew at all — signed out, or the diff never arrived" } + + const edge = "[^A-Za-z0-9_$]" + let row = null, token = null, inside = -1 + for (const root of drawn) { + for (const one of root.querySelectorAll('[data-line="${line}"]')) { + for (const span of one.querySelectorAll("span")) { + if (span.querySelector("span")) continue + const text = span.textContent || "" + const found = text.search(new RegExp("(^|" + edge + ")" + ${JSON.stringify(word)} + "(" + edge + "|$)")) + if (found === -1) continue + row = one; token = span; inside = text.indexOf(${JSON.stringify(word)}, found) + break + } + if (token) break + } + if (token) break + } + if (!token) return { drew: true, why: "the renderer drew no token holding that word on that line" } + + row.scrollIntoView({ block: "center", behavior: "instant" }) + await sleep(400) + + const at = token.getBoundingClientRect() + const text = token.textContent || "" + const wide = at.width / Math.max(1, text.length) + const where = { + bubbles: true, composed: true, cancelable: true, + clientX: at.left + (inside + ${JSON.stringify(word)}.length / 2) * wide, + clientY: at.top + at.height / 2, + metaKey: true, pointerId: 1, isPrimary: true, pointerType: "mouse" + } + + const seen = { + drew: true, + row: [...row.attributes].map((a) => a.name + "=" + a.value).join(" "), + token: { char: token.getAttribute("data-char"), text: text.slice(0, 60), inside } + } + + document.dispatchEvent(new KeyboardEvent("keydown", { key: "Meta", bubbles: true })) + token.dispatchEvent(new PointerEvent("pointerover", where)) + token.dispatchEvent(new PointerEvent("pointermove", where)) + + let underlined = false + for (let waited = 0; waited < 15000; waited += 50) { + if ((token.style.textDecoration || "") !== "" || + [...token.querySelectorAll("span")].some((s) => (s.style.textDecoration || "") !== "")) { + underlined = true + break + } + await sleep(50) + } + if (!underlined) return { ...seen, underlined: false, why: "held the key and it never underlined" } + + // Where the pane is before the press, so a move can be told from nothing. + const scrollerOf = (el) => { + let up = el + while (up) { + if (up.scrollHeight > up.clientHeight + 4) return up + up = up.parentElement || (up.getRootNode() || {}).host || null + } + return document.scrollingElement + } + const scroller = scrollerOf(row) + const before = scroller ? scroller.scrollTop : 0 + + token.dispatchEvent(new PointerEvent("pointerdown", where)) + token.dispatchEvent(new PointerEvent("pointerup", where)) + token.dispatchEvent(new MouseEvent("click", where)) + + let panel = null + for (let waited = 0; waited < 8000; waited += 100) { + panel = document.querySelector('[aria-label^="Uses of "]') + if (panel) break + await sleep(100) + } + const after = scroller ? scroller.scrollTop : 0 + + return { + ...seen, + underlined: true, + press: panel + ? "opened " + JSON.stringify(panel.getAttribute("aria-label")) + : after !== before + ? "moved the pane from " + before + " to " + after + : "nothing: no panel, and the pane did not move" + } + })() + `) + +try { + console.log(`page: ${PAGE}\n`) + for (const { word, line } of NAMES) { + const seen = await look(word, line) + console.log(`${word} @ L${line}`) + for (const [key, value] of Object.entries(seen)) { + console.log(` ${key}: ${typeof value === "object" ? JSON.stringify(value) : value}`) + } + console.log() + await session.evaluate(` + (() => { + document.dispatchEvent(new KeyboardEvent("keyup", { key: "Meta", bubbles: true })) + document.body.dispatchEvent(new PointerEvent("pointerdown", { bubbles: true, composed: true, pointerId: 1, isPrimary: true, pointerType: "mouse" })) + })() + `) + await sleep(500) + } + + const problems = session.problems() + if (problems.length > 0) { + console.log("the page logged:") + for (const one of problems.slice(0, 8)) console.log(` ${one.split("\n")[0]}`) + } +} finally { + await session.screenshot(`${import.meta.dir}/../.output/qa/diff-following.png`) + session.stop() +} diff --git a/src/entrypoints/offscreen/ledger.ts b/src/entrypoints/offscreen/ledger.ts index b5e331c9..a7ae9c51 100644 --- a/src/entrypoints/offscreen/ledger.ts +++ b/src/entrypoints/offscreen/ledger.ts @@ -19,6 +19,7 @@ import { withinPackage, type Held } from "@/ledger/packages" +import { within } from "@/ledger/reaching" import { heldIn, holdIn, holdingOf } from "@/ledger/holding" import { idbStore, noStore, type Store } from "@/ledger/store" import { usesAcross, type Asked } from "@/ledger/uses" @@ -604,8 +605,24 @@ const beyond = (work: LedgerBeyondWork): Effect.Effect => // Its own, which is most monorepos and costs no request at all. const own = ledger?.packages.get(packageOf(work.specifier)) if (own !== undefined) { - const within = withinPackage(work.specifier) - const path = within === null ? own.entry : `${own.at === "" ? "" : `${own.at}/`}${within}` + const deeper = withinPackage(work.specifier) + const named = + deeper === null ? own.entry : `${own.at === "" ? "" : `${own.at}/`}${deeper}` + /* + * And checked against the files the repository really has. + * + * A deep import names a path inside the package and not a file: + * `@org/type-utils/result-monad` is `packages/type-utils/result-monad`, + * with no ending on it and nothing of that name on disk. It was handed on + * as written, matched no file, and the answer fell through to the first + * Writing of that name anywhere in the repository — a different thing + * with the same spelling, offered to the reader as the place it is + * written. Which is the one mistake this whole feature exists to prevent. + */ + const path = + named === null || ledger === undefined + ? named + : within(named, new Set(ledger.files.keys())) ?? named if (path !== null) { const found = ledger === undefined ? [] : placesFor(ledger, work.name) const here = found.find((one) => one.path === path) ?? found[0] diff --git a/src/ledger/reaching.test.ts b/src/ledger/reaching.test.ts index e2d3e25e..14719a9e 100644 --- a/src/ledger/reaching.test.ts +++ b/src/ledger/reaching.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test" -import { couldBe, reaching } from "./reaching" +import { couldBe, reaching, within } from "./reaching" /** A repository laid out the way most of them are. */ const PATHS = new Set([ @@ -54,3 +54,52 @@ describe("which file a specifier names", () => { expect(reaching("src/two.ts", "./one", both)).toBe("src/one.ts") }) }) + +/** + * A path arrived at through a package's own folder, rather than through a + * specifier relative to a file. + * + * `@org/type-utils` held at `packages/type-utils` and imported as + * `@org/type-utils/result-monad` means `packages/type-utils/result-monad` — a + * path with no ending on it and nothing of that name on disk. It was handed on + * as written, so it matched no file, and the answer fell through to the first + * Writing of that name anywhere in the repository. A reader pressing + * `AsyncResult` was shown a different thing with the same spelling and told it + * was the place. + */ +describe("a path inside the repository", () => { + const paths = new Set([ + "packages/type-utils/result-monad.ts", + "packages/helpers/read-body-within-limit.ts", + "services/api/src/index.tsx", + "packages/deep/nested/index.ts" + ]) + + test("finds the file a folder and a name really mean", () => { + expect(within("packages/type-utils/result-monad", paths)).toBe( + "packages/type-utils/result-monad.ts" + ) + }) + + test("finds a folder's own index, which is how a package names itself", () => { + expect(within("packages/deep/nested", paths)).toBe("packages/deep/nested/index.ts") + }) + + test("takes a path that already names its ending as written", () => { + expect(within("services/api/src/index.tsx", paths)).toBe("services/api/src/index.tsx") + }) + + test("answers nothing where the repository holds no such file", () => { + // A path built from a package's layout is a claim about the repository, and + // it is checked like every other claim here rather than believed. + expect(within("packages/type-utils/nowhere", paths)).toBeNull() + expect(within("", paths)).toBeNull() + }) + + test("says `a/b/../c` plainly, and refuses a climb out of the repository", () => { + expect(within("packages/helpers/../type-utils/result-monad", paths)).toBe( + "packages/type-utils/result-monad.ts" + ) + expect(within("../../etc/passwd", paths)).toBeNull() + }) +}) diff --git a/src/ledger/reaching.ts b/src/ledger/reaching.ts index fdb1a80b..14956fb3 100644 --- a/src/ledger/reaching.ts +++ b/src/ledger/reaching.ts @@ -67,11 +67,29 @@ export const couldBe = (from: string, specifier: string): ReadonlyArray const folder = from.slice(0, Math.max(0, from.lastIndexOf("/"))) const asked = plainly(`${folder}/${specifier}`) - if (asked === null || asked === "") return [] + if (asked === null) return [] - // A specifier that already names its ending is taken as written. TypeScript's - // own `.js`-means-`.ts` rule is the one exception worth keeping, because - // every ES-module TypeScript repository is written that way. + return endingsFor(asked) +} + +/** + * The files a path inside the repository could be, before anything is checked. + * + * Split out from {@link couldBe} because a relative specifier is not the only + * way to arrive at a path with no ending on it. A package this repository holds + * itself resolves to a folder and a path inside it — `@org/type-utils` at + * `packages/type-utils`, imported as `@org/type-utils/result-monad`, means + * `packages/type-utils/result-monad` and whatever that file is really called. + * That path was being handed on with no ending at all, so it matched no file in + * the repository and the answer fell through to the first Writing of that name + * anywhere — a different thing with the same spelling, offered as the place. + */ +export const endingsFor = (asked: string): ReadonlyArray => { + if (asked === "") return [] + + // A path that already names its ending is taken as written. TypeScript's own + // `.js`-means-`.ts` rule is the one exception worth keeping, because every + // ES-module TypeScript repository is written that way. if (asked.endsWith(".ts") || asked.endsWith(".tsx")) return [asked] if (asked.endsWith(".js")) { return [`${asked.slice(0, -3)}.ts`, `${asked.slice(0, -3)}.tsx`, asked] @@ -92,3 +110,15 @@ export const reaching = ( specifier: string, paths: ReadonlySet ): string | null => couldBe(from, specifier).find((path) => paths.has(path)) ?? null + +/** + * The file a path inside the repository names, out of the paths that exist. + * + * {@link reaching} for a path that is already a path — arrived at through a + * package's own folder rather than through a specifier relative to a file. + * Nothing where the repository holds no such file, so a path built from a + * package's layout is checked before it is believed, like every other answer + * here. + */ +export const within = (path: string, paths: ReadonlySet): string | null => + endingsFor(plainly(path) ?? "").find((one) => paths.has(one)) ?? null diff --git a/src/ui/Ask.tsx b/src/ui/Ask.tsx index 9fe23f57..1fb9d3ec 100644 --- a/src/ui/Ask.tsx +++ b/src/ui/Ask.tsx @@ -6,7 +6,7 @@ import type { ArtName } from "./art" import { useArt } from "./art" import { ASK, ASK_GROUP, ASK_MORE, ASK_NO, ASK_OUT, ASK_YES, FLOAT } from "./dress" import { LOOK } from "./rowDoings" -import { ROOT_ID } from "./mount" +import { OVER_ID, outsideHost } from "./outside" import { Says } from "./says" export type MergeActions = { @@ -378,7 +378,7 @@ const Caret = ({ const art = useArt() const Down = art["chevron-down"] const Tick = art.tick - const inOurs = typeof document === "undefined" ? null : document.getElementById(ROOT_ID) + const inOurs = typeof document === "undefined" ? null : outsideHost(document, OVER_ID) return ( @@ -470,7 +470,7 @@ export const Overflow = ({ }) => { const art = useArt() const More = art.more - const inOurs = typeof document === "undefined" ? null : document.getElementById(ROOT_ID) + const inOurs = typeof document === "undefined" ? null : outsideHost(document, OVER_ID) const offered = verbs.filter((doing) => actions?.[doing] !== undefined) if (offered.length === 0) return null diff --git a/src/ui/Doings.tsx b/src/ui/Doings.tsx index 31984585..fc6f8a8c 100644 --- a/src/ui/Doings.tsx +++ b/src/ui/Doings.tsx @@ -8,7 +8,7 @@ import { type Set, useArt } from "./art" import { askAndSay } from "./askAndSay" import { FLOAT } from "./dress" import { Cap } from "./Cap" -import { ROOT_ID } from "./mount" +import { OVER_ID, outsideHost } from "./outside" import { SpinnerIcon } from "./spinner" import { ARMED, COPY_LETTER, LETTER, LOOK, ORDER, WORD } from "./rowDoings" import { useKeying, useLetters } from "./useLetters" @@ -269,7 +269,7 @@ export const Doings = ({ > {waiting ? : } - + - typeof document === "undefined" ? null : document.getElementById(ROOT_ID) + typeof document === "undefined" ? null : outsideHost(document, OVER_ID) export type SettingsMenuProps = { readonly settings: Settings diff --git a/src/ui/Settle.tsx b/src/ui/Settle.tsx index db65389f..44d579cf 100644 --- a/src/ui/Settle.tsx +++ b/src/ui/Settle.tsx @@ -8,7 +8,7 @@ import type { IssueState } from "../domain/issues" import { type ArtName, useArt } from "./art" import { Cap } from "./Cap" import { FIELD, FLOAT, PRESSABLE } from "./dress" -import { ROOT_ID } from "./mount" +import { OVER_ID, outsideHost } from "./outside" import { useKeying, useLetters } from "./useLetters" import { onward } from "@/observability/report" @@ -175,7 +175,7 @@ export const Settle = ({ state, where, allowed, onSettle, onReopen }: SettleProp Close issue - + { if (scope === "document") return document.documentElement - return document.getElementById(ROOT_ID) + return rootIn(document) } /** diff --git a/src/ui/motion.ts b/src/ui/motion.ts index 75ad2e78..8906cd85 100644 --- a/src/ui/motion.ts +++ b/src/ui/motion.ts @@ -8,6 +8,22 @@ * tunes the first one. */ export const millisOf = (name: string, fallback: number): number => { + /* + * `document` on purpose, and not our tree. + * + * This is one half of a seam whose other half is `tests/paced.ts`, which + * plants a `#gitquiet-root` in `document.body` and writes the durations a test + * needs onto it. Reading through `rootIn` instead makes the two halves + * disagree the moment a screen stands a root inside the shadow root: the + * planted durations are ignored, a dissolve paced to never finish finishes at + * once, and React throws the element away and mounts a second one — which is a + * transition with nothing to transition from. It cost two CI runs to find, + * being a fault that depends on what else is in the worker. + * + * Moving the clock into the shadow root is worth doing — the CSS owns these + * numbers and the sheet is in there now — but it is a change to both halves and + * it is not this one. + */ const root = document.getElementById("gitquiet-root") if (root === null) return fallback diff --git a/src/ui/mount.ts b/src/ui/mount.ts index 5df50fbe..0417f6e8 100644 --- a/src/ui/mount.ts +++ b/src/ui/mount.ts @@ -118,8 +118,18 @@ const oursIn = (target: Document): ParentNode => ourTree(target) ?? target * `getElementById` found it there without being asked twice. Looking only in the * shadow root made such a container invisible to everything here: the screen was * on the page, nothing could see it, and the bar it had drawn never came down. + * + * Exported because this is the only way to ask, and the components were asking + * the other way. `document.getElementById(ROOT_ID)` was right for as long as the + * root was a child of `body`; the shadow root ended that and the call sites were + * left behind, each one silently answering null. Radix reads a null `container` + * as "portal to `body`", so every dropdown in the interface rendered into their + * document, where the gate rule hides anything unmarked — a menu that opened, + * drew, and could not be seen. The theme's fallback target and the motion + * durations went the same way. There is nothing to find in `document` any more, + * so nothing may look there. */ -const rootIn = (target: Document): HTMLElement | null => +export const rootIn = (target: Document): HTMLElement | null => oursIn(target).querySelector(`#${ROOT_ID}`) ?? target.querySelector(`#${ROOT_ID}`) diff --git a/src/ui/primer.css b/src/ui/primer.css index b1a2ca76..fe293af1 100644 --- a/src/ui/primer.css +++ b/src/ui/primer.css @@ -87,6 +87,29 @@ min-width: 0; } +/** + * The same typographic baseline for our furniture out in their document. + * + * `#gitquiet-root` above declares the font and the size; everything outside it — + * the bar, the hover cards, anything Radix portals — used to need no such + * declaration, because it stood in `body` and inherited whatever GitHub's + * stylesheet put there. Then we started turning their stylesheets off, and + * `body` was left with no font at all: the bar rendered in Times New Roman at + * the browser's own sixteen pixels, against an interface set in Inter at + * fourteen. Measured on a pull request — `font-family: Times`, `font-size: 16px` + * on the bar — which is also why its padding looked wrong, every row inside it + * being a seventh taller than it was drawn to be. + * + * Inheritance is not a thing to rely on once you have taken the page it comes + * from. Through `:where` so it costs nothing: a `text-sm` on the bar itself + * still wins, and so does every utility inside it. + */ +:where([data-gitquiet-outside]) { + color: var(--color-ink); + font-family: var(--font-sans); + font-size: var(--text-base); +} + #gitquiet-root *, #gitquiet-root *::before, #gitquiet-root *::after, @@ -151,8 +174,28 @@ * back, and `.markdown-body p` — GitHub's own sixteen pixels between the * paragraphs of a README we insert whole — never hears this at all. */ +:where(#gitquiet-root) :is(p, h1, h2, h3, h4, h5, h6, ul, ol, dl, dd, figure, blockquote, pre) { + margin: 0; +} + +/* + * The same reset for our furniture out in their document, where the weight above + * is still needed and `html` still matches. + * + * These were one rule until the interface moved into a shadow root, and the + * `html` in front of it — the whole of its weight — quietly stopped matching + * anything: `html` is the document's element and a shadow tree has none. So the + * reset was off for every screen, and every paragraph and heading this interface + * writes carried the browser's own margin. It showed as slack between the rows of + * a panel, most of a line of it, on a layout built to sit tight. + * + * Split rather than made to match both, because the two halves want different + * weights for different reasons. Nothing of GitHub's reaches inside the shadow + * root, so one class-lighter selector wins there uncontested; out here theirs is + * still on the page and this has to outweigh `p { margin-bottom: 10px }`. + */ html - :where(#gitquiet-root, [data-gitquiet-outside]) + :where([data-gitquiet-outside]) :is(p, h1, h2, h3, h4, h5, h6, ul, ol, dl, dd, figure, blockquote, pre) { margin: 0; } diff --git a/src/ui/refraction.ts b/src/ui/refraction.ts index c7a4a58f..ec9c605a 100644 --- a/src/ui/refraction.ts +++ b/src/ui/refraction.ts @@ -15,6 +15,8 @@ * floats over a page the reader is still scrolling. */ +import { OUTSIDE } from "./mount" + /** The name the stylesheet asks for. See `glass.css`. */ export const REFRACTION_ID = "gitquiet-refraction" @@ -52,6 +54,17 @@ export const keepRefraction = (page: Document): void => { const host = page.createElement("div") host.id = HOST_ID + /* + * Marked, because the gate rule hides every child of `body` that is not. + * + * The paragraph above says a filter in a hidden subtree is one Chrome declines + * to run, and that the failure is a bar with no backdrop at all. The gate rule + * this interface now hides their page with put `display: none` on exactly this + * host — our own definition, swept up with theirs — so the effect this whole + * file exists for was off in production and nothing said so. The mark is what + * tells that rule ours from theirs. + */ + host.setAttribute(OUTSIDE, "") host.setAttribute("aria-hidden", "true") host.style.position = "absolute" host.style.width = "0" diff --git a/src/ui/settingsMenu.test.tsx b/src/ui/settingsMenu.test.tsx index 220050a6..b6b02baf 100644 --- a/src/ui/settingsMenu.test.tsx +++ b/src/ui/settingsMenu.test.tsx @@ -2,20 +2,28 @@ import { afterEach, describe, expect, test } from "bun:test" import { cleanup, fireEvent, render, screen } from "@testing-library/react" import userEvent from "@testing-library/user-event" import { DEFAULTS, type Settings } from "../domain/Settings" -import { ROOT_ID } from "./mount" +import { OUTSIDE, ROOT_ID } from "./mount" import { SettingsMenu } from "./SettingsMenu" afterEach(cleanup) /** - * Everything this button opens has to open inside our own root. + * Everything this button opens has to open somewhere painted. * - * The colours are inline custom properties on `#gitquiet-root` and not on - * ``, because the rest of the document is GitHub's page and our names on - * their root would repaint their chrome. So anything Radix portals to - * `document.body` is drawn with the stylesheet's defaults, which are the light - * pack — white panel, near-black text, on a dark page. `outside.ts` was written - * for that failure, having been paid for once by the bar. + * The colours are inline custom properties rather than rules on ``, because + * the rest of the document is GitHub's page and our names on their root would + * repaint their chrome. So anything Radix portals to a plain `document.body` is + * drawn with the stylesheet's defaults, which are the light pack — white panel, + * near-black text, on a dark page. `outside.ts` was written for that failure, + * having been paid for once by the bar. + * + * It used to say "inside `#gitquiet-root`", and that was one painted place rather + * than the property. The interface has since moved into a shadow root, where the + * root is out of reach of a lookup on `document`, and an overlay has to escape + * whatever its row is clipped by — which is the one thing a panel standing inside + * the root cannot do. So this asks what it always meant to ask: that the panel + * stands in a host of ours, which is a host `outside.ts` has painted and the gate + * rule spares. */ describe("the panel of knobs", () => { const ourRoot = (): HTMLElement => { @@ -43,7 +51,10 @@ describe("the panel of knobs", () => { const panel = await opened(root) - expect(root.contains(panel)).toBe(true) + // `closest` rather than a named id: what has to be true is that the panel is + // inside something of ours, and the mark is what says so — to the theme that + // paints it and to the gate rule that would otherwise hide it. + expect(panel.closest(`[${OUTSIDE}]`)).not.toBeNull() }) /** diff --git a/src/ui/shadowReach.test.ts b/src/ui/shadowReach.test.ts new file mode 100644 index 00000000..0ea34892 --- /dev/null +++ b/src/ui/shadowReach.test.ts @@ -0,0 +1,196 @@ +import { describe, expect, test } from "bun:test" +import { readdirSync, readFileSync } from "node:fs" +import { join } from "node:path" +import { OUTSIDE, ROOT_ID, rootIn } from "./mount" +import { outsideHost } from "./outside" +import { theHost } from "./theHost" +import { keepRefraction } from "./refraction" + +/** + * What moving into a shadow root put out of reach, and keeps out of reach. + * + * The migration moved the whole interface under `#gitquiet-host`'s shadow root. + * Three kinds of thing carried on addressing it as though it were still a child + * of `body`, and every one of them failed silently — no error, no warning, a + * lookup that answered null or a rule that matched nothing: + * + * - `document.getElementById("gitquiet-root")` returns null now. Radix reads a + * null portal `container` as "put it in `body`", so every dropdown in the + * interface rendered into their document, where the gate rule hid it. The + * reader saw a menu that would not open. The answer is the host every other + * overlay already used — see below; the root was never the right target for a + * thing that has to escape whatever clips it. + * - A selector led by `html` matches nothing inside a shadow tree, because a + * shadow tree has no document element. That was the whole weight of the margin + * reset, so every paragraph and heading in the interface wore the browser's + * default margin. + * - The gate rule hides every child of `body` without the outside mark, and one + * of the things it was hiding was ours: the SVG filter the bar's glass is made + * of, defined in a subtree Chrome then declined to run. + * + * Each of those was one line. None of them could fail a test that only renders + * components, because each one is a lookup that answers politely and wrongly. So + * the class is tested rather than the three instances. + */ +const uiDir = new URL(".", import.meta.url).pathname +const sourcesIn = (dir: string, ext: string): ReadonlyArray<[string, string]> => + readdirSync(dir) + .filter((name) => name.endsWith(ext) && !name.includes(".test.")) + .map((name) => [name, readFileSync(join(dir, name), "utf8")] as [string, string]) + +describe("nothing looks for our own elements in their document", () => { + test("no source asks `document` for the root", () => { + // The lookup that broke every dropdown. `rootIn` asks the shadow root first + // and their document after, which is the only way that answers on both. + const guilty = [...sourcesIn(uiDir, ".ts"), ...sourcesIn(uiDir, ".tsx")] + // `mount.ts` defines the helper. `motion.ts` reads the stylesheet's clock + // through `document` deliberately, because the test seam that writes those + // durations plants its root there — see the comment on `millisOf`, and the + // two CI runs it cost to establish that the two halves have to agree. + .filter(([name]) => name !== "mount.ts" && name !== "motion.ts") + .filter(([, text]) => + /document\.getElementById\(\s*(ROOT_ID|["'`]gitquiet-root)/.test(text) || + /document\.querySelector\w*\(\s*["'`]#gitquiet-root/.test(text) + ) + .map(([name]) => name) + + expect(guilty).toEqual([]) + }) + + test("and `rootIn` finds it through the shadow boundary", () => { + // The positive half: the helper the rule above points at has to actually + // answer where `document` cannot, or the rule is just a ban. + const page = document.implementation.createHTMLDocument("github") + const { shadow } = theHost(page) + const root = page.createElement("div") + root.id = ROOT_ID + shadow.append(root) + + // The lookup the components used to make, against the tree they now stand in. + expect(page.getElementById(ROOT_ID)).toBeNull() + expect(rootIn(page)).toBe(root) + }) +}) + +describe("no rule reaches our root from the document element", () => { + test("nothing keyed on `html` or `body` styles anything inside the shadow root", () => { + /* + * `html` and `body` are the document's elements. A shadow tree has neither, + * so a rule led by one of them is a rule that matches nothing in here — and + * it fails by doing nothing at all, which is the hardest kind of failure to + * notice. The rules that legitimately key on `html` are the gates and the + * widths, and every one of those styles GitHub's page or our furniture out + * in it, never the interface. + */ + const guilty: string[] = [] + for (const [name, text] of sourcesIn(uiDir, ".css")) { + // Every prelude in the file: the run of characters before an opening brace + // that is not itself a brace, which is a selector list or an at-rule. + for (const found of text.replace(/\/\*[\s\S]*?\*\//g, "").matchAll(/([^{}]+)\{/g)) { + const selector = (found[1] ?? "").trim() + if (selector.startsWith("@")) continue + if (!selector.includes(`#${ROOT_ID}`)) continue + // A selector list, because one rule may carry several. + for (const one of selector.split(",")) { + if (/^(html|body)\b/.test(one.trim()) && one.includes(`#${ROOT_ID}`)) { + guilty.push(`${name}: ${one.trim().replace(/\s+/g, " ")}`) + } + } + } + } + + expect(guilty).toEqual([]) + }) +}) + +describe("every overlay goes to the one host built for overlays", () => { + test("no menu portals into the screen root", () => { + /* + * The hover cards, the settings dialog and the toasts all portal to + * `outsideHost(document, OVER_ID)`: a child of `body` that carries the + * outside mark, so the gate rule spares it, and the theme tokens, so it is + * painted. The dropdown menus were the only overlays not using it — they + * named `#gitquiet-root` instead, which was a child of `body` too until the + * interface moved into a shadow root and the lookup started answering null. + * + * Radix then put them in `body` unmarked, where the gate rule hid them. A + * menu that opens and cannot be seen. + * + * The root is the wrong target regardless: an overlay exists to escape + * whatever its row is clipped by, which is the one thing standing inside the + * root cannot do. + */ + const guilty = [...sourcesIn(uiDir, ".tsx")] + .filter(([, text]) => /container=\{[^}]*\bROOT_ID\b|container=\{[^}]*\brootIn\(/.test(text)) + .map(([name]) => name) + + expect(guilty).toEqual([]) + }) + + test("and the host they do use is one their page cannot hide", () => { + const page = document.implementation.createHTMLDocument("github") + const host = outsideHost(page, "gitquiet-over") + + expect(host.parentElement).toBe(page.body) + expect(host.hasAttribute(OUTSIDE)).toBe(true) + }) +}) + +describe("nothing of ours inherits from a page we have undressed", () => { + test("our furniture in their document declares its own typography", () => { + /* + * The bar and the hover cards stand in `body`, and used to take their font + * from whatever GitHub's stylesheet put there. Then we started turning their + * stylesheets off for the recalculation saving, and `body` was left with no + * font at all: the bar came out in Times New Roman at sixteen pixels beside + * an interface set in Inter at fourteen, which is also why its padding read + * as wrong — every row in it a seventh taller than it was drawn to be. + * + * Whatever the root declares for itself, the furniture outside it has to + * declare too. There is nothing left to inherit from. + */ + const primer = readFileSync(join(uiDir, "primer.css"), "utf8") + const declaredFor = (selector: string): ReadonlyArray => { + const at = primer.indexOf(`\n${selector} {`) + if (at === -1) return [] + const block = primer.slice(at, primer.indexOf("}", at)) + return ["font-family", "font-size", "color"].filter((one) => block.includes(`${one}:`)) + } + + expect(declaredFor("#gitquiet-root")).not.toEqual([]) + expect(declaredFor(":where([data-gitquiet-outside])")).toEqual(declaredFor("#gitquiet-root")) + }) +}) + +describe("everything of ours in their document is marked as ours", () => { + test("the glass filter is spared by the gate rule", () => { + /* + * It was not, and the file that puts it there says in its own comment why + * that is fatal: Chrome drops a `backdrop-filter` whose filter it cannot + * resolve, and a filter inside a `display: none` subtree is one it declines + * to run. The bar had no backdrop in production and nothing failed. + */ + const page = document.implementation.createHTMLDocument("github") + keepRefraction(page) + + const host = page.getElementById("gitquiet-glass") + expect(host).not.toBeNull() + expect(host?.hasAttribute(OUTSIDE)).toBe(true) + }) + + test("and so is everything else we put straight into body", () => { + // The general form. Anything appended to their `body` is subject to the gate + // rule, so the mark is not a detail of one host — it is the condition of + // being allowed to live there at all. + const page = document.implementation.createHTMLDocument("github") + keepRefraction(page) + outsideHost(page, "gitquiet-over") + + const unmarked = [...page.body.children] + .filter((el) => el.id !== "gitquiet-host") + .filter((el) => !el.hasAttribute(OUTSIDE)) + .map((el) => el.id || el.tagName) + + expect(unmarked).toEqual([]) + }) +}) diff --git a/src/ui/theHost.ts b/src/ui/theHost.ts index 2fd35636..c8b8c254 100644 --- a/src/ui/theHost.ts +++ b/src/ui/theHost.ts @@ -259,6 +259,28 @@ export const keepTheirStylesOff = (target: Document): void => { watching.observe(target.documentElement, { childList: true, subtree: true }) } +/** + * The host off a document, and forgotten, for a suite that is many documents in one. + * + * The host is module state twice over: an element in `body` and an entry in + * {@link hosts} keyed by the document. A test file leaves both behind, and the + * file that runs next in the same worker inherits a shadow root with the last + * file's screen still standing in it. Everything that looks for our tree then + * finds that one first — `rootIn` answers with a root nobody rendered, and a menu + * portalled into it lands where `screen` cannot see it, so the test reads as a + * menu that never opened. + * + * It surfaced as a CI-only failure, which is the signature of the fault rather + * than a detail of it: `bun test --parallel` shards by core count, so which files + * share a worker differs between a runner and a developer's machine, and the same + * commit was green here and red there. + */ +export const forgetTheHost = (page: Document): void => { + page.getElementById(HOST_ID)?.remove() + hosts.delete(page) + letTheirStylesBack(page) +} + /** Their sheets back on, and the watch let go. */ export const letTheirStylesBack = (target: Document): void => { watching?.disconnect() diff --git a/src/ui/widths.css b/src/ui/widths.css index 65105f4c..9657e435 100644 --- a/src/ui/widths.css +++ b/src/ui/widths.css @@ -86,10 +86,17 @@ * that adds its own is a screen that disagrees with the bar the moment either number changes, * which is what `WorkingSetScreen` did until this rule took the inset off it. * - * It cannot reach anything of GitHub's: they have no element with this id, and the takeover - * attribute means it says nothing at all on a page this extension left alone. + * It cannot reach anything of GitHub's: they have no element with this id, which is the whole + * of what keeps it off their page. + * + * It was `html[data-gitquiet-taken] #gitquiet-root` until the interface moved into a shadow + * root, and `html` is the document's element — a shadow tree has none, so the rule matched + * nothing and every screen lost its gutter, flush to the edge of the window with the bar above + * it still inset. The takeover attribute was never load-bearing here, as the line above already + * says: their page has no element with this id to reach. So it goes, and the id stands on its + * own in both trees. */ -html[data-gitquiet-taken] #gitquiet-root { +#gitquiet-root { padding-inline: var(--gitquiet-gutter); } diff --git a/tests/setup.ts b/tests/setup.ts index ff62ed03..212a23fe 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -3,6 +3,7 @@ import { afterEach, setDefaultTimeout } from "bun:test" import { forgetFlights } from "../src/github/flight" import { forgetDrawn } from "../src/ui/lastDrawn" import { forgetTheSpot } from "../src/shell/handOver" +import { forgetTheHost } from "../src/ui/theHost" import { forgetLanded } from "../src/ui/landing" import { forgetLanded as forgetOurWrites } from "../src/github/landed" import { forgetEverything } from "./storage" @@ -151,3 +152,16 @@ afterEach(forgetDrawn) * must not inherit where the file before it left the thing. */ afterEach(forgetTheSpot) + +/* + * And the host the interface stands in, which is one element and one entry in a map + * keyed by the document. + * + * A file that stands the interface up leaves a shadow root in `body` with its last + * screen still in it, and the file after it in the same worker inherits both. Every + * lookup for our tree finds that one first: a menu portalled into it renders where + * `screen` cannot see it, and the test reads as a menu that never opened. It showed + * up only on CI, because `--parallel` shards by core count and which files share a + * worker is a fact about the machine. + */ +afterEach(() => forgetTheHost(document))