From aea4a27485f2edd748722d52ce8fe05a5eb9a782 Mon Sep 17 00:00:00 2001 From: Mario Juarros Date: Mon, 31 Aug 2026 13:27:11 -0600 Subject: [PATCH] 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 | 7 +- .../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 | 17 +-- .../src/core/icons/iconRegistry.ts | 101 ++++++++++++++---- .../src/core/icons/iconSurfaces.test.tsx | 33 ++++++ .../graph-explorer/src/core/icons/index.ts | 3 + .../src/core/icons/svgSanitize.ts | 32 ++++++ .../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 ++++ 18 files changed, 448 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/svgSanitize.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..99bd72340 100644 --- a/packages/graph-explorer/src/components/VertexIcon.tsx +++ b/packages/graph-explorer/src/components/VertexIcon.tsx @@ -3,13 +3,16 @@ import { DynamicIcon } from "lucide-react/dynamic"; import SVG from "react-inlinesvg"; import { useVertexStyle, type VertexStyle, type VertexType } from "@/core"; +import { ensureSvgViewBox, SVG_ALLOWED_ATTR } 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 }, + ALLOWED_ATTR: SVG_ALLOWED_ATTR, }); + return ensureSvgViewBox(sanitized); } interface Props { @@ -54,7 +57,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..200c73368 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 () => { diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.ts b/packages/graph-explorer/src/core/icons/iconRegistry.ts index e4ed001dc..486861bb3 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.ts @@ -4,11 +4,13 @@ import { logger } from "@/utils"; import { getLucideSvgString } from "@/utils/lucideIcons"; import { type IconSource, type IconSourceId, iconSourceId } from "./iconSource"; +import { SVG_ALLOWED_ATTR } from "./svgSanitize"; +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 +50,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 +128,97 @@ 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 }, + ALLOWED_ATTR: SVG_ALLOWED_ATTR, }); // 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..d4c56efeb 100644 --- a/packages/graph-explorer/src/core/icons/index.ts +++ b/packages/graph-explorer/src/core/icons/index.ts @@ -1,4 +1,7 @@ +export * from "./aspectFit"; export * from "./iconImageUrl"; export * from "./iconRegistry"; export * from "./iconSource"; +export * from "./svgSanitize"; +export * from "./svgViewBox"; export * from "./useResolvedIcons"; diff --git a/packages/graph-explorer/src/core/icons/svgSanitize.ts b/packages/graph-explorer/src/core/icons/svgSanitize.ts new file mode 100644 index 000000000..34e8bd29b --- /dev/null +++ b/packages/graph-explorer/src/core/icons/svgSanitize.ts @@ -0,0 +1,32 @@ +/** + * DOMPurify's default SVG profile drops `width`/`height`/`viewBox` — safe + * attributes it doesn't allowlist by default — which silently breaks aspect + * ratio for every sanitized icon. Every consumer that sanitizes an untrusted + * SVG needs this same allowlist, so it lives here once rather than drifting + * between the DOM and canvas render paths. + */ +export const SVG_ALLOWED_ATTR = [ + "width", + "height", + "viewBox", + "xmlns", + "preserveAspectRatio", + "fill", + "stroke", + "id", + "class", + "style", + "x", + "y", + "x1", + "y1", + "x2", + "y2", + "cx", + "cy", + "r", + "offset", + "stop-color", + "stop-opacity", + "opacity", +]; 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 60a65f807..75803631c 100644 --- a/packages/graph-explorer/src/setupTests.ts +++ b/packages/graph-explorer/src/setupTests.ts @@ -14,6 +14,24 @@ import { iconRegistry } from "@/core/icons"; expect.extend(matchers); +/** + * 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"), () => { @@ -30,6 +48,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.