fix(importer): include all color attributes in style cache key#703
Open
ly-wang19 wants to merge 1 commit into
Open
fix(importer): include all color attributes in style cache key#703ly-wang19 wants to merge 1 commit into
ly-wang19 wants to merge 1 commit into
Conversation
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
approved these changes
Jul 1, 2026
Contributor
|
The change itself looks good (approving), but this branch currently isn't mergeable and needs a rebase. The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In the newly-added
@maic/importer,StyleResolver.buildColorCacheKeybuilds the color cache key from each node'slocalName+@valonly. ButresolveColorUncachedreads several OOXML color types from other attributes:scrgbClr@r@g@bhslClr@hue@sat@lumsysClr@lastClrSo every
scrgbClrfill yields the same key regardless of its RGB (likewise everyhslClr). The first such color resolved in aRenderContextis cached and then wrongly returned for every later one.Collision trace
Two shapes on one slide:
<a:solidFill><a:scrgbClr r="100000" g="0" b="0"/></a:solidFill>→ red<a:solidFill><a:scrgbClr r="0" g="0" b="100000"/></a:solidFill>→ bluebuildColorCacheKeyreturnssolidFill||scrgbClr:for both → B reuses A's cached red. Live on the import path (lib/import/use-import-pptx.ts→@maic/importer→resolveFill→resolveColor).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 barescrgbClr/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
@maic/importercurrently ships no test setup, the root vitest config globs onlytests/**with no DOM env, andparseXmlneedsDOMParser— 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.prettierclean,eslintclean,tsc --noEmiterror count unchanged (the only errors are pre-existing unresolved-workspace-module noise;StyleResolver.tstype-checks clean).Closes #702