trevato/forge

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

More
Forks1g fMapg aRuns12g rEventsFlows

The same twenty icons, once each: the front door stops re-sending its drawings #154

Merged masoncrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

mason/forge:mason/134-the-same-twenty-icons-once-each-the-fron → main 1 commit fork

  • Merged. mason/134-the-same-twenty-icons-once-each-the-fron was merged into main as 3e85d1a.

Review

Sign in to review this change.
6 files changed +270 −17
src/web/client.ts +6
@@ -1919,6 +1919,12 @@ export const liveRegionsJs = /* js */ `
19191919 .then(function (r) { return r.ok ? r.text() : Promise.reject(r); })
19201920 .then(function (text) {
19211921 var fresh = new DOMParser().parseFromString(text, 'text/html');
1922 // The fresh page's drawings come across first: a region swapping in
1923 // an icon this page never painted would otherwise point at a
1924 // <symbol> the document does not have (src/web/icons.ts).
1925 var theirSprite = fresh.querySelector('.sprite');
1926 var mySprite = document.querySelector('.sprite');
1927 if (theirSprite && mySprite) mySprite.replaceWith(theirSprite);
19221928 ids.forEach(function (id) {
19231929 var mine = document.getElementById(id);
19241930 var theirs = fresh.getElementById(id);
src/web/icons.ts +64 −6
@@ -12,16 +12,31 @@
1212 * fixed 16px slot by `.ico` so a row's baseline never moves when one appears.
1313 * Icons are decorative: they carry `aria-hidden`, and any control that shows
1414 * only an icon must carry its own `aria-label`.
15 *
16 * A drawing is sent once per page, not once per use. `icon()` renders the
17 * 16px slot with a one-line `<use>` inside it; the page's markup is read for
18 * those references and `iconSprite()` emits one `<symbol>` per distinct icon
19 * the page actually used, at the end of `<body>` (`layout.ts`) — legal, and
20 * resolved fine by a `<use>` earlier in the document. The stroke, the fill
21 * and the viewBox ride on the `<symbol>`, so the slot itself is four
22 * attributes; `currentColor` still resolves against whatever colour the
23 * `<use>` sits in, which is what keeps every theme working.
24 *
25 * A fragment (`layout.fragment()`) is markup dropped into a document it does
26 * not own, so it cannot reference a sprite it did not ship: `inlineIcons()`
27 * expands every reference back into the nested `<svg>` a `<use>` would have
28 * instantiated anyway — same pixels, no ids to collide.
1529 */
1630
1731 import { raw, type Html } from "./html.ts";
1832
19 const svg = (paths: string, size: number, name = ""): Html =>
33 /** What the drawing needs, on the symbol rather than on every slot. */
34 const DRAW = `viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="1.75" stroke-linecap="round" stroke-linejoin="round"`;
35
36 const svg = (name: string, size: number): Html =>
2037 raw(
21 `<svg class="ico${name === "" ? "" : ` ico-${name}`}" width="${size}" height="${size}" viewBox="0 0 24 24" ` +
22 `fill="none" stroke="currentColor" stroke-width="1.75" ` +
23 `stroke-linecap="round" stroke-linejoin="round" ` +
24 `aria-hidden="true" focusable="false">${paths}</svg>`,
38 `<svg class="ico ico-${name}" width="${size}" height="${size}" ` +
39 `aria-hidden="true" focusable="false"><use href="#i-${name}"/></svg>`,
2540 );
2641
2742 const PATHS = {
@@ -96,7 +111,50 @@ export type IconName = keyof typeof PATHS;
96111
97112 /** One icon, in the standard 16px slot (20px for empty-state headers). */
98113 export function icon(name: IconName, size: 16 | 20 = 16): Html {
99 return svg(PATHS[name], size, name);
114 return svg(name, size);
115 }
116
117 /** The reference `icon()` leaves behind, and the only thing read back. */
118 const REFERENCE = /<use href="#i-([A-Za-z]+)"\/>/g;
119
120 /** Every icon this markup references, once each, in first-seen order. */
121 function referenced(markup: string): IconName[] {
122 const names: IconName[] = [];
123 for (const match of markup.matchAll(REFERENCE)) {
124 const name = match[1];
125 if (name === undefined || !(name in PATHS)) continue;
126 if (!names.includes(name as IconName)) names.push(name as IconName);
127 }
128 return names;
129 }
130
131 /**
132 * The drawings a page used, defined once each. Sat at the end of `<body>`
133 * (`layout.ts`), zero-sized by `.sprite` so it can never move a layout;
134 * `<symbol>` content is never rendered where it is defined, only where a
135 * `<use>` instantiates it.
136 */
137 export function iconSprite(markup: string): Html {
138 const names = referenced(markup);
139 if (names.length === 0) return raw("");
140 const symbols = names
141 .map((name) => `<symbol id="i-${name}" ${DRAW}>${PATHS[name]}</symbol>`)
142 .join("");
143 return raw(
144 `<svg class="sprite" width="0" height="0" aria-hidden="true" focusable="false">${symbols}</svg>`,
145 );
146 }
147
148 /**
149 * The same markup with every reference expanded in place — for a fragment,
150 * which lands in a document whose sprite is none of its business. A nested
151 * `<svg>` with the drawing's viewBox is exactly what `<use>` on a `<symbol>`
152 * instantiates, so the pixels are the same either way.
153 */
154 export function inlineIcons(markup: string): string {
155 return markup.replace(REFERENCE, (whole, name: string) =>
156 name in PATHS ? `<svg ${DRAW}>${PATHS[name as IconName]}</svg>` : whole,
157 );
100158 }
101159
102160 /** The half-filled circle that opens the theme menu — a glyph, not a Lucide icon. */
src/web/layout.ts +18 −8
@@ -28,7 +28,7 @@ import {
2828 } from "./flash.ts";
2929 import { prefersJson } from "./form.ts";
3030 import { html, raw, when, type Html } from "./html.ts";
31 import { icon, themeGlyph } from "./icons.ts";
31 import { icon, iconSprite, inlineIcons, themeGlyph } from "./icons.ts";
3232 import { appsOf, askCountFor } from "./studio/asks.ts";
3333 import { btn, iconBtn, kbd, menu, pill, type MenuItem } from "./ui.ts";
3434
@@ -474,6 +474,17 @@ export function page(options: PageOptions, status = 200): Response {
474474 ? `${shellJs}\n${options.scriptJs}`
475475 : shellJs;
476476
477 // The body's markup, built before the document so the icons it used can be
478 // counted: every `icon()` left a `<use>` behind, and the sprite defining
479 // them goes last, after the markup that references it (legal, and resolved
480 // once the document is parsed). Only what this page used ships.
481 const shell = html`${options.bare ? null : topbar(ctx, theme, options.crumbs)}
482 <main class="${options.bare ? "bare" : "shell"}" id="main">
483 ${flash ? flashToast(flash) : null}${options.body}
484 </main>
485 ${options.bare ? null : footer()} ${overlays()}
486 ${paletteIndex(ctx, options.palette ?? [])}`;
487
477488 const doc = html`<!doctype html>
478489 <html lang="en" ${theme ? html`data-theme="${theme}"` : null}>
479490 <head>
@@ -516,12 +527,7 @@ export function page(options: PageOptions, status = 200): Response {
516527 : null}
517528 </head>
518529 <body>
519 ${options.bare ? null : topbar(ctx, theme, options.crumbs)}
520 <main class="${options.bare ? "bare" : "shell"}" id="main">
521 ${flash ? flashToast(flash) : null}${options.body}
522 </main>
523 ${options.bare ? null : footer()} ${overlays()}
524 ${paletteIndex(ctx, options.palette ?? [])}
530 ${shell}${iconSprite(shell.text)}
525531 ${raw(`<script nonce="${nonce}">${scriptJs}</script>`)}
526532 </body>
527533 </html>`;
@@ -567,9 +573,13 @@ export function redirect(
567573 * sandbox CSP is the point: this markup is inserted into a live document, so
568574 * it is served with a policy that would make it inert if anything ever
569575 * navigated to it directly.
576 *
577 * Its icons are inlined rather than referenced: a fragment cannot count on
578 * the sprite of the document it is being dropped into, and must not add ids
579 * to it (`inlineIcons`, `icons.ts`).
570580 */
571581 export function fragment(body: Html, status = 200): Response {
572 return new Response(body.text, {
582 return new Response(inlineIcons(body.text), {
573583 status,
574584 headers: {
575585 "content-type": "text/html; charset=utf-8",
src/web/pages/thread.ts +5 −3
@@ -21,7 +21,7 @@ import { listEvents } from "../../core/events.ts";
2121 import type { Issue, Label } from "../../core/issues.ts";
2222 import { getIssue, labelsFor, listComments } from "../../core/issues.ts";
2323 import { canRead, canWrite, type Repo } from "../../core/repos.ts";
24 import type { IconName as Glyph } from "../icons.ts";
24 import { inlineIcons, type IconName as Glyph } from "../icons.ts";
2525 import {
2626 getUserById,
2727 getUserByName,
@@ -253,7 +253,9 @@ export function threadComment(
253253 /**
254254 * GET …/thread?after=<id> — the comments the page has not seen, as a bare
255255 * fragment. Served under a `sandbox` CSP with nothing else allowed, so the
256 * fragment can only ever be what it is: markup to append.
256 * fragment can only ever be what it is: markup to append. Its icons are
257 * inlined, like any fragment's: the page's sprite is not this markup's to
258 * reference (`inlineIcons`, `../icons.ts`).
257259 */
258260 export function threadFragment(ctx: Ctx, kind: "issue" | "pull"): Response {
259261 const repo = loadRepo(ctx);
@@ -273,7 +275,7 @@ export function threadFragment(ctx: Ctx, kind: "issue" | "pull"): Response {
273275 originKind: glyph,
274276 }),
275277 )}`;
276 return new Response(fragment.text, {
278 return new Response(inlineIcons(fragment.text), {
277279 headers: {
278280 "content-type": "text/html; charset=utf-8",
279281 "content-security-policy":
src/web/style/components.ts +4
@@ -16,6 +16,10 @@ export const components = /* css */ `
1616 display: inline-flex; flex: none; width: 16px; height: 16px;
1717 align-items: center; justify-content: center; vertical-align: -0.15em;
1818 }
19 /* The page's drawings, defined once each at the end of <body> (\`iconSprite\`,
20 src/web/icons.ts). A <symbol> never renders where it is defined, but the
21 <svg> holding them would take a line box, so it is taken out of flow. */
22 .sprite { position: absolute; width: 0; height: 0; overflow: hidden; }
1923
2024 /* ---- button ---- */
2125 .btn {
test/web.test.ts +173
@@ -6,6 +6,12 @@ import { createApp, type App } from "../src/app.ts";
66 import { health } from "../src/http/health.ts";
77 import { serve, type Servers } from "../src/http/server.ts";
88 import { escapeHtml, html, raw, when } from "../src/web/html.ts";
9 import {
10 icon,
11 iconSprite,
12 inlineIcons,
13 type IconName,
14 } from "../src/web/icons.ts";
915 import { renderMarkdown } from "../src/web/markdown.ts";
1016 import { THEME_NAMES } from "../src/design/themes.ts";
1117
@@ -101,6 +107,173 @@ describe("markdown", () => {
101107 });
102108 });
103109
110 describe("icons", () => {
111 test("the slot is a reference; the drawing is defined once, on the symbol", () => {
112 expect(icon("issue").text).toBe(
113 `<svg class="ico ico-issue" width="16" height="16" aria-hidden="true" ` +
114 `focusable="false"><use href="#i-issue"/></svg>`,
115 );
116 // The spinner's class is what the CSS animation reaches for, and both
117 // sizes `icon()` offers still stamp the slot they sit in.
118 expect(icon("loader").text).toContain('class="ico ico-loader"');
119 expect(icon("issue", 20).text).toContain('width="20" height="20"');
120
121 const sprite = iconSprite(icon("issue").text.repeat(4)).text;
122 expect(sprite.match(/<symbol /g)).toHaveLength(1);
123 expect(sprite).toContain(
124 `<symbol id="i-issue" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="1.75"`,
125 );
126 expect(sprite).toContain('<circle cx="12" cy="12" r="1"/>');
127 // Markup with no icons defines none at all.
128 expect(iconSprite("<p>a page of words</p>").text).toBe("");
129 });
130
131 test("a fragment carries its drawings inline, referencing no sprite", () => {
132 const inlined = inlineIcons(icon("issue").text);
133 expect(inlined).not.toContain("<use");
134 expect(inlined).not.toContain("symbol");
135 // A nested <svg> with the drawing's viewBox is what <use> instantiates.
136 expect(inlined).toContain(
137 `<svg viewBox="0 0 24 24" fill="none" stroke="currentColor"`,
138 );
139 expect(inlined).toContain('<circle cx="12" cy="12" r="1"/>');
140 expect(inlined).toContain('class="ico ico-issue"');
141 });
142
143 test("a hundred uses of twenty drawings weigh twenty drawings", () => {
144 // The census the front door showed: 98 uses, 20 distinct.
145 const census: [IconName, number][] = [
146 ["issue", 18],
147 ["check", 17],
148 ["fork", 17],
149 ["pull", 15],
150 ["rocket", 7],
151 ["repo", 5],
152 ["user", 2],
153 ["comment", 2],
154 ["clock", 2],
155 ["branch", 2],
156 ["commit", 2],
157 ["search", 1],
158 ["plus", 1],
159 ["help", 1],
160 ["signOut", 1],
161 ["users", 1],
162 ["map", 1],
163 ["settings", 1],
164 ["key", 1],
165 ["lock", 1],
166 ];
167 const markup = census
168 .map(([name, count]) => icon(name).text.repeat(count))
169 .join("");
170 const shipped = markup.length + iconSprite(markup).text.length;
171 // What the same page would weigh with a drawing per use.
172 const perUse = inlineIcons(markup).length;
173 expect(perUse - shipped).toBeGreaterThan(15_000);
174 });
175 });
176
177 describe("a page ships each drawing once", () => {
178 let dataDir: string;
179 let app: App;
180 let servers: Servers;
181 let base: string;
182 let cookie = "";
183
184 beforeAll(async () => {
185 dataDir = await mkdtemp(join(tmpdir(), "forge-icons-"));
186 app = createApp({ dataDir, port: 0 });
187 servers = await serve(app);
188 base = servers.url;
189 const signup = await fetch(`${base}/signup`, {
190 method: "POST",
191 body: new URLSearchParams({
192 username: "ada",
193 email: "[email protected]",
194 password: "analytical-engine",
195 }),
196 redirect: "manual",
197 });
198 cookie = (signup.headers.get("set-cookie") ?? "").split(";")[0]!;
199 const mode = await fetch(`${base}/mode`, {
200 method: "POST",
201 headers: { cookie, origin: base },
202 body: new URLSearchParams({ mode: "studio", back: "/" }),
203 redirect: "manual",
204 });
205 cookie = `${cookie}; ${(mode.headers.get("set-cookie") ?? "").split(";")[0]}`;
206 await fetch(`${base}/new`, {
207 method: "POST",
208 headers: { cookie, origin: base },
209 body: new URLSearchParams({ name: "engine", visibility: "public" }),
210 redirect: "manual",
211 });
212 for (let n = 0; n < 12; n++) {
213 await fetch(`${base}/ada/engine/issues/new`, {
214 method: "POST",
215 headers: { cookie, origin: base },
216 body: new URLSearchParams({ title: `bug ${n}`, body: "a body" }),
217 redirect: "manual",
218 });
219 }
220 });
221
222 afterAll(async () => {
223 servers.stop();
224 app.close();
225 await rm(dataDir, { recursive: true, force: true });
226 });
227
228 test("a list of a dozen rows sends the row's drawing once, not a dozen times", async () => {
229 const text = await (
230 await fetch(`${base}/ada/engine/issues`, { headers: { cookie } })
231 ).text();
232 expect(text.match(/<use href="#i-issue"\/>/g)!.length).toBeGreaterThan(11);
233 // The drawing itself — the issue glyph's inner dot — exactly once.
234 expect(text.match(/<circle cx="12" cy="12" r="1"\/>/g)).toHaveLength(1);
235 expect(text.match(/<symbol id="i-issue"/g)).toHaveLength(1);
236 });
237
238 test("what a page defines is exactly what it used — no more, no less", async () => {
239 for (const path of ["/", "/ada/engine/issues", "/settings"]) {
240 const text = await (
241 await fetch(`${base}${path}`, { headers: { cookie } })
242 ).text();
243 const used = new Set(
244 [...text.matchAll(/<use href="#i-([A-Za-z]+)"\/>/g)].map((m) => m[1]),
245 );
246 const defined = [...text.matchAll(/<symbol id="i-([A-Za-z]+)"/g)].map(
247 (m) => m[1],
248 );
249 expect(defined.length).toBe(used.size); // each defined once
250 expect(new Set(defined)).toEqual(used); // and nothing else shipped
251 expect(used.size).toBeGreaterThan(0);
252 }
253 // The front door is not the settings page's icons: a page with three
254 // drawings does not ship twenty.
255 const home = await (await fetch(base, { headers: { cookie } })).text();
256 expect(home).not.toContain('id="i-webhook"');
257 });
258
259 test("the thread fragment inlines its own drawings", async () => {
260 await fetch(`${base}/ada/engine/issues/1/comments`, {
261 method: "POST",
262 headers: { cookie, origin: base },
263 body: new URLSearchParams({ intent: "comment", body: "I can repro." }),
264 redirect: "manual",
265 });
266 const fragment = await (
267 await fetch(`${base}/ada/engine/issues/1/thread?after=`, {
268 headers: { cookie },
269 })
270 ).text();
271 expect(fragment).toContain("I can repro.");
272 expect(fragment).toContain('class="ico ico-comment"');
273 expect(fragment).not.toContain("<use href=");
274 });
275 });
276
104277 describe("web pages end to end", () => {
105278 let dataDir: string;
106279 let work: string;

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs