Skip to content

feat(page): carry the clip box of a PDF text cell - #964

Open
wittjeff wants to merge 2 commits into
docling-project:mainfrom
wittjeff:feat/pdf-text-cell-clip-bbox
Open

wittjeff wants to merge 2 commits into
docling-project:mainfrom
wittjeff:feat/pdf-text-cell-clip-bbox

Conversation

@wittjeff

@wittjeff wittjeff commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Companion to docling-project/docling-parse#443 (fixes docling-parse#404), for docling-project/docling#4762.

Some PDF producers fit long text into a table cell by clipping it to the cell's box (q <cell> re W n ... Q). The text layer keeps the whole string, whose hidden end runs under the next column, and the glyph rectangle alone cannot say which cell the text belongs to. docling-parse now tracks that clip per glyph; this PR gives it a place to land on the model.

Change

PdfTextCell.clip_bbox: BoundingBox | None = None: the bounding box of the clip region in force when the text was painted (ISO 32000-1, 8.5.4), in the same coordinate origin as rect; None when nothing clipped it. rect stays the full glyph box; the part of it outside clip_bbox is not visible on the page. A word or line carries the box when all of its glyphs share it.

to_top_left_origin() and to_bottom_left_origin() on PdfTextCell move the box with the rectangle, so a consumer that flips the page origin keeps the two consistent.

Two commits, rebased on main, no merge commits: the model change, and a follow-up that makes SegmentedPdfPage.get_cells_in_bbox convert the copy it returns through the cell's own origin methods, so a clipped cell comes back with rect and clip_bbox in the query's origin (the advisory first review caught that it reassigned rect alone). Squash on merge is fine. No schema version bump: PdfTextCell is not part of DoclingDocument, the field is optional with a None default, and check-compat-projectors passes.

Compatibility

  • A payload without the field still validates (test_legacy_payload_still_validates).
  • With exclude_none=True the field is absent from exports of cells that have no clip, so existing serialised pages do not change.
  • docling-parse fills the field only when the installed docling-core has it ("clip_bbox" in PdfTextCell.model_fields), so the two PRs can land in either order; once this one is released, docling-parse can raise its docling-core floor and drop the guard.

Tests

Seven tests in tests/test_page.py::TestPdfTextCellClipBbox: default and export omission, legacy payload, JSON round trip, origin conversion with and without a clip, and get_cells_in_bbox with a clipped cell queried in each origin. tests/test_page.py passes (48 tests); ruff, the compat-projector check and ty pass on the changed files.

Downstream

  • docling-parse#443: the parser side, with the A/B over the regression corpus (22 of 950 pages regroup, all clipped tables).
  • docling#4762: once a word carries clip_bbox, the table matcher can place it by that box instead of its glyph box; the docling-only fix in docling#4786 could not see the clip.

🤖 Generated with Claude Code

Some PDF producers fit long text into a table cell by clipping it to the
cell's box (`q <cell> re W n ... Q`). The text layer keeps the whole
string, whose hidden end runs under the next column, and the glyph
rectangle alone cannot say which cell the text belongs to.

`PdfTextCell.clip_bbox` holds the bounding box of the clip region that
was in force when the text was painted, in the same coordinate origin
as `rect`, or None when nothing clipped it. The origin conversions move
it with the rectangle. docling-parse fills it from its clip state
(docling-project/docling-parse#404); the table matcher in docling can
then place a clipped word by its clip rather than its glyph box
(docling-project/docling#4762).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Jeff Witt <1848307+wittjeff@users.noreply.github.com>
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

✅ DCO Check Passed

Thanks @wittjeff, all your commits are properly signed off. 🎉

@mergify

mergify Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 1 of 2 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require two reviewer for test updates 👀 reviews
🟢 Enforce conventional commit —

🔴 Require two reviewer for test updates

Waiting for

  • #approved-reviews-by >= 2
This rule is failing.

When test data is updated, we require two reviewers

  • #approved-reviews-by >= 2

Show 1 satisfied protection

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

@github-actions github-actions Bot added ai:ci-safe AI triage found no CI-safety concern (advisory) document-model DoclingDocument core model, metadata, hierarchy, validation enhancement New feature or request labels Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

AI triage (advisory)

Checked commit 37de3adc0e. The analysis read the diff only. It did not run code from this pull request.

The PR adds an optional clip_bbox field to PdfTextCell. The field holds the clip region that was active when the text was painted. PdfTextCell now moves clip_bbox together with rect in to_top_left_origin() and to_bottom_left_origin(), and SegmentedPdfPage.get_cells_in_bbox() now calls these methods.

CI safety: no concern found. A maintainer must still approve CI.

The ai:* labels are advisory. They do not approve CI and they do not add tests:full. A new push runs this check again.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI first review (advisory): blocker found

This review covers the full diff at b68744cd8f. It read the code and did not run it.

The new clip_bbox field is optional with a None default, so existing JSON still loads. The main risk is that one origin-conversion path in SegmentedPdfPage.get_cells_in_bbox does not move clip_bbox with rect. The PR says the two always share one origin. Check that path before merging.

Findings outside the diff lines

  • blocker docling_core/types/doc/page.py:826: get_cells_in_bbox converts rect but not clip_bbox
    The field description says clip_bbox is "in the same coordinate origin as rect". This PR adds PdfTextCell.to_top_left_origin() and to_bottom_left_origin() to keep the two boxes consistent. SegmentedPdfPage.get_cells_in_bbox does not call them. It assigns pc.rect directly, in both the TOPLEFT branch and the BOTTOMLEFT branch (also at line 828).

    Trigger: a page has PdfTextCells with a clip_bbox, and the caller passes a bbox in the other origin.

    Result: the returned cell has a rect in the origin of bbox, but its clip_bbox stays in the page origin. A consumer that compares the two boxes, for example with BoundingBox.get_intersection_bbox, gets wrong geometry or a ValueError("BoundingBoxes have different CoordOrigin"). The consumer would do this to find which table cell a clipped string belongs to, which is the purpose of the field.

    Fix: call the new method on the copy. For example, use pc.to_top_left_origin(self.dimension.height) and pc.to_bottom_left_origin(self.dimension.height). TextCell has the same methods, so this works for both cell types. Add a test that calls get_cells_in_bbox with a clipped cell and a bbox in the other origin. The new tests cover only the direct method calls.

A maintainer makes the merge decision. This review never approves the pull request.

@github-actions github-actions Bot added the ai:review-changes AI first review suggests changes label Nov 3, 2026
…bbox

`get_cells_in_bbox` reassigned `rect` on the copy it returns, so a
clipped cell came back with `rect` in the query's origin and
`clip_bbox` still in the page's. Convert the copy through the cell's
own origin methods instead; both `TextCell` and `PdfTextCell` have
them, so mixed cell lists keep working. Adds a test that queries a
clipped cell in each origin.

Found by the advisory first review on docling-project#964.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Jeff Witt <1848307+wittjeff@users.noreply.github.com>
@github-actions github-actions Bot added ai:ci-safe AI triage found no CI-safety concern (advisory) and removed ai:ci-safe AI triage found no CI-safety concern (advisory) ai:review-changes AI first review suggests changes labels Nov 3, 2026

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

ai:ci-safe AI triage found no CI-safety concern (advisory) document-model DoclingDocument core model, metadata, hierarchy, validation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant