Skip to content

fix(pdf): rest glyph boxes on the real descender (row detection was systematically shifted) - #16

Merged
hallelx2 merged 1 commit into
mainfrom
halleluyaholudele/hal-510-afm-vertical-metrics
Aug 2, 2026
Merged

fix(pdf): rest glyph boxes on the real descender (row detection was systematically shifted)#16
hallelx2 merged 1 commit into
mainfrom
halleluyaholudele/hal-510-afm-vertical-metrics

Conversation

@hallelx2

@hallelx2 hallelx2 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

The vertical counterpart of #9. Targets main directly — the rest of this chain (#9, #13, #14, #15) has landed.

Two independent bugs, both pushing every glyph box too high

1. Missing metrics. The 14 standard fonts may omit /FontDescriptor for exactly the reason they may omit /Widths — a consumer is expected to already know their metrics. So Ascent/Descent stayed 0 and a glyph's box collapsed to [baseline, baseline+size] instead of resting on the descender.

2. Missing scale factor — and this one would have survived fixing the first. Descent is stored in /1000ths of an em, so reaching text space needs both 0.001 and the font size, the same two factors the advance width already gets:

descent := font.Descent * 0.001              // before
descent := font.Descent * 0.001 * fontSize   // after

pdfminer does get_descent() * fontsize, where get_descent() is already descent/1000. Without the font size the descender was a fixed fraction of a point rather than a fraction of the glyph — so even a font that did supply a descriptor was short by a factor of the font size. At 12pt that is 12×.

Why it matters structurally

Columns come from word X extents; rows come from word Y extents. A systematic vertical shift of ~20% of a font size can merge or split table rows — corrupting the table an LLM then reasons over. It also nudged citation-highlight overlays (#11) upward on the page.

Measurement

Against pdfplumber on the golden fixtures:

              before                       after
12pt text     2.484pt off                  0.0000pt
24pt text     4.968pt off                  0.0000pt

Both were exactly 0.207 × size — Helvetica's −207/1000 descender, which is what identified the cause.

The golden envelope drops from 6pt to 0.01pt, so both axes are now asserted at the same tolerance as page dimensions. X was already 0.01 from #9; Y sat at 6pt explicitly flagged as documenting a defect rather than blessing it.

Data provenance

afm_vmetrics.go is generated from pdfminer.six's fontmetrics.py — the same Adobe AFM data, and the implementation we measure parity against — not transcribed by hand.

Symbol and ZapfDingbats are deliberately absent: their AFMs genuinely carry no Ascender/Descender entry, and pdfminer reads them as 0. Substituting their FontBBox would be defensible but would diverge from the reference. A test pins the absence so it reads as a decision rather than an oversight.

Verification

  • go build, go vet, go test ./... -count=1 -race — all green.
  • All golden tests pass at 0.01pt on both axes, including the table goldens (row boundaries did not shift in a way that changes cell output).
  • Re-checked on 3M 2018 10-K: all 103/103 negatives still intact, balance sheet rows unchanged.
  • TestDescentScalesWithFontSize asserts the pre-fix formula does not coincide with the correct one at any tested size, so the test genuinely detects the regression rather than passing by accident.

Closes HAL-510

Summary by Sourcery

Correct glyph vertical positioning using real descender metrics and tighten coordinate parity with pdfplumber.

Bug Fixes:

  • Use AFM vertical metrics for standard-14 fonts when /FontDescriptor is omitted so glyph boxes rest on the true descender instead of collapsing to baseline-plus-size.
  • Scale font descent by both 0.001 and font size when computing glyph bounding boxes so descender offsets are correct across sizes.
  • Preserve glyphs that slightly straddle a table’s outer edge by assigning them to the enclosing cell instead of discarding them at the boundary.

Enhancements:

  • Expose helpers to convert glyph bounding boxes into viewer-space rectangles for citation highlighting with normalized coordinates.
  • Bundle AFM-based vertical metrics for the 14 standard PDF fonts, matching pdfminer.six behaviour.

Documentation:

  • Update README and changelog to document the vertical metrics fix, improved golden tolerances, and new viewer-space bbox helpers.

Tests:

  • Tighten golden word-position tolerances to 0.01pt on both axes and confirm zero drift against pdfplumber.
  • Add tests for standard-14 vertical metrics, including deliberate Symbol/ZapfDingbats absences and alias handling.
  • Add a regression test ensuring descent scaling includes font size and would fail under the previous buggy formula.

Two independent bugs, both leaving every glyph box too high.

First, the 14 standard fonts may omit /FontDescriptor for exactly the
reason they may omit /Widths -- a consumer is expected to know their
metrics -- so Ascent and Descent stayed 0 and a glyph box collapsed to
[baseline, baseline+size] instead of resting on the descender. Bundles
the AFM Ascent/Descent alongside the widths, generated from the same
Adobe data pdfminer.six ships rather than transcribed. Symbol and
ZapfDingbats are deliberately absent: their AFMs genuinely carry no
Ascender/Descender and pdfminer reads them as 0, so substituting their
FontBBox would break parity.

Second, and it would have survived the first fix: descent was scaled by
0.001 but not by the font size. It is stored in /1000ths of an em, so
reaching text space needs both factors -- the same two the advance width
already gets. pdfminer does get_descent() * fontsize. Without it the
descender was a fixed fraction of a point instead of a fraction of the
glyph, so even a font that DID supply a descriptor was short by a factor
of the font size.

This is the vertical counterpart of the width fix and matters for the
same structural reason: columns come from word X extents, rows come from
word Y extents. A systematic vertical shift of ~20% of a font size can
merge or split table rows, which corrupts the table an LLM then reasons
over.

Measured against pdfplumber on the golden fixtures, Y drift goes from
2.484pt at 12pt and 4.968pt at 24pt -- both exactly 0.207*size, matching
Helvetica -207/1000 -- to 0.0000pt. The golden envelope drops from 6pt
to 0.01pt, so both axes are now asserted at the same tolerance as page
dimensions. Re-checked on 3M 2018 10-K: all 103 negatives still intact
and the balance sheet rows unchanged.

@sourcery-ai sourcery-ai 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.

Sorry @hallelx2, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@sourcery-ai

sourcery-ai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Reviewer's Guide

Fixes glyph bounding box vertical positioning by supplying AFM-based ascent/descent metrics for standard-14 fonts and correctly scaling descent by font size, tightens golden position tolerances, and adds tests and documentation to pin metrics behavior and parity with pdfminer.six.

Sequence diagram for applying standard-14 vertical metrics to glyph bounding boxes

sequenceDiagram
    participant Reader
    participant Font
    participant Standard14VMetrics
    participant Interpreter
    participant GlyphBBox

    Reader->>Font: readFont(ref)
    alt Standard14 with missing FontDescriptor
        Reader->>Standard14VMetrics: Standard14VMetrics(baseFont)
        Standard14VMetrics-->>Reader: VMetrics(Ascent, Descent), ok
        Reader->>Font: set Ascent, Descent from VMetrics
    else Non-standard or Symbol/ZapfDingbats
        Reader->>Font: keep existing Ascent, Descent
    end

    Interpreter->>Font: showString(s)
    Interpreter->>Font: get Descent, CharWidth(cid)
    Interpreter->>Interpreter: dxScale(fontSize)
    Interpreter->>Interpreter: descent = font.Descent * 0.001 * fontSize
    Interpreter->>Interpreter: adv = font.CharWidth(cid) * dxScale
    Interpreter->>GlyphBBox: build [0, descent+rise, adv, descent+rise+fontSize]
    Interpreter->>GlyphBBox: ApplyRect(combined, bbox)
Loading

File-Level Changes

Change Details Files
Bundle AFM vertical metrics for standard-14 fonts and apply them when /FontDescriptor is missing so glyph boxes rest on the real descender.
  • Introduce VMetrics struct and AFM-based ascent/descent table for the 14 standard fonts.
  • Implement Standard14VMetrics helper that resolves the same aliases and subset names as Standard14Widths while intentionally not matching Symbol/ZapfDingbats or condensed variants.
  • Update Reader.readFont to populate Font.Ascent/Font.Descent from Standard14VMetrics when a standard-14 font lacks a /FontDescriptor.
internal/pdf/afm_vmetrics.go
internal/pdf/reader.go
Correct descent scaling in glyph bbox computation so descender offsets scale with font size instead of being a fixed fraction of a point.
  • Adjust glyph bbox computation in the text interpreter to multiply font.Descent by both 0.001 and fontSize in line with pdfminer.six.
  • Document the rationale in code comments, including the previous 12x error at 12pt and parity with pdfminer.six’s get_descent()*fontsize behavior.
internal/pdf/content.go
Add tests that lock in AFM vertical metrics, alias behavior, deliberate omissions, and the correct descent scaling formula.
  • Add TestStandard14VMetrics to assert ascent/descent values for key standard fonts and their aliases, verify Symbol/ZapfDingbats absence, and ensure condensed/custom fonts do not match.
  • Add TestDescentScalesWithFontSize to assert the correct descent*fontSize scaling at multiple sizes and to guarantee the pre-fix formula cannot accidentally pass.
internal/pdf/vmetrics_test.go
Tighten golden word position tolerances to 0.01pt on both axes now that vertical metrics are fixed and document the resolved defect.
  • Update golden_test position tolerance comments to describe the previous vertical defect, its two causes, and that both axes now measure 0.0000pt drift against pdfplumber.
  • Reduce posTolY from 6.0pt to 0.01pt to match posTolX and enforce stricter parity.
golden_test.go
Update README and CHANGELOG to reflect fixed vertical metrics, table-row stability, glyph-at-table-edge handling, and viewer-coordinate helpers.
  • Document that vertical positions now match pdfplumber, both axes assert 0.01pt drift, and explain why AFM vertical metrics are bundled for standard-14 fonts.
  • Describe the fix for glyphs straddling a table’s outer edge and its impact on financial statement signs, and note the tightened golden position parity envelope.
  • Mention new BBox.Viewport and BBox.Normalized helpers for viewer coordinate conversion for citation highlights (implementation not shown in diff but documented).
README.md
CHANGELOG.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@hallelx2, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1c5602d6-92e2-4150-935c-9abcb532a9f8

📥 Commits

Reviewing files that changed from the base of the PR and between a5482af and 2412b3f.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • README.md
  • golden_test.go
  • internal/pdf/afm_vmetrics.go
  • internal/pdf/content.go
  • internal/pdf/reader.go
  • internal/pdf/vmetrics_test.go

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@hallelx2
hallelx2 merged commit 7224c2c into main Aug 2, 2026
5 checks passed
@hallelx2
hallelx2 deleted the halleluyaholudele/hal-510-afm-vertical-metrics branch August 2, 2026 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant