Do not derive a zero text chunk height from an empty font bounding box - #750
Do not derive a zero text chunk height from an empty font bounding box#750SangWJDev wants to merge 1 commit into
Conversation
When a font descriptor has no /Ascent and no /Descent, the vertical extent of a text chunk is taken from /FontBBox. A font bounding box whose four elements are all zero carries no size information, so this yields an ascent and a descent of zero and every chunk of that font ends up with a height of exactly zero. Nothing downstream can tell that apart from a glyph that really has no height, and consumers that discard tiny text drop the whole document without an exception, a warning or a log entry. Real files hit this: PowerPoint exports embed Type 3 fonts with /FontBBox [0 0 0 0] in both the font dictionary and the descriptor, and with neither /Ascent nor /Descent. Fall back to a common em split when there are no usable vertical metrics at all. The values are divided by the vertical scaling factor so that they are expressed in glyph space, which keeps them correct for a Type 3 font with any FontMatrix.
📝 WalkthroughWalkthrough
ChangesFont metric fallback
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Adding the exact wording from ISO 32000-1:2008. It turns out the spec states this explicitly, and — usefully for this report — it states it in the Type 3 font dictionary table rather than only in the generic font descriptor table, so it applies directly to the font in the attached file. 9.6.5, Type 3 Fonts, Table 112 — Entries in a Type 3 font dictionary,
That is a In case the attached file looks malformed at first glance — it is not. The spec exempts Type 3 fonts from every single entry involved here:
So a Type 3 font with no This does not settle which fallback values are right — that part of the patch is still just a proposal — but it does settle that propagating the zero is not an option. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextChunksHelper.java`:
- Around line 72-83: Update the x-bound calculations in TextChunksHelper to use
verticalScalingFactor for the ascent and descent terms, matching the fallback
metric conversion above and avoiding horizontal/vertical scale mixing or sign
reversal. Add regression coverage for non-uniform and negative Type 3 FontMatrix
values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10bd2a51-68e5-4df3-a30c-953123f6a87f
📒 Files selected for processing (1)
wcag-validation/src/main/java/org/verapdf/gf/model/factory/chunks/TextChunksHelper.java
Fixes the silent text loss reported in veraPDF/veraPDF-library#1624.
When a font descriptor has no
/Ascentand no/Descent,calculateTextBoundingBoxHtakes the vertical extent from/FontBBox. A font bounding box whose four elements are all zero carries no size information, so this yields an ascent and a descent of zero and every text chunk of that font ends up with a height of exactly zero.Nothing downstream can tell that apart from a glyph that really has no height. Consumers that discard tiny text then drop the whole document — in the case that led me here, a 156-page file extracted 0 characters with no exception, no warning and no log entry.
This falls back to a common em split when there are no usable vertical metrics at all:
The two scaling-factor lookups move up to the top of the method so the fallback can use them; they are otherwise unchanged. Dividing by the vertical scaling factor expresses the values in glyph space, which keeps them correct for a Type 3 font with any
FontMatrixas well as for ordinary fonts.The constants are a proposal, not a claim
0.8/−0.2is a widely used default, but I have no attachment to those numbers — the point of the patch is not to propagate a meaningless0. If you would ratherd1operands of the Type 3CharProcs,say so and I will rework it. I would also be glad to hear if you think the right fix belongs somewhere else entirely.
Verification
repro-issueB-zero-fontbbox.pdfis a 1.8 KB hand-written file: one Type 3 font with the conventional/FontMatrix [0.001 0 0 0.001 0 0],/FontBBox [0 0 0 0]in both the font dictionary and the descriptor, no/Ascent, no/Descent, and a plainTjwith no kerning.pdftotextABCABCOn the 156-page document, extraction goes from 0 characters to 38 469 non-whitespace characters. That document also hits the separate
TJkerning problem, so the end-to-end run there had both patches applied; the repro file above isolates this one.For regression, I ran an 11-document corpus (roughly 1 400 pages) through the stock and patched builds. The 10 documents that do not hit this path produced byte-identical output — the new branch is only reachable when a font supplies no usable vertical metrics at all.
Builds clean:
mvn -pl wcag-validation -am compile.How the runtime numbers were produced, so you can weigh them properly: the behavioural runs above were made with the equivalent change applied to the v1.31.99 sources and the recompiled classes swapped into an opendataloader-pdf fat jar, since that is a build that can actually extract text end to end. The branch itself is against
integrationand was verified to compile there; the change is the same in both.Note
I could not add a regression test —
wcag-validationhas nosrc/testdirectory and I did not want to introduce a test module unprompted. If you would like the repro PDF committed as a fixture, tell me where it should go and I will add it with a test.Summary by CodeRabbit