Skip to content

feat: Phase D.20: Font.shading — run-level background color#92

Open
citconv-agents[bot] wants to merge 2 commits intomasterfrom
agent/issue-33
Open

feat: Phase D.20: Font.shading — run-level background color#92
citconv-agents[bot] wants to merge 2 commits intomasterfrom
agent/issue-33

Conversation

@citconv-agents
Copy link
Copy Markdown

@citconv-agents citconv-agents bot commented Apr 5, 2026

Summary

Implements #33

This PR was automatically generated by the Developer Agent.

Original Issue

Add support for run-level background/shading (distinct from highlight color).

API Design

  • font.shading_color — RGBColor or None
  • Separate from font.highlight_color which uses the limited WD_COLOR_INDEX palette
  • Shading allows any RGB color as background behind text

XML

w:rPr > w:shd with w:val='clear' w:fill='RRGGBB'

Note: font.highlight_color already exists for the basic highlights. This is the custom RGB background.

Upstream: python-openxml#309 (11 comments)


Generated by Developer Agent using Claude Code

Add support for run-level background/shading via w:rPr > w:shd, allowing
any RGB color as text background. Distinct from Font.highlight_color which
is limited to the WD_COLOR_INDEX palette.

Closes #33

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@citconv-agents
Copy link
Copy Markdown
Author

citconv-agents bot commented Apr 5, 2026

Security Agent Report

SECURITY_PASS

Security Review: PR #92 — Font.shading_color for run-level RGB background color

Scope

Three files changed:

  • src/docx/oxml/text/font.py — CT_RPr element class (oxml layer)
  • src/docx/text/font.py — Font proxy class (document API layer)
  • tests/text/test_font.py — Unit tests

Findings

XML Injection / XXE

No issues found.

The shd.fill attribute is set from an RGBColor object (a structured, typed value), not from raw user-supplied strings. The getter also guards with an explicit isinstance(fill, RGBColor) check before returning, preventing unexpected typed values from propagating. The shd.val attribute is set from the WD_SHADING_PATTERN.CLEAR enum constant — not from user input. No raw XML string construction or parsing from untrusted input occurs in the changed code.

Path Traversal

No issues found. No file paths are involved in these changes.

Dependency Risks

No issues found. No new dependencies are introduced. The change reuses existing internal types (RGBColor, CT_Shd, WD_SHADING_PATTERN) already present in the library.

Data Exposure / Unsafe File Handling

No issues found. No file I/O, no sensitive data handling.

Secrets in Code

No issues found. No API keys, tokens, or passwords present. The hex values in tests (FF0000, 3C2F80, etc.) are ordinary test color literals.

Miscellaneous

  • The lazy import inside the setter (from docx.enum.table import WD_SHADING_PATTERN) is a style pattern already used elsewhere in this codebase and poses no security risk.
  • Type annotations and isinstance guards are consistent with the rest of the codebase and reduce the risk of type-confusion bugs.

Summary

All changes are confined to adding a well-typed property (shading_color) that reads and writes an XML attribute through the existing, validated oxml abstraction layer. No untrusted input reaches XML serialization without going through typed intermediaries. No new attack surface is introduced.

@citconv-agents citconv-agents bot added the security-passed Security agent passed label Apr 5, 2026
@citconv-agents
Copy link
Copy Markdown
Author

citconv-agents bot commented Apr 5, 2026

Review Agent

REVIEW_CHANGES_NEEDED

PR #92 — Font.shading_color for run-level RGB background color

Issue: Import inside property setter

File: src/docx/oxml/text/font.py, line 193

@shading_color.setter
def shading_color(self, value: RGBColor | None) -> None:
    if value is None:
        self._remove_shd()
        return
    shd = self.get_or_add_shd()
    from docx.enum.table import WD_SHADING_PATTERN   # ← move to module level
    shd.val = WD_SHADING_PATTERN.CLEAR
    shd.fill = value

All other enum imports in this file are at module level (lines 9-10: from docx.enum.dml import MSO_THEME_COLOR, from docx.enum.text import WD_COLOR_INDEX, WD_UNDERLINE). This inline import should be moved there too:

from docx.enum.table import WD_SHADING_PATTERN

Everything else looks correct

  • Successor index: _tag_seq[30:] for w:shd is correct — w:shd is at index 29 in _tag_seq, so [30:] correctly captures all following elements as successors.
  • isinstance(fill, RGBColor) check: Correct. ST_HexColor.convert_from_xml returns either RGBColor or "auto" (ST_HexColorAuto.AUTO), so this check safely filters out the "auto" case.
  • val=CLEAR on set: Correct OOXML pattern for solid background fill — no foreground pattern, fill color is the background.
  • Empty rPr on None set: Consistent with how all other Font setters behave (e.g., _set_bool_prop also calls get_or_add_rPr() unconditionally).
  • Tests: Good coverage — getter/setter parametrized tests cover no-rPr, empty rPr, set, update, and clear cases.
  • CT_Shd type import under TYPE_CHECKING: Correct.
  • Callable stubs (get_or_add_shd, _remove_shd) added to the CT_RPr class body: correct pattern, in alphabetical order.

@citconv-agents citconv-agents bot added the review-changes-needed Review agent requested changes label Apr 5, 2026
Address review feedback on PR #92 — inline import in shading_color
setter moved to top of file, consistent with all other enum imports.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@citconv-agents citconv-agents bot removed security-passed Security agent passed review-changes-needed Review agent requested changes labels Apr 5, 2026
@citconv-agents
Copy link
Copy Markdown
Author

citconv-agents bot commented Apr 5, 2026

Security Agent Report

SECURITY_PASS

Security Review — PR #92

Reviewed: 2026-04-05
Branch: agent/issue-33 vs origin/master
Scope: Font.shading_color feature (run-level RGB background color)

Summary

No security issues found. The PR is clean.

Files Reviewed

File Change
src/docx/oxml/text/font.py Adds shd element descriptor and shading_color property to CT_RPr
src/docx/text/font.py Adds shading_color property to Font proxy
tests/text/test_font.py Parametrized unit tests for the new property

Checks

XML / XXE Injection

No risk. The fill attribute on CT_Shd is typed as ST_HexColor, which:

  • Serializes via "%02X%02X%02X" % value — a fixed-format integer format, no user string interpolated
  • Validates that the value is an RGBColor object (raises ValueError otherwise)
  • RGBColor.__new__ enforces all three components are int in [0, 255]

There is no pathway for arbitrary string content to reach the XML attribute.

Path Traversal

Not applicable. No file I/O introduced.

New Dependencies

None added.

Sensitive Data / Secrets

None present.

Data Exposure

The feature reads and writes w:shd/@w:fill XML attributes within a document's in-memory XML tree. No external I/O, no logging of values, no leakage surface introduced.

Logic / Access Control

The setter correctly delegates to the existing CT_Shd attribute machinery, and removal (value is None) calls _remove_shd() rather than leaving a partially-populated element behind.

Conclusion

The change is a straightforward, narrowly-scoped addition. All user-visible data flows through strongly-typed value objects with range validation before reaching XML serialization. No security concerns identified.

@citconv-agents citconv-agents bot added the security-passed Security agent passed label Apr 5, 2026
@citconv-agents
Copy link
Copy Markdown
Author

citconv-agents bot commented Apr 5, 2026

Review Agent

REVIEW_APPROVED

PR #92: Font.shading_color for run-level RGB background color

The implementation is correct, well-tested, and consistent with existing patterns.

Positive notes

Consistent with existing table API: The isinstance(fill, RGBColor) guard in the getter
(src/docx/oxml/text/font.py:183) is the correct approach — ST_HexColor.convert_from_xml()
returns RGBColor | str (the "auto" case returns a string), so the guard is needed. This
matches the identical pattern used in _Cell.fill_color getter at src/docx/table.py:360.

Correct _tag_seq index: w:shd is at index 29 in the tag sequence, so
successors=_tag_seq[30:] is the right slice.

Setter behavior is internally consistent: Removing the entire <w:shd> element when
setting None (rather than just nulling the fill attribute like _Cell.fill_color does)
is appropriate here since shading_color owns the full lifecycle of the shd element on
a run.

Good test coverage: The parametrized test matrix covers all meaningful cases — no rPr,
empty rPr, set/update/clear/noop.

Minor observations (non-blocking)

  1. Empty <w:rPr> on None set: Setting font.shading_color = None on a bare <w:r>
    creates <w:r><w:rPr/></w:r>. The test documents this behavior explicitly, and it's
    consistent with how Font.name = None works, so this is acceptable, but worth noting
    as a small inefficiency.

  2. No w:color attribute in setter: The setter writes only w:val and w:fill; it omits
    w:color. Word typically writes w:color="auto" on run shading. However, _Cell.fill_color
    also omits w:color in the same way, making this consistent. For w:val=clear this has
    no visual impact.

  3. No oxml-layer unit tests for CT_RPr.shading_color: The proxy-level tests in
    tests/text/test_font.py verify XML output end-to-end so coverage is adequate, but the
    project has tests/oxml/text/test_run.py which could host CT_RPr-level tests. Not
    required given the existing test quality.

@citconv-agents citconv-agents bot added the review-approved Review agent approved label Apr 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-pr PR created by an agent review-approved Review agent approved security-passed Security agent passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant