Skip to content

Improve theme style ref fidelity - #17

Open
RealSid08 wants to merge 2 commits into
mainfrom
codex/theme-color-fidelity
Open

RealSid08 wants to merge 2 commits into
mainfrom
codex/theme-color-fidelity

Conversation

@RealSid08

Copy link
Copy Markdown
Member

Summary

  • preserve theme style-ref defaults for text-bearing shapes without overriding explicit useBgFill or noFill
  • add parser and renderer regressions for style-matrix-backed text shapes and background-fill guardrails
  • make theme-dependent visual fixtures keep a slide-layout relationship so LibreOffice references inherit the same theme context
  • add a focused theme-style-refs visual case to measure this fidelity surface end to end

Validation

  • pnpm run fixtures:generate
  • pnpm run verify
  • pnpm run pack:check
  • pnpm run test:browser
  • PPTX_VISUAL_CASES=complex-sanitized,theme-colors,theme-luminance,theme-style-refs pnpm run test:visual

Visual Notes

  • complex-sanitized remains stable at 0.2895 mean diff
  • corrected theme fixture inheritance drops theme-colors to 0.0462 and theme-luminance to 0.0523
  • new theme-style-refs case improves from 0.1345 on main to 0.0214 on this branch

@RealSid08 RealSid08 added the release:patch Publish a patch release on merge label Apr 1, 2026
@github-actions

github-actions Bot commented Apr 1, 2026 •

Copy link
Copy Markdown
Contributor

Visual PR Diff

Compared 15 visual cases against the latest successful main visual artifact and detected no renderer-output changes.

Renderer Change Ranking

Case Max Diff Mean Diff Top Changed Slides
bullets-numbering 0.0000 0.0000 1 (0.0000)
complex-sanitized 0.0000 0.0000 1 (0.0000), 4 (0.0000), 5 (0.0000), 8 (0.0000)
diagram-fallback 0.0000 0.0000 2 (0.0000), 10 (0.0000)
group-transform 0.0000 0.0000 1 (0.0000)
image-cropping 0.0000 0.0000 1 (0.0000)
image-placement 0.0000 0.0000 1 (0.0000)
nested-groups 0.0000 0.0000 1 (0.0000)
paragraph-spacing 0.0000 0.0000 1 (0.0000)
slop-sample 0.0000 0.0000 1 (0.0000), 2 (0.0000), 3 (0.0000), 4 (0.0000), 5 (0.0000)
table-layout 0.0000 0.0000 1 (0.0000)

Missing baseline coverage: theme-style-refs (missing-baseline)

Most Improved Vs Office Reference

Case Mean Delta Max Delta Best Slide Delta
theme-luminance -0.1316 -0.1316 1 (-0.1316)
theme-colors -0.0758 -0.0758 1 (-0.0758)

No renderer deltas were detected in this PR. Open the published reports for full slide-by-slide inspection.

Artifacts include HTML side-by-side reports under visual-regression-artifacts and visual-pr-comparison.

github-actions Bot added a commit that referenced this pull request Apr 1, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbbdbc07c3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/parser.ts
Comment on lines +571 to +575
function shouldApplyThemeLineFallback(sp: Element): boolean {
const ln = sp.querySelector(":scope > spPr > ln");
if (!ln) return true;

return !ln.querySelector(":scope > noFill");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Prevent line fallback from overriding explicit line fill

shouldApplyThemeLineFallback currently enables style-matrix fallback for any <a:ln> that does not contain <a:noFill>, so shapes with explicit line fills like <a:gradFill> or <a:pattFill> still get lnRef defaults applied. In this commit, parseShape/parseTextShapeFrame then use that fallback when parseStrokeColor cannot parse the explicit non-solid fill, which silently replaces the authored line style with theme color/dash/width. This regresses fidelity for non-solid line definitions and should be gated the same way as fill fallback (only when no explicit line fill content exists).

Useful? React with 👍 / 👎.

github-actions Bot added a commit that referenced this pull request Apr 1, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 68b3b19e6a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/parser.ts
fillType: themeFill?.fillType,
gradient: themeFill?.gradient,
strokeColor: parseStrokeColor(sp, theme.colors) || themeLine?.strokeColor,
strokeWidth: parseStrokeWidth(sp) || themeLine?.strokeWidth,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve explicit zero-width lines over theme fallback

The strokeWidth fallback uses parseStrokeWidth(sp) || themeLine?.strokeWidth, so an explicit OOXML line width of w="0" is treated as missing and replaced by the lnRef theme width. This causes shapes with an intentionally zero-width line to render a border when a style matrix line ref is present. Please use nullish fallback (??) so 0 remains authoritative; the same pattern is also present in parseTextShapeFrame.

Useful? React with 👍 / 👎.

This branch has not been deployed

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

Labels

release:patch Publish a patch release on merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant