Skip to content

fix(importer): include all color attributes in style cache key#703

Open
ly-wang19 wants to merge 1 commit into
THU-MAIC:mainfrom
ly-wang19:fix/importer-color-cache-key
Open

fix(importer): include all color attributes in style cache key#703
ly-wang19 wants to merge 1 commit into
THU-MAIC:mainfrom
ly-wang19:fix/importer-color-cache-key

Conversation

@ly-wang19

Copy link
Copy Markdown
Contributor

Problem

In the newly-added @maic/importer, StyleResolver.buildColorCacheKey builds the color cache key from each node's localName + @val only. But resolveColorUncached reads several OOXML color types from other attributes:

color type identity attributes read in cache key?
scrgbClr @r @g @b
hslClr @hue @sat @lum
sysClr @lastClr

So every scrgbClr fill yields the same key regardless of its RGB (likewise every hslClr). The first such color resolved in a RenderContext is cached and then wrongly returned for every later one.

Collision trace

Two shapes on one slide:

  • A: <a:solidFill><a:scrgbClr r="100000" g="0" b="0"/></a:solidFill> → red
  • B: <a:solidFill><a:scrgbClr r="0" g="0" b="100000"/></a:solidFill> → blue

buildColorCacheKey returns solidFill||scrgbClr: for both → B reuses A's cached red. Live on the import path (lib/import/use-import-pptx.ts@maic/importerresolveFillresolveColor).

Fix

Key each node by the full set of value-bearing attributes the resolver actually reads — val, lastClr, r, g, b, hue, sat, lum — applied to the top-level node and its children/grandchildren (the top-level node can itself be a bare scrgbClr/hslClr/sysClr, e.g. lines 167-190). Behavior-preserving for all previously-distinct keys; only previously-colliding keys become distinct.

With the fix the two fills above key to …|scrgbClr:,,100000,0,0,,, vs …|scrgbClr:,,0,0,100000,,, — distinct, correct colors.

Notes

  • No test added: @maic/importer currently ships no test setup, the root vitest config globs only tests/** with no DOM env, and parseXml needs DOMParser — a regression test would require standing up jsdom + a package test harness, out of scope for this fix. The collision trace above is the verification.
  • Verified locally: prettier clean, eslint clean, tsc --noEmit error count unchanged (the only errors are pre-existing unresolved-workspace-module noise; StyleResolver.ts type-checks clean).

Closes #702

buildColorCacheKey keyed color nodes only by localName + @Val, but
resolveColorUncached reads scrgbClr from @r/@g/@b, hslClr from
@hue/@sat/@lum, and sysClr from @lastClr. Every scrgbClr (and every
hslClr) fill therefore produced an identical cache key, so the first
such color resolved in a RenderContext was wrongly reused for all later
ones — importing later shapes/runs with the wrong fill or text color.

Key each node by the full set of value-bearing attributes the resolver
actually reads (val, lastClr, r, g, b, hue, sat, lum), applied to the
top-level node as well as its children/grandchildren, since the
top-level node can itself be a bare scrgbClr/hslClr/sysClr.

Closes THU-MAIC#702
@wyuc wyuc closed this Jul 1, 2026
@wyuc wyuc reopened this Jul 1, 2026
@wyuc

wyuc commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

The change itself looks good (approving), but this branch currently isn't mergeable and needs a rebase.

The @maic/importer package was renamed to @openmaic/importer on main (the SDK publish in #780), so packages/@maic/importer/src/serializer/StyleResolver.ts no longer exists at that path — GitHub can't create the merge commit against the renamed tree. Could you git rebase origin/main and force-push? The fix should re-home cleanly onto packages/@openmaic/importer/src/serializer/StyleResolver.ts (no content conflict, just the path move). Once it's rebased I'll merge. Thanks!

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.

bug(importer): color cache key collides for scrgbClr/hslClr/sysClr → wrong fill colors on import

2 participants