From 4eebf64aae4f000ba5238151988be262122f3da6 Mon Sep 17 00:00:00 2001 From: Mario Juarros Date: Mon, 31 Aug 2026 13:27:11 -0600 Subject: [PATCH 1/7] Fix non-square icons squashed instead of scaled to fit (issue #2108) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A custom icon whose width and height differ was stretched into a square on both the graph canvas and DOM surfaces (search results, legends), instead of being scaled down while preserving its aspect ratio. Affects raster images and SVGs alike. Root causes, all now fixed: - defaultNodeStyle forced backgroundWidth/backgroundHeight to the same 60% for every icon regardless of shape. useGraphStyles now computes per-icon percentages from the icon's real aspect ratio, falling back to 60%/60% only when dimensions are unknown. - Icon dimensions were never measured: raster icons resolved synchronously with no size info, and SVGs weren't inspected at all. iconRegistry now measures a raster's natural size via `Image`, and extracts an SVG's size from its width/height or viewBox. - iconImageUrl forced every SVG's own intrinsic width/height to a fixed 24x24 square before handing it to cytoscape. For a non-square icon this baked a mismatched-aspect letterbox into the rasterized image, which cytoscape's own aspect-aware background-width/height then stretched a second time — distorting worse than doing nothing. It now scales the intrinsic box to the icon's real aspect ratio instead. - An SVG with width/height but no viewBox has no coordinate system to scale from, so forcing a different display box just clips the content instead of scaling it. Added ensureSvgViewBox() to synthesize one when missing, applied wherever an SVG is sanitized (DOM render and canvas resolution) via a new shared SVG_ALLOWED_ATTR allowlist — DOMPurify's default SVG profile otherwise strips width/height/viewBox outright. - VertexIcon's plain `` (non-SVG raster fallback) had no `object-fit`, so the browser's default `fill` stretched it; added `object-contain`. Added an `Image` test double to setupTests.ts, since jsdom never decodes images and would otherwise hang any test that resolves a raster icon's dimensions. --- CONTEXT.md | 2 +- .../src/components/VertexIcon.tsx | 6 +- .../src/core/icons/aspectFit.test.ts | 17 ++++ .../src/core/icons/aspectFit.ts | 16 +++ .../src/core/icons/iconImageUrl.test.ts | 36 +++++++ .../src/core/icons/iconImageUrl.ts | 39 ++++++-- .../src/core/icons/iconRegistry.test.ts | 42 ++++++-- .../src/core/icons/iconRegistry.ts | 99 ++++++++++++++----- .../src/core/icons/iconSurfaces.test.tsx | 33 +++++++ .../graph-explorer/src/core/icons/index.ts | 2 + .../src/core/icons/svgViewBox.test.ts | 34 +++++++ .../src/core/icons/svgViewBox.ts | 30 ++++++ .../GraphViewer/useBackgroundImageMap.test.ts | 48 +++++++-- .../GraphViewer/useBackgroundImageMap.ts | 53 ++++++++-- .../GraphViewer/useGraphStyles.test.tsx | 5 + .../src/modules/GraphViewer/useGraphStyles.ts | 14 ++- packages/graph-explorer/src/setupTests.ts | 21 ++++ 17 files changed, 437 insertions(+), 60 deletions(-) create mode 100644 packages/graph-explorer/src/core/icons/aspectFit.test.ts create mode 100644 packages/graph-explorer/src/core/icons/aspectFit.ts create mode 100644 packages/graph-explorer/src/core/icons/svgViewBox.test.ts create mode 100644 packages/graph-explorer/src/core/icons/svgViewBox.ts diff --git a/CONTEXT.md b/CONTEXT.md index 9d42611ef..f74189512 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -54,7 +54,7 @@ The classification of a Vertex Type's configured icon into what it takes to rend _Avoid_: Icon type (ambiguous with `iconImageType`, the stored MIME string) **Icon Registry**: -The single store of resolved icons, keyed by Icon Source Id and shared by every Icon Surface. Holds a color-free artifact — a sanitized SVG string or a raster url — so applying a Vertex Type's color stays a pure transform at the point of use. A plain external store outside React/Jotai, bridged by `useSyncExternalStore`; explicitly **not** TanStack Query, because a per-hook subscription scaled with Vertex Type count and locked up the Schema View at 10k. Resolves a raster url synchronously, allows a failed icon three attempts in total, and never stores a failure as a result. See `docs/adr/20260813-icon-registry-not-react-query.md`. +The single store of resolved icons, keyed by Icon Source Id and shared by every Icon Surface. Holds a color-free artifact — a sanitized SVG string or a raster url, plus its natural width/height so a non-square icon scales instead of stretching — so applying a Vertex Type's color stays a pure transform at the point of use. A plain external store outside React/Jotai, bridged by `useSyncExternalStore`; explicitly **not** TanStack Query, because a per-hook subscription scaled with Vertex Type count and locked up the Schema View at 10k. Every icon kind resolves asynchronously (a raster's dimensions are measured by loading it), allows a failed icon three attempts in total, and never stores a failure as a result. See `docs/adr/20260813-icon-registry-not-react-query.md`. _Avoid_: Icon cache (it is the source of truth for resolution, not a layer in front of one) **Icon Surface**: diff --git a/packages/graph-explorer/src/components/VertexIcon.tsx b/packages/graph-explorer/src/components/VertexIcon.tsx index bea91d8a2..2005d193e 100644 --- a/packages/graph-explorer/src/components/VertexIcon.tsx +++ b/packages/graph-explorer/src/components/VertexIcon.tsx @@ -3,13 +3,15 @@ import { DynamicIcon } from "lucide-react/dynamic"; import SVG from "react-inlinesvg"; import { useVertexStyle, type VertexStyle, type VertexType } from "@/core"; +import { ensureSvgViewBox } from "@/core/icons"; import { cn } from "@/utils"; import { getLucideName, isValidLucideIconName } from "@/utils/lucideIcons"; function sanitizeSvg(svg: string): string { - return DOMPurify.sanitize(svg, { + const sanitized = DOMPurify.sanitize(svg, { USE_PROFILES: { svg: true, svgFilters: true }, }); + return ensureSvgViewBox(sanitized); } interface Props { @@ -54,7 +56,7 @@ function VertexIcon({ vertexStyle, className, alt }: Props) { {altText} ); diff --git a/packages/graph-explorer/src/core/icons/aspectFit.test.ts b/packages/graph-explorer/src/core/icons/aspectFit.test.ts new file mode 100644 index 000000000..da7bec25d --- /dev/null +++ b/packages/graph-explorer/src/core/icons/aspectFit.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from "vitest"; + +import { fitAspectRatio } from "./aspectFit"; + +describe("fitAspectRatio", () => { + it("caps the wider axis at base and shrinks the shorter one proportionally", () => { + expect(fitAspectRatio(400, 100, 24)).toEqual([24, 6]); + }); + + it("caps the taller axis at base and shrinks the shorter one proportionally", () => { + expect(fitAspectRatio(100, 400, 24)).toEqual([6, 24]); + }); + + it("returns the base for both axes when already square", () => { + expect(fitAspectRatio(100, 100, 24)).toEqual([24, 24]); + }); +}); diff --git a/packages/graph-explorer/src/core/icons/aspectFit.ts b/packages/graph-explorer/src/core/icons/aspectFit.ts new file mode 100644 index 000000000..a09c18581 --- /dev/null +++ b/packages/graph-explorer/src/core/icons/aspectFit.ts @@ -0,0 +1,16 @@ +/** + * Scales a width/height pair to fit a `base`-sized box, preserving aspect + * ratio: the longer axis becomes exactly `base`, the shorter one shrinks + * proportionally. Returns raw numbers — each caller formats its own unit + * (an absolute pixel size, a cytoscape percentage, ...). + */ +export function fitAspectRatio( + width: number, + height: number, + base: number, +): [width: number, height: number] { + const aspectRatio = width / height; + return aspectRatio >= 1 + ? [base, base / aspectRatio] + : [base * aspectRatio, base]; +} diff --git a/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts b/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts index 632ae8792..7652b684e 100644 --- a/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts +++ b/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts @@ -103,4 +103,40 @@ describe("toIconImageUrl", () => { expect(red).not.toBe(blue); }); + + // Issue #2108: forcing every icon's intrinsic size to a fixed 24x24 square + // bakes a mismatched-aspect letterbox into the rasterized image, which the + // consumer's own aspect-aware background-width/height then stretches a + // second time — distorting a non-square icon worse than doing nothing. + describe("non-square icons (issue #2108)", () => { + const WIDE_SVG = ``; + const TALL_SVG = ``; + + it("scales a wide icon's intrinsic width/height to its real aspect ratio", () => { + const result = toIconImageUrl( + { kind: "svg", svg: WIDE_SVG, width: 400, height: 100 }, + "#FF0000", + ); + + expect(decode(result)).toContain('width="24"'); + expect(decode(result)).toContain('height="6"'); + }); + + it("scales a tall icon's intrinsic width/height to its real aspect ratio", () => { + const result = toIconImageUrl( + { kind: "svg", svg: TALL_SVG, width: 100, height: 400 }, + "#FF0000", + ); + + expect(decode(result)).toContain('width="6"'); + expect(decode(result)).toContain('height="24"'); + }); + + it("falls back to a 24x24 square when dimensions are unknown", () => { + const result = toIconImageUrl({ kind: "svg", svg: WIDE_SVG }, "#FF0000"); + + expect(decode(result)).toContain('width="24"'); + expect(decode(result)).toContain('height="24"'); + }); + }); }); diff --git a/packages/graph-explorer/src/core/icons/iconImageUrl.ts b/packages/graph-explorer/src/core/icons/iconImageUrl.ts index 64e9f1d08..261e9c2cc 100644 --- a/packages/graph-explorer/src/core/icons/iconImageUrl.ts +++ b/packages/graph-explorer/src/core/icons/iconImageUrl.ts @@ -1,7 +1,9 @@ import type { ResolvedIcon } from "./iconRegistry"; -/** Intrinsic size; both consumers scale from it. Matches the cytoscape node size. */ -const ICON_SIZE = "24"; +import { fitAspectRatio } from "./aspectFit"; + +/** Intrinsic size baseline; matches the cytoscape node size. */ +const ICON_SIZE = 24; /** * Pure transform to an image url. @@ -16,19 +18,44 @@ export function toIconImageUrl(icon: ResolvedIcon, color: string): string { case "raster": return icon.url; case "svg": - return encodeSvg(applySizeAndColor(icon.svg, color)); + return encodeSvg( + applySizeAndColor(icon.svg, color, icon.width, icon.height), + ); } } -function applySizeAndColor(svgContent: string, color: string): string { +/** + * Sets the SVG's own intrinsic width/height. This must preserve the icon's + * real aspect ratio (scaled to fit a 24px box), not force a fixed square: + * forcing a square here bakes a mismatched-aspect letterbox into the + * rasterized image, which the consumer's own aspect-aware background-width/ + * height then stretches a second time, distorting worse than doing nothing. + */ +function applySizeAndColor( + svgContent: string, + color: string, + naturalWidth?: number, + naturalHeight?: number, +): string { const doc = new DOMParser().parseFromString(svgContent, "application/xml"); const root = doc.documentElement; - root.setAttribute("width", ICON_SIZE); - root.setAttribute("height", ICON_SIZE); + const [width, height] = fitToIconSize(naturalWidth, naturalHeight); + root.setAttribute("width", String(width)); + root.setAttribute("height", String(height)); applyColor(root, color); return new XMLSerializer().serializeToString(root); } +function fitToIconSize( + naturalWidth?: number, + naturalHeight?: number, +): [width: number, height: number] { + if (!naturalWidth || !naturalHeight) { + return [ICON_SIZE, ICON_SIZE]; + } + return fitAspectRatio(naturalWidth, naturalHeight, ICON_SIZE); +} + /** * Sets `color` on the root so `currentColor`-authored icons follow the vertex * color by inheritance; hardcoded fills are left untouched. Isolated here so diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts index 644b85777..f99145c08 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts @@ -49,7 +49,7 @@ describe("iconRegistry", () => { iconRegistry.request([source]); await settle(); - expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toStrictEqual( + expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toMatchObject( { kind: "raster", url: "https://example.test/a.png", @@ -58,18 +58,23 @@ describe("iconRegistry", () => { expect(fetch).not.toBeCalled(); }); - // A url needs no resolution, so making the consumer wait a render for it - // would be a pointless async round trip. - it("resolves a raster icon synchronously", () => { + // Measuring a raster's natural size requires loading it, so — unlike a url, + // which needs no resolution — this can no longer settle in the same tick. + it("measures a raster icon's natural dimensions", async () => { const source = classifyIconSource({ iconUrl: "https://example.test/a.png", iconImageType: "image/png", }); iconRegistry.request([source]); + await settle(); - expect(iconRegistry.getSnapshot().has(iconSourceId(source)!)).toBe(true); - expect(iconRegistry.pendingCount).toBe(0); + expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toMatchObject( + { + width: expect.any(Number), + height: expect.any(Number), + }, + ); }); it("fetches and sanitizes a remote svg", async () => { @@ -291,6 +296,31 @@ describe("sanitizes a user-supplied svg before storing it", () => { return (resolved as { kind: "svg"; svg: string }).svg; } + // The svg profile already allowlists these, so no ALLOWED_ATTR override is + // needed to keep aspect ratio. An explicit list would also be a trap: it + // cannot take effect alongside USE_PROFILES (DOMPurify rebuilds ALLOWED_ATTR + // from the profile sets), so it reads as load-bearing while doing nothing, + // and any attribute it omitted would silently vanish if the profiles were + // ever dropped. + it("preserves the geometry and path attributes an icon needs to scale", async () => { + const svg = await resolveCustomSvg( + ``, + ); + + for (const attribute of [ + "width", + "height", + "viewBox", + "preserveAspectRatio", + "d", + "stroke-width", + "transform", + "points", + ]) { + expect(svg).toContain(attribute); + } + }); + it("strips a `, diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.ts b/packages/graph-explorer/src/core/icons/iconRegistry.ts index e4ed001dc..206e72429 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.ts @@ -4,11 +4,12 @@ import { logger } from "@/utils"; import { getLucideSvgString } from "@/utils/lucideIcons"; import { type IconSource, type IconSourceId, iconSourceId } from "./iconSource"; +import { ensureSvgViewBox } from "./svgViewBox"; /** An icon resolved to a renderable form, with no color applied yet. */ export type ResolvedIcon = - | { kind: "raster"; url: string } - | { kind: "svg"; svg: string }; + | { kind: "raster"; url: string; width?: number; height?: number } + | { kind: "svg"; svg: string; width?: number; height?: number }; /** * Bounded so a permanently broken icon stops re-fetching, but not one-shot: a @@ -48,30 +49,17 @@ class IconRegistry { /** Idempotent: starts only what is neither resolved, running, nor exhausted. */ request(sources: Iterable): void { - let next: Map | undefined; - for (const source of sources) { const id = iconSourceId(source); if (id === null || this.#resolved.has(id) || this.#inFlight.has(id)) { continue; } - if (source.kind === "raster") { - // A url needs no work, so resolve it now rather than a render later. - next ??= new Map(this.#resolved); - next.set(id, { kind: "raster", url: source.url }); - continue; - } if ((this.#failures.get(id) ?? 0) >= MAX_ATTEMPTS) { continue; } this.#inFlight.add(id); void this.#resolve(id, source, this.#epoch); } - - if (next) { - this.#resolved = next; - this.#notify(); - } } /** @@ -139,29 +127,96 @@ async function resolveIconSource( switch (source.kind) { case "none": return null; - case "raster": - return { kind: "raster", url: source.url }; + case "raster": { + const dimensions = await measureImageDimensions(source.url); + return { kind: "raster", url: source.url, ...dimensions }; + } case "lucide": { - const svg = await getLucideSvgString(source.name); - if (svg === null) { + const raw = await getLucideSvgString(source.name); + if (raw === null) { logger.warn("Unknown lucide icon", source.name); return null; } - return { kind: "svg", svg }; + const svg = ensureSvgViewBox(raw); + const dimensions = extractSvgDimensions(svg); + return { kind: "svg", svg, ...dimensions }; } case "svg": { // Untrusted: a user-supplied SVG, sanitized before it is used anywhere. const response = await fetch(source.url); - const svg = DOMPurify.sanitize(await response.text(), { + const sanitized = DOMPurify.sanitize(await response.text(), { USE_PROFILES: { svg: true, svgFilters: true }, }); // A 404 body sanitizes to something that is not SVG. Reject it here so // consumers can treat `ResolvedIcon` as renderable. - return isParseableSvg(svg) ? { kind: "svg", svg } : null; + if (!isParseableSvg(sanitized)) { + return null; + } + const svg = ensureSvgViewBox(sanitized); + const dimensions = extractSvgDimensions(svg); + return { kind: "svg", svg, ...dimensions }; } } } +async function measureImageDimensions( + url: string, +): Promise<{ width?: number; height?: number }> { + try { + // Never rejects — `onerror` resolves to a fallback instead. This only + // guards a synchronous throw from constructing `Image` or setting `src`. + return await new Promise(resolve => { + const img = new Image(); + img.onload = () => { + resolve({ width: img.naturalWidth, height: img.naturalHeight }); + }; + img.onerror = () => { + resolve({}); + }; + img.src = url; + }); + } catch (_e) { + return {}; + } +} + +function extractSvgDimensions(svg: string): { + width?: number; + height?: number; +} { + try { + const doc = new DOMParser().parseFromString(svg, "application/xml"); + const root = doc.documentElement; + + if (root.localName !== "svg") { + return {}; + } + + const width = parseFloat(root.getAttribute("width") ?? ""); + const height = parseFloat(root.getAttribute("height") ?? ""); + + if (!isNaN(width) && !isNaN(height)) { + return { width, height }; + } + + const viewBox = root.getAttribute("viewBox"); + if (viewBox) { + const parts = viewBox.split(/\s+/); + if (parts.length >= 4) { + const vbWidth = parseFloat(parts[2]); + const vbHeight = parseFloat(parts[3]); + if (!isNaN(vbWidth) && !isNaN(vbHeight)) { + return { width: vbWidth, height: vbHeight }; + } + } + } + + return {}; + } catch (_e) { + return {}; + } +} + function isParseableSvg(svg: string): boolean { const doc = new DOMParser().parseFromString(svg, "application/xml"); return ( diff --git a/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx b/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx index 4db651a74..3328e50dd 100644 --- a/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx +++ b/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx @@ -73,4 +73,37 @@ describe("icon resolution across surfaces", () => { // One additional fetch for the new icon, not two for the whole set. expect(fetch).toBeCalledTimes(2); }); + + // Issue #2108: a non-square custom icon (a wide logo, say) must keep its + // aspect ratio on the canvas rather than being squashed into a square. + // The uploaded SVG has width/height but no viewBox — exactly what a plain + // `` export produces — so this also covers viewBox + // synthesis end to end, not just the aspect-ratio math in isolation. + it("computes aspect-ratio-preserving background dimensions for a wide custom icon", async () => { + vi.stubGlobal( + "fetch", + vi.fn(() => + Promise.resolve( + new Response( + ``, + ), + ), + ), + ); + + const canvas = renderHook(() => + useBackgroundImageMap([ + style({ + type: createVertexType("Wide"), + iconUrl: "https://example.test/wide-logo.svg", + }), + ]), + ); + await waitFor(() => expect(canvas.result.current.size).toBe(1)); + + const imageData = canvas.result.current.get(createVertexType("Wide"))!; + expect(imageData.width).toBe("60%"); + // 60% / (400/100) = 15%, not the 60% a square icon would get. + expect(imageData.height).toBe("15.0%"); + }); }); diff --git a/packages/graph-explorer/src/core/icons/index.ts b/packages/graph-explorer/src/core/icons/index.ts index 5308889c1..bb56ab387 100644 --- a/packages/graph-explorer/src/core/icons/index.ts +++ b/packages/graph-explorer/src/core/icons/index.ts @@ -1,4 +1,6 @@ +export * from "./aspectFit"; export * from "./iconImageUrl"; export * from "./iconRegistry"; export * from "./iconSource"; +export * from "./svgViewBox"; export * from "./useResolvedIcons"; diff --git a/packages/graph-explorer/src/core/icons/svgViewBox.test.ts b/packages/graph-explorer/src/core/icons/svgViewBox.test.ts new file mode 100644 index 000000000..ee8ee5a79 --- /dev/null +++ b/packages/graph-explorer/src/core/icons/svgViewBox.test.ts @@ -0,0 +1,34 @@ +// @vitest-environment jsdom + +// DEV NOTE: happy-dom's DOMParser is not reliable for the svg render path. + +import { describe, expect, it } from "vitest"; + +import { ensureSvgViewBox } from "./svgViewBox"; + +describe("ensureSvgViewBox", () => { + // Issue #2108: without a viewBox, an SVG has no coordinate system to scale + // from — forcing a different width/height on the root just clips the + // content instead of scaling it. + it("synthesizes a viewBox from width/height when one is missing", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toContain('viewBox="0 0 400 100"'); + }); + + it("leaves an existing viewBox untouched", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toBe(svg); + }); + + it("leaves the svg untouched when width/height are also missing", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toBe(svg); + }); + + it("leaves non-svg or unparseable input untouched", () => { + expect(ensureSvgViewBox("not xml at all <<<")).toBe("not xml at all <<<"); + }); +}); diff --git a/packages/graph-explorer/src/core/icons/svgViewBox.ts b/packages/graph-explorer/src/core/icons/svgViewBox.ts new file mode 100644 index 000000000..7843cc572 --- /dev/null +++ b/packages/graph-explorer/src/core/icons/svgViewBox.ts @@ -0,0 +1,30 @@ +/** + * Ensures an SVG has a `viewBox`, synthesizing one from `width`/`height` when + * absent. + * + * Without a `viewBox`, an SVG has no internal coordinate system to scale from: + * forcing a different CSS or attribute size on the root just clips the content + * to the new box instead of scaling it (`preserveAspectRatio` has nothing to + * map). A synthesized `viewBox="0 0 "` gives the renderer that + * mapping, so resizing scales instead of crops. + */ +export function ensureSvgViewBox(svg: string): string { + try { + const doc = new DOMParser().parseFromString(svg, "application/xml"); + const root = doc.documentElement; + if (root.localName !== "svg" || root.hasAttribute("viewBox")) { + return svg; + } + + const width = parseFloat(root.getAttribute("width") ?? ""); + const height = parseFloat(root.getAttribute("height") ?? ""); + if (isNaN(width) || isNaN(height) || width <= 0 || height <= 0) { + return svg; + } + + root.setAttribute("viewBox", `0 0 ${width} ${height}`); + return new XMLSerializer().serializeToString(root); + } catch { + return svg; + } +} diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts index f15338e94..bad779573 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts @@ -51,13 +51,43 @@ describe("useBackgroundImageMap", () => { const { result } = renderMap([config]); await waitFor(() => - expect(result.current.get(createVertexType("Raster"))).toBe( + expect(result.current.get(createVertexType("Raster"))?.url).toBe( "https://example.test/a.png", ), ); expect(fetch).not.toBeCalled(); }); + // Issue #2108: a non-square raster (not just SVG) must keep its aspect + // ratio too. setupTests.ts's global Image double always measures 24x24, so + // this overrides it for one test to prove a real wide/tall raster result. + it("computes aspect-ratio-preserving dimensions for a non-square raster", async () => { + class WideImage { + onload: (() => void) | null = null; + naturalWidth = 400; + naturalHeight = 100; + set src(_value: string) { + queueMicrotask(() => this.onload?.()); + } + } + vi.stubGlobal("Image", WideImage); + + const config = makeConfig({ + type: createVertexType("WideRaster"), + iconUrl: "https://example.test/wide.png", + iconImageType: "image/png", + }); + + const { result } = renderMap([config]); + + await waitFor(() => + expect(result.current.get(createVertexType("WideRaster"))).toMatchObject({ + width: "60%", + height: "15.0%", + }), + ); + }); + it("styles a fetched svg into a data uri", async () => { const config = makeConfig({ type: createVertexType("Svg"), @@ -71,9 +101,9 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.has(createVertexType("Svg"))).toBe(true), ); - const value = result.current.get(createVertexType("Svg"))!; - expect(value.startsWith("data:image/svg+xml;utf8,")).toBe(true); - expect(decodeURIComponent(value)).toContain("color:#FF0000"); + const imageData = result.current.get(createVertexType("Svg"))!; + expect(imageData.url.startsWith("data:image/svg+xml;utf8,")).toBe(true); + expect(decodeURIComponent(imageData.url)).toContain("color:#FF0000"); }); it("styles a lucide icon into a data uri carrying the node color", async () => { @@ -89,9 +119,9 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.has(createVertexType("Lucide"))).toBe(true), ); - const value = result.current.get(createVertexType("Lucide"))!; - expect(value.startsWith("data:image/svg+xml;utf8,")).toBe(true); - expect(decodeURIComponent(value)).toContain("color:#00FF00"); + const imageData = result.current.get(createVertexType("Lucide"))!; + expect(imageData.url.startsWith("data:image/svg+xml;utf8,")).toBe(true); + expect(decodeURIComponent(imageData.url)).toContain("color:#00FF00"); }); it("omits configs with no icon and unresolvable icons", async () => { @@ -135,10 +165,10 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.size).toBe(2)); expect( - decodeURIComponent(result.current.get(createVertexType("Red"))!), + decodeURIComponent(result.current.get(createVertexType("Red"))!.url), ).toContain("color:#FF0000"); expect( - decodeURIComponent(result.current.get(createVertexType("Blue"))!), + decodeURIComponent(result.current.get(createVertexType("Blue"))!.url), ).toContain("color:#0000FF"); // One icon identity, so one fetch — color is applied by a pure transform. expect(fetch).toBeCalledTimes(1); diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts index 1a52a156e..40a986161 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts @@ -2,15 +2,23 @@ import type { VertexStyle, VertexType } from "@/core"; import { classifyIconSource, + fitAspectRatio, type IconSource, type IconSourceId, iconSourceId, + type ResolvedIcon, toIconImageUrl, useResolvedIcons, } from "@/core/icons"; +export interface BackgroundImageData { + url: string; + width: string; + height: string; +} + /** - * Maps each vertex type to its cytoscape `background-image`. + * Maps each vertex type to its cytoscape `background-image` with aspect-ratio-aware dimensions. * * The set of UNIQUE icons is tiny (dozens) even with thousands of vertex types, * so resolution is keyed by icon identity and shared through the icon registry. @@ -18,7 +26,7 @@ import { */ export function useBackgroundImageMap( vtConfigs: VertexStyle[], -): Map { +): Map { // Single pass: this runs on every render over every vertex type, so each // config is classified once and the id is reused for both lookups below. const uniqueSources = new Map(); @@ -41,20 +49,45 @@ export function useBackgroundImageMap( const icons = useResolvedIcons([...uniqueSources.values()]); - const result = new Map(); - const rendered = new Map(); + const result = new Map(); + const rendered = new Map(); for (const { type, id, color } of identified) { const icon = icons.get(id); if (!icon) { continue; } - const renderKey = `${id}\u0000${color}`; - let backgroundImage = rendered.get(renderKey); - if (backgroundImage === undefined) { - backgroundImage = toIconImageUrl(icon, color); - rendered.set(renderKey, backgroundImage); + const renderKey = `${id}|${color}`; + let imageData = rendered.get(renderKey); + if (imageData === undefined) { + const url = toIconImageUrl(icon, color); + const { width, height } = computeAspectRatioAwareDimensions(icon); + imageData = { url, width, height }; + rendered.set(renderKey, imageData); } - result.set(type, backgroundImage); + result.set(type, imageData); } return result; } + +const BASE_PERCENT = 60; + +function computeAspectRatioAwareDimensions(icon: ResolvedIcon): { + width: string; + height: string; +} { + if (!icon.width || !icon.height) { + return { width: "60%", height: "60%" }; + } + + const [width, height] = fitAspectRatio(icon.width, icon.height, BASE_PERCENT); + return { + width: toPercent(width), + height: toPercent(height), + }; +} + +// The untouched axis stays an exact "60%" rather than "60.0%", matching the +// existing default so this is a no-op change in style output for square icons. +function toPercent(value: number): string { + return value === BASE_PERCENT ? "60%" : `${value.toFixed(1)}%`; +} diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx index 33da0ec8a..d55bc5400 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx @@ -59,6 +59,11 @@ describe("useGraphStyles", () => { "background-image": RASTER_ICON.iconUrl, "background-color": "#128EE5", "background-opacity": 0.8, + // Aspect-ratio-aware sizing (issue #2108): square by default since + // the test double measures every raster icon as 24x24. + "background-fit": "none", + "background-width": "60%", + "background-height": "60%", "border-color": "#000000", "border-width": 2, "border-opacity": 1, diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts index 869ded042..a3341ffe8 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts @@ -11,7 +11,10 @@ import { type VertexType, } from "@/core"; -import { useBackgroundImageMap } from "./useBackgroundImageMap"; +import { + useBackgroundImageMap, + type BackgroundImageData, +} from "./useBackgroundImageMap"; const LINE_PATTERN = { solid: undefined, @@ -38,19 +41,22 @@ export default function useGraphStyles() { function createGraphStyles( deferredVtConfigs: VertexStyle[], deferredEtConfigs: EdgeStyle[], - backgroundImageMap: Map, + backgroundImageMap: Map, ): GraphProps["styles"] { const styles: GraphProps["styles"] = {}; for (const vtConfig of deferredVtConfigs) { const vt = vtConfig.type; - const backgroundImage = backgroundImageMap.get(vt); + const imageData = backgroundImageMap.get(vt); styles[`node[type="${vt}"]`] = { - "background-image": backgroundImage, + "background-image": imageData?.url, "background-color": vtConfig.color, "background-opacity": vtConfig.backgroundOpacity, + "background-width": imageData?.width, + "background-height": imageData?.height, + "background-fit": "none", "border-color": vtConfig.borderColor, "border-width": vtConfig.borderWidth, "border-opacity": vtConfig.borderWidth > 0 ? 1 : 0, diff --git a/packages/graph-explorer/src/setupTests.ts b/packages/graph-explorer/src/setupTests.ts index 1e6000afd..995086a48 100644 --- a/packages/graph-explorer/src/setupTests.ts +++ b/packages/graph-explorer/src/setupTests.ts @@ -12,6 +12,24 @@ import { afterEach, vi } from "vitest"; import { iconRegistry } from "@/core/icons"; +/** + * jsdom never actually decodes images, so a real `Image` never fires + * `onload`/`onerror` — raster icon dimension measurement would hang every + * test that resolves one. This double fires `onload` on the next microtask + * with a fixed square size, matching the pre-measurement fallback so + * existing assertions about square icons stay valid. + */ +class MockImage { + onload: (() => void) | null = null; + onerror: (() => void) | null = null; + naturalWidth = 24; + naturalHeight = 24; + + set src(_value: string) { + queueMicrotask(() => this.onload?.()); + } +} + // Mock getAppStore to return a specific test store let store = createStore(); vi.mock(import("@/core/StateProvider/appStore"), () => { @@ -28,6 +46,9 @@ beforeEach(async () => { store = createStore(); vi.stubEnv("DEV", true); vi.stubEnv("PROD", false); + // Re-stubbed every test: a test file's own afterEach may call + // `vi.unstubAllGlobals()`, which would otherwise wipe this after its first test. + vi.stubGlobal("Image", MockImage); // The icon registry is a module singleton, so resolved icons would otherwise // bleed between tests. From 89faff2335b060618aeff0bd556f9c04822b963e Mon Sep 17 00:00:00 2001 From: Mario Juarros Date: Mon, 21 Sep 2026 11:35:08 -0600 Subject: [PATCH 2/7] Use a NUL separator for the icon render cache key An Icon Source Id embeds the user-supplied icon url verbatim and the vertex color is an unvalidated string, so a printable separator lets two distinct (icon, color) pairs collide and swap icons between vertex types. --- .../GraphViewer/useBackgroundImageMap.test.ts | 27 +++++++++++++++++++ .../GraphViewer/useBackgroundImageMap.ts | 4 ++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts index bad779573..bb0d51418 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts @@ -88,6 +88,33 @@ describe("useBackgroundImageMap", () => { ); }); + // The (icon, color) render cache is keyed by concatenation, so the separator + // must be a character that cannot occur in either half. An IconSourceId + // embeds the user-supplied icon url verbatim, and the color is an + // unvalidated string, so a printable separator like "|" lets two distinct + // pairs produce one key and swap icons between vertex types. + it("does not collide when a separator character appears in the icon url and color", async () => { + const shared = makeConfig({ + type: createVertexType("PipeInColor"), + iconUrl: "https://example.test/a.svg", + iconImageType: "image/svg+xml", + color: "x|#FF0000", + }); + const shifted = makeConfig({ + type: createVertexType("PipeInUrl"), + iconUrl: "https://example.test/a.svg|x", + iconImageType: "image/svg+xml", + color: "#FF0000", + }); + + const { result } = renderMap([shared, shifted]); + + await waitFor(() => expect(result.current.size).toBe(2)); + const second = result.current.get(createVertexType("PipeInUrl"))!; + expect(decodeURIComponent(second.url)).toContain("color:#FF0000"); + expect(decodeURIComponent(second.url)).not.toContain("color:x|#FF0000"); + }); + it("styles a fetched svg into a data uri", async () => { const config = makeConfig({ type: createVertexType("Svg"), diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts index 40a986161..627e683de 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts @@ -56,7 +56,9 @@ export function useBackgroundImageMap( if (!icon) { continue; } - const renderKey = `${id}|${color}`; + // NUL cannot occur in an icon url or a color, so it is the only safe + // separator: an IconSourceId embeds the user-supplied url verbatim. + const renderKey = `${id}\u0000${color}`; let imageData = rendered.get(renderKey); if (imageData === undefined) { const url = toIconImageUrl(icon, color); From 274ad01e4e49788e655e3be033dd958aee01b7e2 Mon Sep 17 00:00:00 2001 From: Mario Juarros Date: Mon, 21 Sep 2026 13:43:07 -0600 Subject: [PATCH 3/7] Fit canvas icons with a padded svg wrapper instead of measured percentages Cytoscape cannot both preserve an image's aspect ratio and inset it, so the inset is baked into a square svg wrapper and the nested image's preserveAspectRatio does the fitting. That needs no measurement, so the raster Image() probe, the async raster path, the carried dimensions, and the per-type background width/height all go away. The wrapper is applied on the canvas path only. VertexSymbolIcon already insets to 60% in its own svg coordinates, so wrapping inside toIconImageUrl would apply the inset twice. Keeps ensureSvgViewBox: without a viewBox the nested image has no intrinsic ratio to fit and fills the padded box, coming out square, which is the bug itself. --- CONTEXT.md | 4 +- .../Graph/styles/defaultNodeStyle.ts | 9 +- .../src/core/icons/aspectFit.test.ts | 17 --- .../src/core/icons/aspectFit.ts | 16 --- .../src/core/icons/iconImageUrl.test.ts | 47 +++----- .../src/core/icons/iconImageUrl.ts | 50 ++------- .../src/core/icons/iconRegistry.test.ts | 17 +-- .../src/core/icons/iconRegistry.ts | 89 ++++----------- .../src/core/icons/iconSurfaces.test.tsx | 19 ++-- .../graph-explorer/src/core/icons/index.ts | 1 - .../GraphViewer/useBackgroundImageMap.test.ts | 105 +++++++++++------- .../GraphViewer/useBackgroundImageMap.ts | 78 +++++++------ .../GraphViewer/useGraphStyles.test.tsx | 10 +- .../src/modules/GraphViewer/useGraphStyles.ts | 14 +-- packages/graph-explorer/src/setupTests.ts | 21 ---- 15 files changed, 186 insertions(+), 311 deletions(-) delete mode 100644 packages/graph-explorer/src/core/icons/aspectFit.test.ts delete mode 100644 packages/graph-explorer/src/core/icons/aspectFit.ts diff --git a/CONTEXT.md b/CONTEXT.md index f74189512..5555813e3 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -54,11 +54,11 @@ The classification of a Vertex Type's configured icon into what it takes to rend _Avoid_: Icon type (ambiguous with `iconImageType`, the stored MIME string) **Icon Registry**: -The single store of resolved icons, keyed by Icon Source Id and shared by every Icon Surface. Holds a color-free artifact — a sanitized SVG string or a raster url, plus its natural width/height so a non-square icon scales instead of stretching — so applying a Vertex Type's color stays a pure transform at the point of use. A plain external store outside React/Jotai, bridged by `useSyncExternalStore`; explicitly **not** TanStack Query, because a per-hook subscription scaled with Vertex Type count and locked up the Schema View at 10k. Every icon kind resolves asynchronously (a raster's dimensions are measured by loading it), allows a failed icon three attempts in total, and never stores a failure as a result. See `docs/adr/20260813-icon-registry-not-react-query.md`. +The single store of resolved icons, keyed by Icon Source Id and shared by every Icon Surface. Holds a color-free artifact — a sanitized SVG string or a raster url — so applying a Vertex Type's color stays a pure transform at the point of use. Every stored SVG is guaranteed to carry a `viewBox`, synthesized from `width`/`height` when the source omits one, because the surfaces fit icons by `preserveAspectRatio` and an SVG with no `viewBox` has no ratio to fit (issue #2108). A plain external store outside React/Jotai, bridged by `useSyncExternalStore`; explicitly **not** TanStack Query, because a per-hook subscription scaled with Vertex Type count and locked up the Schema View at 10k. Resolves a raster url synchronously, allows a failed icon three attempts in total, and never stores a failure as a result. See `docs/adr/20260813-icon-registry-not-react-query.md`. _Avoid_: Icon cache (it is the source of truth for resolution, not a layer in front of one) **Icon Surface**: -One of the three places an icon is drawn, which differ in how color is applied and how much they trust the markup. The **canvas** (`useBackgroundImageMap` → cytoscape `background-image`) and the **sandboxed DOM** (`VertexSymbolIcon` → ``) both render the icon as a separate image document, so CSS cannot reach it: an SVG is passed as a `data:` uri with the color baked into the markup, a raster as its plain url. **Inline DOM** (`VertexIcon`, and the lucide branch of `VertexSymbolIcon`) renders live elements that inherit `color` through `currentColor`, making recolor free. Only trusted lucide geometry is inlined by `VertexSymbolIcon`; `VertexIcon` also inlines sanitized user SVG, which predates that rule and is the known outlier. +One of the three places an icon is drawn, which differ in how color is applied and how much they trust the markup. The **canvas** (`useBackgroundImageMap` → cytoscape `background-image`) and the **sandboxed DOM** (`VertexSymbolIcon` → ``) both render the icon as a separate image document, so CSS cannot reach it: an SVG is passed as a `data:` uri with the color baked into the markup, a raster as its plain url. Both inset the icon to 60% of the node and fit it with `preserveAspectRatio`, but only `VertexSymbolIcon` can do so directly, in its own SVG coordinates; cytoscape cannot both preserve a ratio and inset, so the canvas wraps the icon in a padded square SVG and lets the nested `` fit itself (issue #2108). That wrapper is applied on the canvas path alone — adding it inside `toIconImageUrl` would inset twice on the DOM side. **Inline DOM** (`VertexIcon`, and the lucide branch of `VertexSymbolIcon`) renders live elements that inherit `color` through `currentColor`, making recolor free. Only trusted lucide geometry is inlined by `VertexSymbolIcon`; `VertexIcon` also inlines sanitized user SVG, which predates that rule and is the known outlier. _Avoid_: Icon renderer (three similarly-named components — `VertexIcon`, `VertexSymbol`, `VertexSymbolIcon` — differ by surface, so name the surface) **Neighbors**: diff --git a/packages/graph-explorer/src/components/Graph/styles/defaultNodeStyle.ts b/packages/graph-explorer/src/components/Graph/styles/defaultNodeStyle.ts index c7527f98e..60a384659 100644 --- a/packages/graph-explorer/src/components/Graph/styles/defaultNodeStyle.ts +++ b/packages/graph-explorer/src/components/Graph/styles/defaultNodeStyle.ts @@ -4,9 +4,12 @@ const defaultNodeStyle: RenderedNodeStyle = { background: "#128EE5", backgroundOpacity: 0.4, borderColor: "#128EE5", - backgroundFit: "none", - backgroundWidth: "60%", - backgroundHeight: "60%", + // The icon image is a square wrapper that already insets the artwork, so the + // node only has to fit that square. `auto` axes keep cytoscape from forcing + // both dimensions, which is what squashed non-square icons (issue #2108). + backgroundFit: "contain", + backgroundWidth: "auto", + backgroundHeight: "auto", borderWidth: 1, borderStyle: "solid", borderOpacity: 0, diff --git a/packages/graph-explorer/src/core/icons/aspectFit.test.ts b/packages/graph-explorer/src/core/icons/aspectFit.test.ts deleted file mode 100644 index da7bec25d..000000000 --- a/packages/graph-explorer/src/core/icons/aspectFit.test.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { describe, expect, it } from "vitest"; - -import { fitAspectRatio } from "./aspectFit"; - -describe("fitAspectRatio", () => { - it("caps the wider axis at base and shrinks the shorter one proportionally", () => { - expect(fitAspectRatio(400, 100, 24)).toEqual([24, 6]); - }); - - it("caps the taller axis at base and shrinks the shorter one proportionally", () => { - expect(fitAspectRatio(100, 400, 24)).toEqual([6, 24]); - }); - - it("returns the base for both axes when already square", () => { - expect(fitAspectRatio(100, 100, 24)).toEqual([24, 24]); - }); -}); diff --git a/packages/graph-explorer/src/core/icons/aspectFit.ts b/packages/graph-explorer/src/core/icons/aspectFit.ts deleted file mode 100644 index a09c18581..000000000 --- a/packages/graph-explorer/src/core/icons/aspectFit.ts +++ /dev/null @@ -1,16 +0,0 @@ -/** - * Scales a width/height pair to fit a `base`-sized box, preserving aspect - * ratio: the longer axis becomes exactly `base`, the shorter one shrinks - * proportionally. Returns raw numbers — each caller formats its own unit - * (an absolute pixel size, a cytoscape percentage, ...). - */ -export function fitAspectRatio( - width: number, - height: number, - base: number, -): [width: number, height: number] { - const aspectRatio = width / height; - return aspectRatio >= 1 - ? [base, base / aspectRatio] - : [base * aspectRatio, base]; -} diff --git a/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts b/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts index 7652b684e..864f80dbb 100644 --- a/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts +++ b/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts @@ -29,11 +29,14 @@ describe("toIconImageUrl", () => { expect(decode(result)).toContain(" { + // Sizing belongs to the consumer, which fits the icon by `preserveAspectRatio` + // against its own `viewBox`. Overriding the intrinsic size here would fight + // that, and forcing a square would reintroduce issue #2108. + it("leaves the svg's own size and viewBox alone", () => { const result = toIconImageUrl({ kind: "svg", svg: SVG }, "#FF0000"); - expect(decode(result)).toContain('width="24"'); - expect(decode(result)).toContain('height="24"'); + expect(decode(result)).toContain('viewBox="0 0 24 24"'); + expect(decode(result)).not.toContain('width="24"'); }); // The color reaches a currentColor-authored icon through CSS inheritance, so @@ -104,39 +107,21 @@ describe("toIconImageUrl", () => { expect(red).not.toBe(blue); }); - // Issue #2108: forcing every icon's intrinsic size to a fixed 24x24 square - // bakes a mismatched-aspect letterbox into the rasterized image, which the - // consumer's own aspect-aware background-width/height then stretches a - // second time — distorting a non-square icon worse than doing nothing. + // Issue #2108: sizing is the consumer's job. Both consumers place the icon + // with `preserveAspectRatio`, which fits the icon against its own `viewBox`, + // so overriding its intrinsic size here would only fight that — and forcing + // a square would bake in the very distortion the issue is about. describe("non-square icons (issue #2108)", () => { const WIDE_SVG = ``; - const TALL_SVG = ``; - it("scales a wide icon's intrinsic width/height to its real aspect ratio", () => { - const result = toIconImageUrl( - { kind: "svg", svg: WIDE_SVG, width: 400, height: 100 }, - "#FF0000", + it("leaves the icon's own geometry untouched", () => { + const result = decode( + toIconImageUrl({ kind: "svg", svg: WIDE_SVG }, "#FF0000"), ); - expect(decode(result)).toContain('width="24"'); - expect(decode(result)).toContain('height="6"'); - }); - - it("scales a tall icon's intrinsic width/height to its real aspect ratio", () => { - const result = toIconImageUrl( - { kind: "svg", svg: TALL_SVG, width: 100, height: 400 }, - "#FF0000", - ); - - expect(decode(result)).toContain('width="6"'); - expect(decode(result)).toContain('height="24"'); - }); - - it("falls back to a 24x24 square when dimensions are unknown", () => { - const result = toIconImageUrl({ kind: "svg", svg: WIDE_SVG }, "#FF0000"); - - expect(decode(result)).toContain('width="24"'); - expect(decode(result)).toContain('height="24"'); + expect(result).toContain('viewBox="0 0 400 100"'); + expect(result).not.toContain('width="24"'); + expect(result).not.toContain('height="24"'); }); }); }); diff --git a/packages/graph-explorer/src/core/icons/iconImageUrl.ts b/packages/graph-explorer/src/core/icons/iconImageUrl.ts index 261e9c2cc..39fe60f52 100644 --- a/packages/graph-explorer/src/core/icons/iconImageUrl.ts +++ b/packages/graph-explorer/src/core/icons/iconImageUrl.ts @@ -1,10 +1,5 @@ import type { ResolvedIcon } from "./iconRegistry"; -import { fitAspectRatio } from "./aspectFit"; - -/** Intrinsic size baseline; matches the cytoscape node size. */ -const ICON_SIZE = 24; - /** * Pure transform to an image url. * @@ -12,48 +7,18 @@ const ICON_SIZE = 24; * separate image document — the cytoscape `background-image`, and the `` * element used for untrusted SVG — which CSS cannot reach. Icons rendered as * live DOM inherit `color` instead and never call this. + * + * No size is applied. Both consumers place the icon with + * `preserveAspectRatio`, which needs the icon's own `viewBox` to fit against; + * overriding its intrinsic size here would only fight that. */ export function toIconImageUrl(icon: ResolvedIcon, color: string): string { switch (icon.kind) { case "raster": return icon.url; case "svg": - return encodeSvg( - applySizeAndColor(icon.svg, color, icon.width, icon.height), - ); - } -} - -/** - * Sets the SVG's own intrinsic width/height. This must preserve the icon's - * real aspect ratio (scaled to fit a 24px box), not force a fixed square: - * forcing a square here bakes a mismatched-aspect letterbox into the - * rasterized image, which the consumer's own aspect-aware background-width/ - * height then stretches a second time, distorting worse than doing nothing. - */ -function applySizeAndColor( - svgContent: string, - color: string, - naturalWidth?: number, - naturalHeight?: number, -): string { - const doc = new DOMParser().parseFromString(svgContent, "application/xml"); - const root = doc.documentElement; - const [width, height] = fitToIconSize(naturalWidth, naturalHeight); - root.setAttribute("width", String(width)); - root.setAttribute("height", String(height)); - applyColor(root, color); - return new XMLSerializer().serializeToString(root); -} - -function fitToIconSize( - naturalWidth?: number, - naturalHeight?: number, -): [width: number, height: number] { - if (!naturalWidth || !naturalHeight) { - return [ICON_SIZE, ICON_SIZE]; + return encodeSvg(applyColor(icon.svg, color)); } - return fitAspectRatio(naturalWidth, naturalHeight, ICON_SIZE); } /** @@ -61,12 +26,15 @@ function fitToIconSize( * color by inheritance; hardcoded fills are left untouched. Isolated here so * switching to "tint everything" stays a one-function change (issue #2105). */ -function applyColor(root: Element, color: string): void { +function applyColor(svgContent: string, color: string): string { + const doc = new DOMParser().parseFromString(svgContent, "application/xml"); + const root = doc.documentElement; const existing = root.getAttribute("style"); root.setAttribute( "style", existing ? `${existing};color:${color}` : `color:${color}`, ); + return new XMLSerializer().serializeToString(root); } function encodeSvg(svgContent: string): string { diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts index f99145c08..7cd23e8d9 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts @@ -49,7 +49,7 @@ describe("iconRegistry", () => { iconRegistry.request([source]); await settle(); - expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toMatchObject( + expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toStrictEqual( { kind: "raster", url: "https://example.test/a.png", @@ -58,23 +58,18 @@ describe("iconRegistry", () => { expect(fetch).not.toBeCalled(); }); - // Measuring a raster's natural size requires loading it, so — unlike a url, - // which needs no resolution — this can no longer settle in the same tick. - it("measures a raster icon's natural dimensions", async () => { + // A url needs no resolution, so making the consumer wait a render for it + // would be a pointless async round trip. + it("resolves a raster icon synchronously", () => { const source = classifyIconSource({ iconUrl: "https://example.test/a.png", iconImageType: "image/png", }); iconRegistry.request([source]); - await settle(); - expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toMatchObject( - { - width: expect.any(Number), - height: expect.any(Number), - }, - ); + expect(iconRegistry.getSnapshot().has(iconSourceId(source)!)).toBe(true); + expect(iconRegistry.pendingCount).toBe(0); }); it("fetches and sanitizes a remote svg", async () => { diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.ts b/packages/graph-explorer/src/core/icons/iconRegistry.ts index 206e72429..f24c9cd0c 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.ts @@ -8,8 +8,8 @@ import { ensureSvgViewBox } from "./svgViewBox"; /** An icon resolved to a renderable form, with no color applied yet. */ export type ResolvedIcon = - | { kind: "raster"; url: string; width?: number; height?: number } - | { kind: "svg"; svg: string; width?: number; height?: number }; + | { kind: "raster"; url: string } + | { kind: "svg"; svg: string }; /** * Bounded so a permanently broken icon stops re-fetching, but not one-shot: a @@ -49,17 +49,30 @@ class IconRegistry { /** Idempotent: starts only what is neither resolved, running, nor exhausted. */ request(sources: Iterable): void { + let next: Map | undefined; + for (const source of sources) { const id = iconSourceId(source); if (id === null || this.#resolved.has(id) || this.#inFlight.has(id)) { continue; } + if (source.kind === "raster") { + // A url needs no work, so resolve it now rather than a render later. + next ??= new Map(this.#resolved); + next.set(id, { kind: "raster", url: source.url }); + continue; + } if ((this.#failures.get(id) ?? 0) >= MAX_ATTEMPTS) { continue; } this.#inFlight.add(id); void this.#resolve(id, source, this.#epoch); } + + if (next) { + this.#resolved = next; + this.#notify(); + } } /** @@ -127,19 +140,15 @@ async function resolveIconSource( switch (source.kind) { case "none": return null; - case "raster": { - const dimensions = await measureImageDimensions(source.url); - return { kind: "raster", url: source.url, ...dimensions }; - } + case "raster": + return { kind: "raster", url: source.url }; case "lucide": { const raw = await getLucideSvgString(source.name); if (raw === null) { logger.warn("Unknown lucide icon", source.name); return null; } - const svg = ensureSvgViewBox(raw); - const dimensions = extractSvgDimensions(svg); - return { kind: "svg", svg, ...dimensions }; + return { kind: "svg", svg: ensureSvgViewBox(raw) }; } case "svg": { // Untrusted: a user-supplied SVG, sanitized before it is used anywhere. @@ -152,71 +161,11 @@ async function resolveIconSource( if (!isParseableSvg(sanitized)) { return null; } - const svg = ensureSvgViewBox(sanitized); - const dimensions = extractSvgDimensions(svg); - return { kind: "svg", svg, ...dimensions }; + return { kind: "svg", svg: ensureSvgViewBox(sanitized) }; } } } -async function measureImageDimensions( - url: string, -): Promise<{ width?: number; height?: number }> { - try { - // Never rejects — `onerror` resolves to a fallback instead. This only - // guards a synchronous throw from constructing `Image` or setting `src`. - return await new Promise(resolve => { - const img = new Image(); - img.onload = () => { - resolve({ width: img.naturalWidth, height: img.naturalHeight }); - }; - img.onerror = () => { - resolve({}); - }; - img.src = url; - }); - } catch (_e) { - return {}; - } -} - -function extractSvgDimensions(svg: string): { - width?: number; - height?: number; -} { - try { - const doc = new DOMParser().parseFromString(svg, "application/xml"); - const root = doc.documentElement; - - if (root.localName !== "svg") { - return {}; - } - - const width = parseFloat(root.getAttribute("width") ?? ""); - const height = parseFloat(root.getAttribute("height") ?? ""); - - if (!isNaN(width) && !isNaN(height)) { - return { width, height }; - } - - const viewBox = root.getAttribute("viewBox"); - if (viewBox) { - const parts = viewBox.split(/\s+/); - if (parts.length >= 4) { - const vbWidth = parseFloat(parts[2]); - const vbHeight = parseFloat(parts[3]); - if (!isNaN(vbWidth) && !isNaN(vbHeight)) { - return { width: vbWidth, height: vbHeight }; - } - } - } - - return {}; - } catch (_e) { - return {}; - } -} - function isParseableSvg(svg: string): boolean { const doc = new DOMParser().parseFromString(svg, "application/xml"); return ( diff --git a/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx b/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx index 3328e50dd..49a49bdc5 100644 --- a/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx +++ b/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx @@ -77,9 +77,10 @@ describe("icon resolution across surfaces", () => { // Issue #2108: a non-square custom icon (a wide logo, say) must keep its // aspect ratio on the canvas rather than being squashed into a square. // The uploaded SVG has width/height but no viewBox — exactly what a plain - // `` export produces — so this also covers viewBox - // synthesis end to end, not just the aspect-ratio math in isolation. - it("computes aspect-ratio-preserving background dimensions for a wide custom icon", async () => { + // `` export produces — and without a viewBox the wrapper's + // `preserveAspectRatio` has no ratio to fit and the icon fills the padded box + // square. So this covers viewBox synthesis end to end. + it("carries a synthesized viewBox through to the canvas image for a wide custom icon", async () => { vi.stubGlobal( "fetch", vi.fn(() => @@ -101,9 +102,13 @@ describe("icon resolution across surfaces", () => { ); await waitFor(() => expect(canvas.result.current.size).toBe(1)); - const imageData = canvas.result.current.get(createVertexType("Wide"))!; - expect(imageData.width).toBe("60%"); - // 60% / (400/100) = 15%, not the 60% a square icon would get. - expect(imageData.height).toBe("15.0%"); + const url = canvas.result.current.get(createVertexType("Wide"))!; + // The wrapper nests the icon as its own data uri, hence the double decode. + expect(decodeURIComponent(decodeURIComponent(url))).toContain( + 'viewBox="0 0 400 100"', + ); + expect(decodeURIComponent(url)).toContain( + 'preserveAspectRatio="xMidYMid meet"', + ); }); }); diff --git a/packages/graph-explorer/src/core/icons/index.ts b/packages/graph-explorer/src/core/icons/index.ts index bb56ab387..9b1024d54 100644 --- a/packages/graph-explorer/src/core/icons/index.ts +++ b/packages/graph-explorer/src/core/icons/index.ts @@ -1,4 +1,3 @@ -export * from "./aspectFit"; export * from "./iconImageUrl"; export * from "./iconRegistry"; export * from "./iconSource"; diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts index bb0d51418..b1dd910b9 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts @@ -24,6 +24,15 @@ function renderMap(vtConfigs: VertexStyle[]) { return renderHookWithState(() => useBackgroundImageMap(vtConfigs)); } +/** + * The wrapper nests the icon as its own data uri, so the icon's markup is + * encoded twice. Safe for these fixtures: base64 and our test svgs contain no + * literal `%`. + */ +function decodeIcon(url: string): string { + return decodeURIComponent(decodeURIComponent(url)); +} + describe("useBackgroundImageMap", () => { beforeEach(() => { iconRegistry.reset(); @@ -41,7 +50,11 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.size).toBe(0)); }); - it("passes raster icons through untouched", async () => { + // Issue #2108: cytoscape cannot both preserve an image's aspect ratio and + // inset it, so the inset is baked into a square svg wrapper and the nested + // `preserveAspectRatio` does the fitting. That works for every icon kind + // without measuring anything, so a raster is wrapped just like an svg. + it("wraps a raster icon in a padded square svg", async () => { const config = makeConfig({ type: createVertexType("Raster"), iconUrl: "https://example.test/a.png", @@ -51,41 +64,53 @@ describe("useBackgroundImageMap", () => { const { result } = renderMap([config]); await waitFor(() => - expect(result.current.get(createVertexType("Raster"))?.url).toBe( - "https://example.test/a.png", - ), + expect(result.current.has(createVertexType("Raster"))).toBe(true), ); + const url = result.current.get(createVertexType("Raster"))!; + expect(url.startsWith("data:image/svg+xml;utf8,")).toBe(true); + const wrapper = decodeURIComponent(url); + expect(wrapper).toContain('viewBox="0 0 100 100"'); + expect(wrapper).toContain('preserveAspectRatio="xMidYMid meet"'); + // 60% of the node, centred — the inset the ellipse shape needs. + expect(wrapper).toContain('x="20"'); + expect(wrapper).toContain('y="20"'); + expect(wrapper).toContain('width="60"'); + expect(wrapper).toContain('height="60"'); + expect(decodeIcon(url)).toContain("https://example.test/a.png"); expect(fetch).not.toBeCalled(); }); - // Issue #2108: a non-square raster (not just SVG) must keep its aspect - // ratio too. setupTests.ts's global Image double always measures 24x24, so - // this overrides it for one test to prove a real wide/tall raster result. - it("computes aspect-ratio-preserving dimensions for a non-square raster", async () => { - class WideImage { - onload: (() => void) | null = null; - naturalWidth = 400; - naturalHeight = 100; - set src(_value: string) { - queueMicrotask(() => this.onload?.()); - } - } - vi.stubGlobal("Image", WideImage); + // Issue #2108, the case the wrapper alone does not solve: without a viewBox + // the nested image has no intrinsic ratio for `preserveAspectRatio` to fit, + // so it fills the padded box and comes out square — the original bug. A + // synthesized viewBox is what keeps a plain `` export, + // which is exactly what many icon exporters produce, from being squashed. + it("carries a synthesized viewBox for an svg that declares only width and height", async () => { + vi.stubGlobal( + "fetch", + vi.fn(() => + Promise.resolve( + new Response( + ``, + ), + ), + ), + ); const config = makeConfig({ - type: createVertexType("WideRaster"), - iconUrl: "https://example.test/wide.png", - iconImageType: "image/png", + type: createVertexType("NoViewBox"), + iconUrl: "https://example.test/wide.svg", + iconImageType: "image/svg+xml", }); const { result } = renderMap([config]); await waitFor(() => - expect(result.current.get(createVertexType("WideRaster"))).toMatchObject({ - width: "60%", - height: "15.0%", - }), + expect(result.current.has(createVertexType("NoViewBox"))).toBe(true), ); + expect( + decodeIcon(result.current.get(createVertexType("NoViewBox"))!), + ).toContain('viewBox="0 0 400 100"'); }); // The (icon, color) render cache is keyed by concatenation, so the separator @@ -110,9 +135,11 @@ describe("useBackgroundImageMap", () => { const { result } = renderMap([shared, shifted]); await waitFor(() => expect(result.current.size).toBe(2)); - const second = result.current.get(createVertexType("PipeInUrl"))!; - expect(decodeURIComponent(second.url)).toContain("color:#FF0000"); - expect(decodeURIComponent(second.url)).not.toContain("color:x|#FF0000"); + const second = decodeIcon( + result.current.get(createVertexType("PipeInUrl"))!, + ); + expect(second).toContain("color:#FF0000"); + expect(second).not.toContain("color:x|#FF0000"); }); it("styles a fetched svg into a data uri", async () => { @@ -128,9 +155,9 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.has(createVertexType("Svg"))).toBe(true), ); - const imageData = result.current.get(createVertexType("Svg"))!; - expect(imageData.url.startsWith("data:image/svg+xml;utf8,")).toBe(true); - expect(decodeURIComponent(imageData.url)).toContain("color:#FF0000"); + const url = result.current.get(createVertexType("Svg"))!; + expect(url.startsWith("data:image/svg+xml;utf8,")).toBe(true); + expect(decodeIcon(url)).toContain("color:#FF0000"); }); it("styles a lucide icon into a data uri carrying the node color", async () => { @@ -146,9 +173,9 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.has(createVertexType("Lucide"))).toBe(true), ); - const imageData = result.current.get(createVertexType("Lucide"))!; - expect(imageData.url.startsWith("data:image/svg+xml;utf8,")).toBe(true); - expect(decodeURIComponent(imageData.url)).toContain("color:#00FF00"); + const url = result.current.get(createVertexType("Lucide"))!; + expect(url.startsWith("data:image/svg+xml;utf8,")).toBe(true); + expect(decodeIcon(url)).toContain("color:#00FF00"); }); it("omits configs with no icon and unresolvable icons", async () => { @@ -191,12 +218,12 @@ describe("useBackgroundImageMap", () => { const { result } = renderMap([red, blue]); await waitFor(() => expect(result.current.size).toBe(2)); - expect( - decodeURIComponent(result.current.get(createVertexType("Red"))!.url), - ).toContain("color:#FF0000"); - expect( - decodeURIComponent(result.current.get(createVertexType("Blue"))!.url), - ).toContain("color:#0000FF"); + expect(decodeIcon(result.current.get(createVertexType("Red"))!)).toContain( + "color:#FF0000", + ); + expect(decodeIcon(result.current.get(createVertexType("Blue"))!)).toContain( + "color:#0000FF", + ); // One icon identity, so one fetch — color is applied by a pure transform. expect(fetch).toBeCalledTimes(1); }); diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts index 627e683de..a8f3607b4 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts @@ -2,23 +2,15 @@ import type { VertexStyle, VertexType } from "@/core"; import { classifyIconSource, - fitAspectRatio, type IconSource, type IconSourceId, iconSourceId, - type ResolvedIcon, toIconImageUrl, useResolvedIcons, } from "@/core/icons"; -export interface BackgroundImageData { - url: string; - width: string; - height: string; -} - /** - * Maps each vertex type to its cytoscape `background-image` with aspect-ratio-aware dimensions. + * Maps each vertex type to its cytoscape `background-image`. * * The set of UNIQUE icons is tiny (dozens) even with thousands of vertex types, * so resolution is keyed by icon identity and shared through the icon registry. @@ -26,7 +18,7 @@ export interface BackgroundImageData { */ export function useBackgroundImageMap( vtConfigs: VertexStyle[], -): Map { +): Map { // Single pass: this runs on every render over every vertex type, so each // config is classified once and the id is reused for both lookups below. const uniqueSources = new Map(); @@ -49,8 +41,8 @@ export function useBackgroundImageMap( const icons = useResolvedIcons([...uniqueSources.values()]); - const result = new Map(); - const rendered = new Map(); + const result = new Map(); + const rendered = new Map(); for (const { type, id, color } of identified) { const icon = icons.get(id); if (!icon) { @@ -59,37 +51,51 @@ export function useBackgroundImageMap( // NUL cannot occur in an icon url or a color, so it is the only safe // separator: an IconSourceId embeds the user-supplied url verbatim. const renderKey = `${id}\u0000${color}`; - let imageData = rendered.get(renderKey); - if (imageData === undefined) { - const url = toIconImageUrl(icon, color); - const { width, height } = computeAspectRatioAwareDimensions(icon); - imageData = { url, width, height }; - rendered.set(renderKey, imageData); + let backgroundImage = rendered.get(renderKey); + if (backgroundImage === undefined) { + backgroundImage = insetIconImage(toIconImageUrl(icon, color)); + rendered.set(renderKey, backgroundImage); } - result.set(type, imageData); + result.set(type, backgroundImage); } return result; } -const BASE_PERCENT = 60; +/** Fraction of the node the icon occupies, leaving room for the shape's curve. */ +const ICON_RATIO = 0.6; +/** Arbitrary wrapper viewport; only the ratio of inset to box matters. */ +const BOX = 100; -function computeAspectRatioAwareDimensions(icon: ResolvedIcon): { - width: string; - height: string; -} { - if (!icon.width || !icon.height) { - return { width: "60%", height: "60%" }; - } +/** + * Centers an icon at {@link ICON_RATIO} of a square canvas, preserving its + * aspect ratio. + * + * Cytoscape cannot do both parts itself: `background-fit: contain` keeps the + * ratio but fills the whole node box, and the node is an ellipse, so a + * square-ish icon's corners spill outside the shape. Setting explicit + * percentages insets the icon but forces both axes, which is what squashed + * non-square icons (issue #2108). Baking the inset into a square svg leaves + * cytoscape a square to fit, and delegates the ratio to the nested image's + * `preserveAspectRatio`. + * + * The nested icon must carry a `viewBox`, or it has no intrinsic ratio to fit + * and fills the padded box — square again. The icon registry guarantees one. + */ +function insetIconImage(iconUrl: string): string { + const size = BOX * ICON_RATIO; + const offset = (BOX - size) / 2; + return encodeSvg( + `` + + `` + + ``, + ); +} - const [width, height] = fitAspectRatio(icon.width, icon.height, BASE_PERCENT); - return { - width: toPercent(width), - height: toPercent(height), - }; +/** The url becomes an XML attribute value, so `&` and `"` must not break it. */ +function escapeXmlAttribute(value: string): string { + return value.replaceAll("&", "&").replaceAll('"', """); } -// The untouched axis stays an exact "60%" rather than "60.0%", matching the -// existing default so this is a no-op change in style output for square icons. -function toPercent(value: number): string { - return value === BASE_PERCENT ? "60%" : `${value.toFixed(1)}%`; +function encodeSvg(svgContent: string): string { + return "data:image/svg+xml;utf8," + encodeURIComponent(svgContent); } diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx index d55bc5400..eb3ccd7ba 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx @@ -56,14 +56,12 @@ describe("useGraphStyles", () => { await waitFor(() => { const vertexStyle = getStyles(result)[`node[type="Person"]`] as any; expect(vertexStyle).toEqual({ - "background-image": RASTER_ICON.iconUrl, + // The raster is wrapped in a padded square svg so the canvas can fit it + // without measuring (issue #2108); sizing now comes from the node + // defaults, not per type. + "background-image": expect.stringContaining("data:image/svg+xml;utf8,"), "background-color": "#128EE5", "background-opacity": 0.8, - // Aspect-ratio-aware sizing (issue #2108): square by default since - // the test double measures every raster icon as 24x24. - "background-fit": "none", - "background-width": "60%", - "background-height": "60%", "border-color": "#000000", "border-width": 2, "border-opacity": 1, diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts index a3341ffe8..869ded042 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts @@ -11,10 +11,7 @@ import { type VertexType, } from "@/core"; -import { - useBackgroundImageMap, - type BackgroundImageData, -} from "./useBackgroundImageMap"; +import { useBackgroundImageMap } from "./useBackgroundImageMap"; const LINE_PATTERN = { solid: undefined, @@ -41,22 +38,19 @@ export default function useGraphStyles() { function createGraphStyles( deferredVtConfigs: VertexStyle[], deferredEtConfigs: EdgeStyle[], - backgroundImageMap: Map, + backgroundImageMap: Map, ): GraphProps["styles"] { const styles: GraphProps["styles"] = {}; for (const vtConfig of deferredVtConfigs) { const vt = vtConfig.type; - const imageData = backgroundImageMap.get(vt); + const backgroundImage = backgroundImageMap.get(vt); styles[`node[type="${vt}"]`] = { - "background-image": imageData?.url, + "background-image": backgroundImage, "background-color": vtConfig.color, "background-opacity": vtConfig.backgroundOpacity, - "background-width": imageData?.width, - "background-height": imageData?.height, - "background-fit": "none", "border-color": vtConfig.borderColor, "border-width": vtConfig.borderWidth, "border-opacity": vtConfig.borderWidth > 0 ? 1 : 0, diff --git a/packages/graph-explorer/src/setupTests.ts b/packages/graph-explorer/src/setupTests.ts index 995086a48..1e6000afd 100644 --- a/packages/graph-explorer/src/setupTests.ts +++ b/packages/graph-explorer/src/setupTests.ts @@ -12,24 +12,6 @@ import { afterEach, vi } from "vitest"; import { iconRegistry } from "@/core/icons"; -/** - * jsdom never actually decodes images, so a real `Image` never fires - * `onload`/`onerror` — raster icon dimension measurement would hang every - * test that resolves one. This double fires `onload` on the next microtask - * with a fixed square size, matching the pre-measurement fallback so - * existing assertions about square icons stay valid. - */ -class MockImage { - onload: (() => void) | null = null; - onerror: (() => void) | null = null; - naturalWidth = 24; - naturalHeight = 24; - - set src(_value: string) { - queueMicrotask(() => this.onload?.()); - } -} - // Mock getAppStore to return a specific test store let store = createStore(); vi.mock(import("@/core/StateProvider/appStore"), () => { @@ -46,9 +28,6 @@ beforeEach(async () => { store = createStore(); vi.stubEnv("DEV", true); vi.stubEnv("PROD", false); - // Re-stubbed every test: a test file's own afterEach may call - // `vi.unstubAllGlobals()`, which would otherwise wipe this after its first test. - vi.stubGlobal("Image", MockImage); // The icon registry is a module singleton, so resolved icons would otherwise // bleed between tests. From ff54a327f651c4aa91743c22e026f05b7ac42ebd Mon Sep 17 00:00:00 2001 From: Mario Juarros Date: Wed, 23 Sep 2026 13:57:16 -0600 Subject: [PATCH 4/7] Guard the icon wrapper against a malformed url, and share its geometry insetIconImage's encodeURIComponent throws URIError on a url that is not well-formed UTF-16 (a lone surrogate). That runs during style computation, so an uncaught throw took down the whole app through the route-level error boundary, with no in-app way back to fix the stored value. Skip the icon and log a warning instead; the vertex renders with no background image. Pulled ICON_BOX, ICON_RATIO, and encodeSvg into a shared iconGeometry module so useBackgroundImageMap and VertexSymbol read one definition instead of two that could silently drift apart. Adopts 96 (VertexSymbol's already-reasoned value, 4x a canvas node) over the wrapper's previous arbitrary 100. Also adds a tall-icon (1:4, no viewBox) companion to the existing wide one, since only one axis was covered. --- .../components/VertexSymbol/VertexSymbol.tsx | 19 +++--- .../src/core/icons/iconGeometry.test.ts | 23 +++++++ .../src/core/icons/iconGeometry.ts | 19 ++++++ .../src/core/icons/iconImageUrl.ts | 12 ++-- .../GraphViewer/useBackgroundImageMap.test.ts | 68 +++++++++++++++++-- .../GraphViewer/useBackgroundImageMap.ts | 36 ++++++---- 6 files changed, 143 insertions(+), 34 deletions(-) create mode 100644 packages/graph-explorer/src/core/icons/iconGeometry.test.ts create mode 100644 packages/graph-explorer/src/core/icons/iconGeometry.ts diff --git a/packages/graph-explorer/src/components/VertexSymbol/VertexSymbol.tsx b/packages/graph-explorer/src/components/VertexSymbol/VertexSymbol.tsx index 02f4c87bb..c7f2ac04f 100644 --- a/packages/graph-explorer/src/components/VertexSymbol/VertexSymbol.tsx +++ b/packages/graph-explorer/src/components/VertexSymbol/VertexSymbol.tsx @@ -1,21 +1,20 @@ import { useId } from "react"; import { useVertexStyle, type VertexStyle, type VertexType } from "@/core"; +import { ICON_BOX, ICON_RATIO } from "@/core/icons/iconGeometry"; import { cn } from "@/utils"; import { resolveShapeGeometry } from "./nodeShapes"; import { VertexSymbolIcon } from "./VertexSymbolIcon"; -const VIEWBOX = 96; -const ICON_RATIO = 0.6; const CANVAS_NODE_SIZE = 24; /** * How much larger the preview draws things than the graph canvas: the ratio of - * the SVG viewBox (96) to a canvas node's size in cytoscape units (24). Applied - * to any canvas-unit length (border width, label font/padding) to render it at - * preview size. + * the SVG viewBox ({@link ICON_BOX}) to a canvas node's size in cytoscape units + * (24). Applied to any canvas-unit length (border width, label font/padding) + * to render it at preview size. */ -export const PREVIEW_SCALE = VIEWBOX / CANVAS_NODE_SIZE; +export const PREVIEW_SCALE = ICON_BOX / CANVAS_NODE_SIZE; interface Props { vertexStyle: VertexStyle; @@ -26,11 +25,11 @@ export function VertexSymbol({ vertexStyle, className }: Props) { // SVG url(#...) references reject the colons in React's raw useId format. const clipId = `vs-${useId().replace(/:/g, "")}`; const strokeWidth = vertexStyle.borderWidth * PREVIEW_SCALE; - const insetSize = Math.max(1, VIEWBOX - strokeWidth * 2); + const insetSize = Math.max(1, ICON_BOX - strokeWidth * 2); const geometry = resolveShapeGeometry(vertexStyle.shape, insetSize); - const iconSize = VIEWBOX * ICON_RATIO; - const iconOffset = (VIEWBOX - iconSize) / 2; + const iconSize = ICON_BOX * ICON_RATIO; + const iconOffset = (ICON_BOX - iconSize) / 2; // The shape is rendered twice: once filled/stroked, once as the icon's // clipPath. clipPath children must be shape elements directly — a wrapping @@ -41,7 +40,7 @@ export function VertexSymbol({ vertexStyle, className }: Props) { return ( { + // ICON_BOX and ICON_RATIO are the single source of truth for how much of the + // icon's square box the artwork occupies. The canvas (useBackgroundImageMap) + // and the style preview (VertexSymbol) both import them rather than + // declaring their own copy — this pins the values themselves, so a future + // edit to one file cannot silently drift from the other without also + // changing this test. + it("insets the icon to 60% of a box that is 4x a canvas node (24 units)", () => { + expect(ICON_RATIO).toBe(0.6); + expect(ICON_BOX).toBe(96); + expect(ICON_BOX / 24).toBe(4); + }); + + it("percent-encodes svg markup as a data uri", () => { + expect(encodeSvg("&")).toBe( + "data:image/svg+xml;utf8," + encodeURIComponent("&"), + ); + }); +}); diff --git a/packages/graph-explorer/src/core/icons/iconGeometry.ts b/packages/graph-explorer/src/core/icons/iconGeometry.ts new file mode 100644 index 000000000..2d8797503 --- /dev/null +++ b/packages/graph-explorer/src/core/icons/iconGeometry.ts @@ -0,0 +1,19 @@ +/** + * Shared inset geometry for every icon surface. The canvas + * (`useBackgroundImageMap`) and the style preview (`VertexSymbol`) each fit an + * icon into a square box by `preserveAspectRatio`, and both must agree on how + * much of that box the icon occupies — changing one without the other would + * silently desync what the preview shows from what the canvas renders. + * + * {@link ICON_BOX} is not arbitrary: it is exactly 4x a canvas node's size in + * cytoscape units (24), the same ratio `VertexSymbol`'s own viewBox already + * uses for everything else it scales (border width, label font/padding). + */ +export const ICON_BOX = 96; + +/** Fraction of {@link ICON_BOX} the icon occupies, leaving room for the shape's curve. */ +export const ICON_RATIO = 0.6; + +export function encodeSvg(svgContent: string): string { + return "data:image/svg+xml;utf8," + encodeURIComponent(svgContent); +} diff --git a/packages/graph-explorer/src/core/icons/iconImageUrl.ts b/packages/graph-explorer/src/core/icons/iconImageUrl.ts index 39fe60f52..9aba4b5b9 100644 --- a/packages/graph-explorer/src/core/icons/iconImageUrl.ts +++ b/packages/graph-explorer/src/core/icons/iconImageUrl.ts @@ -1,5 +1,7 @@ import type { ResolvedIcon } from "./iconRegistry"; +import { encodeSvg } from "./iconGeometry"; + /** * Pure transform to an image url. * @@ -11,6 +13,12 @@ import type { ResolvedIcon } from "./iconRegistry"; * No size is applied. Both consumers place the icon with * `preserveAspectRatio`, which needs the icon's own `viewBox` to fit against; * overriding its intrinsic size here would only fight that. + * + * Do not wrap the result in another inset SVG here: `VertexSymbolIcon` already + * insets to 60% in its own SVG coordinates, so this stays a single fit for + * every caller. Only the canvas path (`useBackgroundImageMap`) needs its own + * wrapper, because cytoscape — unlike an inline SVG — cannot fit an image by + * `preserveAspectRatio` itself. */ export function toIconImageUrl(icon: ResolvedIcon, color: string): string { switch (icon.kind) { @@ -36,7 +44,3 @@ function applyColor(svgContent: string, color: string): string { ); return new XMLSerializer().serializeToString(root); } - -function encodeSvg(svgContent: string): string { - return "data:image/svg+xml;utf8," + encodeURIComponent(svgContent); -} diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts index b1dd910b9..f64edc58c 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts @@ -9,6 +9,7 @@ import type { VertexStyle } from "@/core"; import { createVertexType } from "@/core/entities/vertex"; import { iconRegistry } from "@/core/icons"; +import { ICON_BOX, ICON_RATIO } from "@/core/icons/iconGeometry"; import { createRandomVertexStyle, renderHookWithState } from "@/utils/testing"; import { useBackgroundImageMap } from "./useBackgroundImageMap"; @@ -50,6 +51,27 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.size).toBe(0)); }); + // A stored icon url is not guaranteed to be well-formed UTF-16 (a lone + // surrogate, say). `encodeURIComponent` throws `URIError` on one, and this + // hook runs during style computation, so an uncaught throw here takes down + // the whole app through the route-level error boundary with no in-app way + // back to fix the value. The vertex must render with no background image + // instead. + it("omits an icon whose url is not well-formed UTF-16 instead of throwing", async () => { + const config = makeConfig({ + type: createVertexType("Malformed"), + iconUrl: "data:image/png;base64,AAA\uD800BBB", + iconImageType: "image/png", + }); + + let result: ReturnType["result"]; + expect(() => { + ({ result } = renderMap([config])); + }).not.toThrow(); + + await waitFor(() => expect(result!.current.size).toBe(0)); + }); + // Issue #2108: cytoscape cannot both preserve an image's aspect ratio and // inset it, so the inset is baked into a square svg wrapper and the nested // `preserveAspectRatio` does the fitting. That works for every icon kind @@ -69,13 +91,17 @@ describe("useBackgroundImageMap", () => { const url = result.current.get(createVertexType("Raster"))!; expect(url.startsWith("data:image/svg+xml;utf8,")).toBe(true); const wrapper = decodeURIComponent(url); - expect(wrapper).toContain('viewBox="0 0 100 100"'); + expect(wrapper).toContain(`viewBox="0 0 ${ICON_BOX} ${ICON_BOX}"`); expect(wrapper).toContain('preserveAspectRatio="xMidYMid meet"'); - // 60% of the node, centred — the inset the ellipse shape needs. - expect(wrapper).toContain('x="20"'); - expect(wrapper).toContain('y="20"'); - expect(wrapper).toContain('width="60"'); - expect(wrapper).toContain('height="60"'); + // Inset the icon needs for the ellipse shape, computed from the same + // constants VertexSymbol's preview box uses, so this also guards the two + // staying in sync. + const size = ICON_BOX * ICON_RATIO; + const offset = (ICON_BOX - size) / 2; + expect(wrapper).toContain(`x="${offset}"`); + expect(wrapper).toContain(`y="${offset}"`); + expect(wrapper).toContain(`width="${size}"`); + expect(wrapper).toContain(`height="${size}"`); expect(decodeIcon(url)).toContain("https://example.test/a.png"); expect(fetch).not.toBeCalled(); }); @@ -113,6 +139,36 @@ describe("useBackgroundImageMap", () => { ).toContain('viewBox="0 0 400 100"'); }); + // Same fix, the other axis: a tall icon must synthesize a viewBox that + // keeps height as the dominant dimension, not just width. + it("carries a synthesized viewBox for a tall svg that declares only width and height", async () => { + vi.stubGlobal( + "fetch", + vi.fn(() => + Promise.resolve( + new Response( + ``, + ), + ), + ), + ); + + const config = makeConfig({ + type: createVertexType("TallNoViewBox"), + iconUrl: "https://example.test/tall.svg", + iconImageType: "image/svg+xml", + }); + + const { result } = renderMap([config]); + + await waitFor(() => + expect(result.current.has(createVertexType("TallNoViewBox"))).toBe(true), + ); + expect( + decodeIcon(result.current.get(createVertexType("TallNoViewBox"))!), + ).toContain('viewBox="0 0 100 400"'); + }); + // The (icon, color) render cache is keyed by concatenation, so the separator // must be a character that cannot occur in either half. An IconSourceId // embeds the user-supplied icon url verbatim, and the color is an diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts index a8f3607b4..8e1d750b3 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts @@ -8,6 +8,8 @@ import { toIconImageUrl, useResolvedIcons, } from "@/core/icons"; +import { encodeSvg, ICON_BOX, ICON_RATIO } from "@/core/icons/iconGeometry"; +import { logger } from "@/utils"; /** * Maps each vertex type to its cytoscape `background-image`. @@ -53,7 +55,11 @@ export function useBackgroundImageMap( const renderKey = `${id}\u0000${color}`; let backgroundImage = rendered.get(renderKey); if (backgroundImage === undefined) { - backgroundImage = insetIconImage(toIconImageUrl(icon, color)); + const wrapped = insetIconImage(toIconImageUrl(icon, color)); + if (wrapped === null) { + continue; + } + backgroundImage = wrapped; rendered.set(renderKey, backgroundImage); } result.set(type, backgroundImage); @@ -61,11 +67,6 @@ export function useBackgroundImageMap( return result; } -/** Fraction of the node the icon occupies, leaving room for the shape's curve. */ -const ICON_RATIO = 0.6; -/** Arbitrary wrapper viewport; only the ratio of inset to box matters. */ -const BOX = 100; - /** * Centers an icon at {@link ICON_RATIO} of a square canvas, preserving its * aspect ratio. @@ -80,12 +81,23 @@ const BOX = 100; * * The nested icon must carry a `viewBox`, or it has no intrinsic ratio to fit * and fills the padded box — square again. The icon registry guarantees one. + * + * Returns `null`, rather than throwing, for a url that is not well-formed + * UTF-16 (e.g. a stored value containing a lone surrogate): `encodeURIComponent` + * throws `URIError` on one, and this runs during style computation, so an + * uncaught throw here takes down the whole app through the route-level error + * boundary with no in-app way back. The vertex renders with no background + * image instead. */ -function insetIconImage(iconUrl: string): string { - const size = BOX * ICON_RATIO; - const offset = (BOX - size) / 2; +function insetIconImage(iconUrl: string): string | null { + if (!iconUrl.isWellFormed()) { + logger.warn("Icon url is not well-formed, skipping", iconUrl); + return null; + } + const size = ICON_BOX * ICON_RATIO; + const offset = (ICON_BOX - size) / 2; return encodeSvg( - `` + + `` + `` + ``, ); @@ -95,7 +107,3 @@ function insetIconImage(iconUrl: string): string { function escapeXmlAttribute(value: string): string { return value.replaceAll("&", "&").replaceAll('"', """); } - -function encodeSvg(svgContent: string): string { - return "data:image/svg+xml;utf8," + encodeURIComponent(svgContent); -} From 8b94b33831100d3e266a26f77c2e686c69d253d3 Mon Sep 17 00:00:00 2001 From: Mario Juarros Date: Wed, 23 Sep 2026 13:57:27 -0600 Subject: [PATCH 5/7] Fix ensureSvgViewBox's numeric guard and remove its unreachable catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseFloat let two malformed inputs through: width="1e400" parsed to Infinity, synthesizing a viewBox that blanks the icon, and width="100%" parsed to the unitless number 100, synthesizing a viewBox that crops the artwork instead of scaling it. Switched to a helper that rejects a trailing % and any non-finite result via Number.isFinite; a plain unit suffix like 400px still parses. Removed the try/catch: DOMParser with application/xml never throws on malformed input, it returns a document whose root is wrapping a parsererror, or whichever mismatched tag the input happened to close. Either way root.localName is not "svg", so the existing guard already rejects it — confirmed with a second unparseable-input case and a comment explaining why it passes. --- .../src/core/icons/svgViewBox.test.ts | 29 ++++++++++++ .../src/core/icons/svgViewBox.ts | 47 +++++++++++++------ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/packages/graph-explorer/src/core/icons/svgViewBox.test.ts b/packages/graph-explorer/src/core/icons/svgViewBox.test.ts index ee8ee5a79..6bb1618c8 100644 --- a/packages/graph-explorer/src/core/icons/svgViewBox.test.ts +++ b/packages/graph-explorer/src/core/icons/svgViewBox.test.ts @@ -28,7 +28,36 @@ describe("ensureSvgViewBox", () => { expect(ensureSvgViewBox(svg)).toBe(svg); }); + // Not a try/catch case: DOMParser with "application/xml" never throws, it + // returns a document whose root is (wrapping a ) or + // whatever mismatched tag the input actually closed. Either way the root's + // localName is not "svg", so the ordinary guard above rejects it. it("leaves non-svg or unparseable input untouched", () => { expect(ensureSvgViewBox("not xml at all <<<")).toBe("not xml at all <<<"); + expect(ensureSvgViewBox("")).toBe(""); + }); + + // A non-finite width/height would synthesize viewBox="0 0 Infinity + // Infinity", which blanks the icon instead of scaling it. + it("leaves the svg untouched when width overflows to Infinity", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toBe(svg); + }); + + // A percentage has no meaning without a viewport to resolve against, so + // parseFloat's unitless "100" from "100%" would produce a bogus viewBox + // that crops the artwork instead of scaling it. + it("leaves the svg untouched when width/height are percentages", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toBe(svg); + }); + + // A plain unit suffix is legitimate SVG and must still synthesize a viewBox. + it("synthesizes a viewBox when width/height carry a px suffix", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toContain('viewBox="0 0 400 100"'); }); }); diff --git a/packages/graph-explorer/src/core/icons/svgViewBox.ts b/packages/graph-explorer/src/core/icons/svgViewBox.ts index 7843cc572..c74865abc 100644 --- a/packages/graph-explorer/src/core/icons/svgViewBox.ts +++ b/packages/graph-explorer/src/core/icons/svgViewBox.ts @@ -9,22 +9,39 @@ * mapping, so resizing scales instead of crops. */ export function ensureSvgViewBox(svg: string): string { - try { - const doc = new DOMParser().parseFromString(svg, "application/xml"); - const root = doc.documentElement; - if (root.localName !== "svg" || root.hasAttribute("viewBox")) { - return svg; - } - - const width = parseFloat(root.getAttribute("width") ?? ""); - const height = parseFloat(root.getAttribute("height") ?? ""); - if (isNaN(width) || isNaN(height) || width <= 0 || height <= 0) { - return svg; - } + const doc = new DOMParser().parseFromString(svg, "application/xml"); + const root = doc.documentElement; + if (root.localName !== "svg" || root.hasAttribute("viewBox")) { + return svg; + } - root.setAttribute("viewBox", `0 0 ${width} ${height}`); - return new XMLSerializer().serializeToString(root); - } catch { + const width = parseFiniteLength(root.getAttribute("width")); + const height = parseFiniteLength(root.getAttribute("height")); + if (width === null || height === null) { return svg; } + + root.setAttribute("viewBox", `0 0 ${width} ${height}`); + return new XMLSerializer().serializeToString(root); +} + +/** + * Parses an SVG `width`/`height` attribute as a positive, finite number of + * user units, or `null` if it is not one. + * + * `parseFloat` alone lets three malformed inputs through: `NaN`/`-5`/`0` are + * already guarded, but `"1e400"` parses to `Infinity` (a `viewBox="0 0 + * Infinity Infinity"` that blanks the icon) and `"100%"` parses to the + * unitless number `100` (a bogus viewBox that crops the artwork instead of + * scaling it, since a percentage has no meaning without a viewport to + * resolve against). A plain unit suffix like `"400px"` is legitimate SVG and + * must still parse, so only a trailing `%` is rejected, not every non-digit + * suffix. + */ +function parseFiniteLength(value: string | null): number | null { + if (value === null || value.trimEnd().endsWith("%")) { + return null; + } + const parsed = parseFloat(value); + return Number.isFinite(parsed) && parsed > 0 ? parsed : null; } From 4268ca6653d936bf81f0a93723de1a8fe0c9ebc9 Mon Sep 17 00:00:00 2001 From: Mario Juarros Date: Wed, 23 Sep 2026 13:57:43 -0600 Subject: [PATCH 6/7] Strengthen two vacuous assertions and add VertexIcon.test.tsx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit iconRegistry.test.ts's geometry test used toContain(attributeName), which a substring of an unrelated attribute already satisfies: preserveAspectRatio contains "d", and stroke-width contains "width". Asserts full attribute values with distinct numbers per attribute instead, so a wrong or missing value actually fails it. useGraphStyles.test.tsx's raster assertion only checked the data:image/svg+xml prefix, which passes even if the wrapper lost the url or wrapped the wrong one. Decodes the wrapper and checks the real icon url is nested inside. VertexIcon — the inline-DOM icon surface — had no test file, so its object-contain fix (the one this component actually needed for issue #2108) shipped unverified. --- .../src/components/VertexIcon.test.tsx | 63 +++++++++++++++++++ .../src/core/icons/iconRegistry.test.ts | 26 ++++---- .../GraphViewer/useGraphStyles.test.tsx | 15 +++-- 3 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 packages/graph-explorer/src/components/VertexIcon.test.tsx diff --git a/packages/graph-explorer/src/components/VertexIcon.test.tsx b/packages/graph-explorer/src/components/VertexIcon.test.tsx new file mode 100644 index 000000000..d1b0c8ee4 --- /dev/null +++ b/packages/graph-explorer/src/components/VertexIcon.test.tsx @@ -0,0 +1,63 @@ +// @vitest-environment jsdom + +import { render, waitFor } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; + +import { + appDefaultVertexStyle, + createVertexType, + type VertexStyle, +} from "@/core"; + +import VertexIcon from "./VertexIcon"; + +function renderIcon(overrides: Partial) { + const vertexStyle: VertexStyle = { + ...appDefaultVertexStyle, + type: createVertexType("Person"), + ...overrides, + }; + return render().container; +} + +describe("VertexIcon", () => { + // The one thing this branch changed here (issue #2108): without + // object-contain, object-fit's default is `fill`, which stretches a + // non-square raster to the fixed size-6 box instead of scaling it. + it("renders a raster icon with object-contain so it scales instead of stretching", () => { + const container = renderIcon({ + iconUrl: "https://example.test/wide.png", + iconImageType: "image/png", + }); + + const img = container.querySelector("img"); + expect(img).toBeTruthy(); + expect(img!.className).toContain("object-contain"); + expect(img!.getAttribute("src")).toBe("https://example.test/wide.png"); + }); + + it("renders a lucide icon inline so it inherits the vertex color", async () => { + const container = renderIcon({ + iconUrl: "lucide:plane", + iconImageType: "image/svg+xml", + color: "#FF0000", + }); + + await waitFor(() => expect(container.querySelector("svg")).toBeTruthy()); + const icon = container.querySelector("svg"); + expect(icon).toBeTruthy(); + expect((icon as SVGElement).style.color).toBe("rgb(255, 0, 0)"); + // Live DOM, not an / — this is the inline-DOM surface. + expect(container.querySelector("img")).toBeNull(); + }); + + it("renders nothing for an unknown lucide reference", () => { + const container = renderIcon({ + iconUrl: "lucide:not-a-real-icon-name-xyz", + iconImageType: "image/svg+xml", + }); + + expect(container.querySelector("svg")).toBeNull(); + expect(container.querySelector("img")).toBeNull(); + }); +}); diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts index 7cd23e8d9..28dd9275b 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts @@ -299,21 +299,21 @@ describe("sanitizes a user-supplied svg before storing it", () => { // ever dropped. it("preserves the geometry and path attributes an icon needs to scale", async () => { const svg = await resolveCustomSvg( - ``, + ``, ); - for (const attribute of [ - "width", - "height", - "viewBox", - "preserveAspectRatio", - "d", - "stroke-width", - "transform", - "points", - ]) { - expect(svg).toContain(attribute); - } + // Distinct numbers per attribute, so a check can only pass if that + // specific attribute-value pair survived — unlike `toContain(name)`, + // which a substring of an unrelated attribute (`preserveAspectRatio` + // contains "d"; `stroke-width` contains "width") can satisfy for free. + expect(svg).toContain('width="401"'); + expect(svg).toContain('height="102"'); + expect(svg).toContain('viewBox="0 0 403 104"'); + expect(svg).toContain('preserveAspectRatio="xMidYMid meet"'); + expect(svg).toContain('d="M0 0h405v106H0z"'); + expect(svg).toContain('stroke-width="7"'); + expect(svg).toContain('transform="translate(9 9)"'); + expect(svg).toContain('points="1,1 11,11"'); }); it("strips a