Skip to content

Fix non-square icons squashed instead of scaled to fit - #2142

Open
mjuarros wants to merge 1 commit into
aws:mainfrom
mjuarros:fix-square-icons-display
Open

mjuarros wants to merge 1 commit into
aws:mainfrom
mjuarros:fix-square-icons-display

Conversation

@mjuarros

@mjuarros mjuarros commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Description

Non-square custom vertex icons (raster images and SVG) were stretched into a square instead of scaled down to fit, distorting them. This happened consistently on both the graph canvas and DOM surfaces (search results, legend).

The root cause: Cytoscape's drawInscribedImage does not preserve aspect ratio when background-width and background-height are both set to percentages. They override the image's natural dimensions, then background-fit: contain scales the already-squashed result — the image is already distorted by then. The DOM rendered images with object-fit: fill (default), which has identical behavior.

How it's fixed

Icon measurement and aspect-ratio carry: The icon registry now measures the natural width/height of raster images when they resolve. That ratio is carried on the icon object through the system.

Aspect-fit computation: The background width/height for the Cytoscape canvas are computed from the target box (60% of node size) and the image's aspect ratio. The shorter axis is shrunk so the image fits inside the box without stretching — e.g., a 4:1 wide logo becomes 60% width, 15% height.

DOM surface fix: VertexIcon gains object-contain so its <img> respects the aspect ratio.

SVG handling: SVG images already carry a viewBox, so they letterbox themselves naturally with preserveAspectRatio=xMidYMid meet. New utility functions extract and validate viewBox data.

How to read

  1. src/core/icons/aspectFit.ts — core logic to compute fit dimensions from aspect ratio
  2. src/core/icons/iconRegistry.ts — measures image dimensions and carries aspect ratio
  3. src/components/VertexIcon.tsx — adds object-contain to preserve aspect on DOM
  4. modules/GraphViewer/useBackgroundImageMap.ts — applies aspect-fit logic to canvas styles
  5. src/core/icons/svgViewBox.ts — SVG viewBox extraction and validation

Supporting files (tests, minor updates): CONTEXT.md, setupTests.ts, and test files for all new/modified functions.

Validation

All unit tests pass. Manual testing confirmed non-square icons now render at their natural aspect ratio on the graph canvas and in DOM surfaces (search results, legend).

Before Fix
wide-logo-fail-handling

After Fix
wide-icon-success-handling

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 `<img>` (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.
@mjuarros
mjuarros marked this pull request as ready for review August 31, 2026 22:03
@kmcginnes

Copy link
Copy Markdown
Collaborator

Your root-cause analysis is correct. There's a shorter route to the same pixels, though.

Wrap every icon, raster included, in a padded square SVG and let preserveAspectRatio do the fitting:

background-fit: contain
background-width: auto
background-height: auto

<svg viewBox="0 0 100 100">
  <image href="<icon>" x="20" y="20" width="60" height="60"
         preserveAspectRatio="xMidYMid meet"/>
</svg>

The prototype I sent produces pixel-identical output to your branch across 4:1, 1:4, 1:1, 10:1 and a raster, in Chrome, Firefox and Safari 27. No measurement anywhere.

Note that contain with auto alone is not enough: it fits the 24x24 bounding box, and our nodes are ellipses, so a square-ish icon's corners spill outside the circle. The padded wrapper is what keeps the 60% inset.

This also lands the canvas on the same mechanism VertexSymbol already uses on the DOM side.

What it removes

measureImageDimensions and new Image(), async raster resolution, width?/height? on ResolvedIcon, fitAspectRatio, computeAspectRatioAwareDimensions, extractSvgDimensions, ensureSvgViewBox, and the Image double in setupTests.ts. Around 40 lines instead of 448, none async. Most of my review notes disappear with that code rather than needing fixes.

Still needed

Delete svgSanitize.ts and both ALLOWED_ATTR arguments. The option never takes effect: DOMPurify reads it at purify.cjs.js:742, then line 806 runs ALLOWED_ATTR = create(null) inside if (USE_PROFILES) and rebuilds from the profile sets. Only ADD_ATTR composes. Output is byte-identical with and without it across an 8-SVG diff, and the svg profile at line 316 already allows width, height, viewbox and preserveaspectratio, so the doc comment's premise is wrong too. Worse than dead: the list omits d, points, transform and stroke-width, so removing the apparently-redundant USE_PROFILES later would blank every path-based icon.

Restore the \u0000 cache key separator in useBackgroundImageMap.ts. | occurs in both the id and the color, so two entries can collide and swap icons between types.

Comment the background-fit line with why contain alone fails, plus a line in CONTEXT.md.

Fix the PR description's SVG bullet. It says SVGs "already carry a viewBox, so they letterbox themselves naturally," but the branch rewrites every SVG's intrinsic size. Your commit message has it right.

Rebase too. The safeSessionStorage.test.ts failure is a stale base, fixed by 5a50963.

Pre-existing

The review surfaced several older issues in files you touched. #2182 is filed; I'm tracking the rest. None block this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-square icons are squashed instead of scaled to fit

2 participants