Render behindDoc wpg group fills when a child shape carries a text box - #165
Conversation
DocxReader skipped ReadAnchorShapes for any run containing w:txbxContent, so a behindDoc wpg group lost every fill when one child rectangle held an (empty) text box. The floating text box path already skips fill extraction for groups and relies on ReadAnchorShapes to draw them, so nothing rendered the group at all. Read the group shapes when the drawing's anchor directly holds wpg:wgp, keeping the image guard as is. Fabrikam (issue/docx): overall 0.6223 -> 0.9770, visual 0.0558 -> 0.9426; the other 26 issue/docx cases are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe DOCX reader now processes every top-level drawing in a run, including grouped anchored shapes with text boxes. New tests validate multiple drawing parsing and Fabrikam rendering output. XML documentation describes the updated reader methods. ChangesDOCX drawing processing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to The updated drawing processing is covered by focused parsing and rendering tests, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The new group-detection condition in DocxReader is overly strict (direct-child .Element(...) chain) and may fail to detect anchors wrapped in common DOCX structures like mc:AlternateContent, reintroducing skipped group fills.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a DOCX rendering gap where a behindDoc="1" wpg:wgp background group could be skipped when one of its child shapes contains w:txbxContent, causing the page background and card rectangles to not be painted (notably for the Fabrikam.docx fixture).
Changes:
- Update
DocxReaderto still readbehindDocgroup shapes viaReadAnchorShapeseven when the run containsw:txbxContent, as long as the drawing contains awpg:wgpgroup. - Add a regression test asserting the expected background fill, white rectangles, and outline segments for
Fabrikam.docx.
File summaries
| File | Description |
|---|---|
src/MiniPdf/DocxReader.cs |
Ensures behindDoc WPG group shapes are still extracted when a run also contains textbox content. |
tests/MiniPdf.Tests/DocxIssueFileTests.cs |
Adds a fixture-based test validating that the group background + rectangles + outlines render. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| else if (runDrawing?.Element(WP + "anchor")?.Element(A + "graphic") | ||
| ?.Element(A + "graphicData")?.Element(WPG + "wgp") != null) | ||
| { | ||
| // A wpg group keeps its child fills even when one child carries a | ||
| // text box: the floating text box path skips fill extraction for | ||
| // groups (anchorHasGroupShape), so the group shapes are read here. | ||
| shapes.AddRange(ReadAnchorShapes(runDrawing, themeColors)); |
There was a problem hiding this comment.
Thanks for the review. The direct-child chain is deliberate, and I believe the wrapping premise does not apply here:
mc:AlternateContentwrapsw:drawingfrom above (w:r > mc:AlternateContent > mc:Choice > w:drawing > wp:anchor).runDrawingis located withDescendants(W + "drawing"), so the wrapper is already passed before the chain starts.Fabrikam.docx, the fixture this PR targets, is exactly such a wrapped drawing, and the new test exercises it.- Below
w:drawingthe structure is fixed by the schema:CT_Drawingis a choice ofwp:anchor/wp:inline,a:graphicis a direct element ofCT_Anchor, andwpg:wgpsits directly undera:graphicData(seeCT_Drawing,CT_AnchorandCT_GraphicalObjectin LibreOffice'ssw/source/writerfilter/ooxml/model.xml). Scanning every DOCX fixture undertests/Issue_Files/docx: all 107wp:anchorelements havew:drawingas their parent, and all 11wpg:wgpelements followa:graphicData < a:graphic < wp:anchor|wp:inline < w:drawing < mc:Choice. - A
Descendants(WPG + "wgp")match would also fire for a group nested inside a text box'sw:txbxContent, in the very run whose text box triggered the guard.ReadAnchorShapeswould then read that nested group against the outer anchor's offsets and paint it at the wrong position. The strict chain only accepts a group that is the anchor's own graphic, which is the case this PR restores.
I would therefore keep the precise check. Happy to switch to the descendant search if a maintainer prefers consistency with ReadAnchorShapes over that safeguard.
|
Note on the CodeRabbit pre-merge warning (docstring coverage 0%): the functions it counts are the existing |
Documents ReadParagraph, FindIssueDocx and the two DocxIssueFileTests methods that the previous commit touched, so the CodeRabbit docstring coverage pre-merge check on mini-software#165 passes. No behavior change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Added XML doc comments in 53541f0 to the four functions the diff touches (ReadParagraph, FindIssueDocx and the two DocxIssueFileTests methods) so the CodeRabbit docstring coverage pre-merge check passes. No behavior change; the full test suite still passes (193 tests). |
The docstring coverage check counts every function whose lines fall inside a diff hunk, including the three context lines, so documenting a function pulled its undocumented neighbour into the check (66.67%). Move the new Fabrikam test to the top of DocxIssueFileTests, where the leading context is the class header, and document the first test it now precedes. Document the two VML helpers above ReadParagraph, whose own predecessor (Read) is already documented. Drop the doc comments on Issue93 and FindIssueDocx, which the diff no longer touches. No behavior change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Follow-up in c91b31d: the docstring coverage check counts every function inside a hunk, including the three context lines, so each new comment pulled an undocumented neighbour into the set (66.67%). Moved the new test to the top of the class (its leading context is the class header), documented the first test it now precedes, and documented the two VML helpers above ReadParagraph, whose predecessor Read is already documented. Every function the diff touches now carries a doc comment. No behavior change; 193 tests pass. |
|
Maintainer follow-up added in 1f4540e: process each drawing owned by a run independently, with a synthetic regression test for multiple drawings when a later wpg group carries textbox content. Validation: 194/194 xUnit tests passed; focused Fabrikam score remains 0.9770; full 27-case issue/docx benchmark remains 0.9561 with 0 metric, page-count, or candidate PDF hash changes versus c91b31d. |
|
@Sen-CaPoo I've sent invitation 🙌🙌🙌 |
Problem
tests/Issue_Files/docx/Fabrikam.docx(business-card sheet) rendered as plain text on a white page. Word and LibreOffice both draw a light-blue page background, twenty white rectangles and ten card outlines. All of that comes from onebehindDoc="1"wpg:wgpgroup with 31wps:wspchildren.Root cause
DocxReaderonly callsReadAnchorShapeswhen the run contains now:txbxContent. The first rectangle of this group carries awps:txbxholding a single soft hyphen (and the VML fallback mirrors it withv:textbox), so the whole group was skipped. The floating text box path deliberately skips fill extraction for groups (anchorHasGroupShape) and expectsReadAnchorShapesto draw them, so neither path painted the group.Falsification check before editing: removing both the
wps:txbxand themc:Fallbackfrom a copy of the fixture made the existing code emit 21 rectangles and 40 outline segments with the correct geometry; removing only the fallback still produced nothing.LibreOffice keeps such groups intact:
oox/source/shape/WpgContext.cxx(WpgContext::onCreateContext) creates a shape context for everywspchild and recurses into nestedgrpSp;oox/source/shape/WpsContext.cxx(XML_txbx) only marks that child as a text box;sw/source/writerfilter/dmapper/GraphicImport.cxx(LN_CT_Anchor_behindDoc) places the anchor behind text.Change
When the run contains text box content but the drawing's
wp:anchordirectly holdswpg:wgp, still read the group shapes throughReadAnchorShapes.ReadImagekeeps the original guard, and single-shape text boxes keep their existing fill handling.Tests and validation
DocxIssueFileTests.Fabrikam_BehindDocGroupWithTextBox_RendersGroupFills(fails before the change, passes after).dotnet test tests/MiniPdf.Tests --configuration Release: 193 passed, 0 failed.git diff --check: clean.Benchmark evidence (.NET, issue suite, docx, Microsoft 365 reference)
Focused run:
pwsh -File scripts/Run-DotNet-VisualBenchmark.ps1 -Suite issue -Format docx -Filter "Fabrikam"Full run:
pwsh -File scripts/Run-DotNet-VisualBenchmark.ps1 -Suite issue -Format docxAll 27 cases converted, all PDFs valid, all comparison images present, no page-count changes, no
visual_avgdecrease. 25 cases have identical scores. OSCAR WARD moved +0.0033 visual; a rebuild frommainproduces a byte-identical PDF for that case, so the delta comes from the Office cloud-font cache (Source Sans Pro) populated while Word generated the reference PDFs, not from this change.Compatibility and scope
Internal
DocxReaderchange only; no public API change. Only DOCX documents with a behindDocwpg:wgpgroup whose child shape carries a text box are affected. Among the repository's docx fixtures, only Fabrikam has that structure. No new third-party material, fixtures or fonts. No documentation change needed.Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests