From 3fa59725716d62b90d08f61b4514782f627828d0 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Tue, 4 Aug 2026 10:14:03 +0800 Subject: [PATCH 1/5] fix: emit page break before a group whose children start on a new page The group branch of _iterate_items() only matched ListGroup | InlineGroup, so a plain GroupItem (KEY_VALUE_AREA, and the per-slide groups the PowerPoint backend produces) matched neither it nor the DocItem branch that follows. The boundary was then emitted by the group's first child, placing the break after the group and attributing the group's content to the previous page. The same branch also yielded its break without advancing prev_page_nr, unlike the DocItem branch, so the group's first child re-emitted the same boundary; only the self_ref dedup in get_parts() kept the duplicate out of the output. Broaden the branch to GroupItem, advance prev_page_nr past the emitted boundary, and key _PageBreakNode.self_ref on the boundary rather than on a running counter. The last part is what makes the second safe: get_parts() re-enters for every group and shares visited with the recursion, so a group spanning a boundary is seen by both the nested and the root scope, and a scope-local counter would make the two disagree on the ref once the group branch starts incrementing it. Add position-asserting tests (the reported symptom has the correct placeholder count throughout) plus an iterator-level test that no boundary is emitted twice, which is the only level at which the duplicate is visible. --- docling_core/transforms/serializer/common.py | 30 ++- test/test_serialization.py | 183 ++++++++++++++++++- 2 files changed, 207 insertions(+), 6 deletions(-) diff --git a/docling_core/transforms/serializer/common.py b/docling_core/transforms/serializer/common.py index d78cf8fa7..d1e8c00e4 100644 --- a/docling_core/transforms/serializer/common.py +++ b/docling_core/transforms/serializer/common.py @@ -44,6 +44,7 @@ FloatingItem, Formatting, FormItem, + GroupItem, InlineGroup, KeyValueItem, ListGroup, @@ -79,6 +80,22 @@ class _PageBreakSerResult(SerializationResult): node: _PageBreakNode +def _page_break_ref(prev_page: int, next_page: int) -> str: + """Build the self_ref identifying a page break by the boundary it closes. + + Keying on the boundary rather than on a running counter is what lets the + ``self_ref`` dedup in ``get_parts()`` collapse the duplicate a group produces + when it spans a boundary: ``get_parts()`` re-enters for every group and shares + ``visited`` with the recursion, so the nested scope and the root scope both see + the same transition. A counter is scope-local -- the nested scope restarts at 0 -- + so the two scopes would disagree on the ref and the duplicate would surface. + + The result satisfies the ``^#(?:/([\\w-]+)(?:/(\\d+))?)?$`` ref validator, since + ``[\\w-]+`` accepts dashes. + """ + return f"#/pb-{prev_page}-{next_page}" + + def _iterate_items( doc: DoclingDocument, layers: Optional[set[ContentLayer]], @@ -89,7 +106,6 @@ def _iterate_items( ) -> Iterable[tuple[NodeItem, int]]: my_visited: set[str] = visited if visited is not None else set() prev_page_nr: Optional[int] = None - page_break_i = 0 for item, lvl in doc.iterate_items( root=node, with_groups=True, @@ -97,7 +113,7 @@ def _iterate_items( traverse_pictures=traverse_pictures, ): if add_page_breaks: - if isinstance(item, ListGroup | InlineGroup) and item.self_ref not in my_visited: + if isinstance(item, GroupItem) and item.self_ref not in my_visited: # if group starts with new page, yield page break before group node my_visited.add(item.self_ref) for it, _ in _iterate_items( @@ -113,12 +129,17 @@ def _iterate_items( if prev_page_nr is not None and page_no > prev_page_nr: yield ( _PageBreakNode( - self_ref=f"#/pb/{page_break_i}", + self_ref=_page_break_ref(prev_page_nr, page_no), prev_page=prev_page_nr, next_page=page_no, ), lvl, ) + # Advance past the boundary we just emitted, exactly as the + # DocItem branch below does. Without this, the group's first + # child re-emits the same boundary and only the self_ref + # dedup in get_parts() hides the duplicate. + prev_page_nr = page_no break elif isinstance(item, DocItem) and item.prov: page_no = item.prov[0].page_no @@ -126,13 +147,12 @@ def _iterate_items( if prev_page_nr is not None: # close previous range yield ( _PageBreakNode( - self_ref=f"#/pb/{page_break_i}", + self_ref=_page_break_ref(prev_page_nr, page_no), prev_page=prev_page_nr, next_page=page_no, ), lvl, ) - page_break_i += 1 prev_page_nr = page_no yield item, lvl diff --git a/test/test_serialization.py b/test/test_serialization.py index 86ad45469..5e9d55da2 100644 --- a/test/test_serialization.py +++ b/test/test_serialization.py @@ -7,7 +7,11 @@ import pytest -from docling_core.transforms.serializer.common import _DEFAULT_LABELS +from docling_core.transforms.serializer.common import ( + _DEFAULT_LABELS, + _iterate_items, + _PageBreakNode, +) from docling_core.transforms.serializer.html import ( HTMLDocSerializer, HTMLMetaSerializer, @@ -32,6 +36,7 @@ DescriptionAnnotation, EntitiesMetaField, EntityMention, + GroupLabel, LanguageMetaField, PictureClassificationMetaField, PictureClassificationPrediction, @@ -1237,3 +1242,179 @@ def test_html_meta_emits_xhtml_compatible_attributes(): assert 'data-meta-name="entities"' in html_out # Output must be parseable by a strict XML parser. ET.fromstring(html_out) + + +# =============================== +# Page break positioning (#705) +# =============================== + + +def _pb_prov(page_no: int) -> ProvenanceItem: + """Provenance on a given page; the bbox/charspan are irrelevant here.""" + return ProvenanceItem( + page_no=page_no, + bbox=BoundingBox.from_tuple((1, 2, 3, 4), origin=CoordOrigin.BOTTOMLEFT), + charspan=(0, 1), + ) + + +def _pb_doc(n_pages: int) -> DoclingDocument: + doc = DoclingDocument(name="page-breaks") + for page_no in range(1, n_pages + 1): + doc.add_page(page_no=page_no, size=Size(width=100, height=100)) + return doc + + +def _pb_render(doc: DoclingDocument) -> str: + return ( + MarkdownDocSerializer( + doc=doc, + params=MarkdownParams(page_break_placeholder=_PB), + ) + .serialize() + .text + ) + + +_PB = "" + + +def _pb_sections(md: str) -> list[list[str]]: + """Split rendered markdown into per-page sections of non-empty lines.""" + return [[line for line in section.splitlines() if line.strip()] for section in md.split(_PB)] + + +def test_md_page_break_precedes_group_starting_on_new_page(): + """A group whose children start on a new page gets the break *before* it. + + Regression test for #705: the break used to be emitted after the group, so the + group's content was attributed to the previous page. + """ + doc = _pb_doc(2) + doc.add_text(label=DocItemLabel.TEXT, text="Page 1 text", prov=_pb_prov(1)) + group = doc.add_group(label=GroupLabel.KEY_VALUE_AREA, name="kv") + doc.add_text(label=DocItemLabel.TEXT, text="KV child", prov=_pb_prov(2), parent=group) + doc.add_text(label=DocItemLabel.TEXT, text="Page 2 tail", prov=_pb_prov(2)) + + sections = _pb_sections(_pb_render(doc)) + + assert len(sections) == 2 + assert sections[0] == ["Page 1 text"] + assert sections[1] == ["KV child", "Page 2 tail"] + + +def test_md_page_break_per_slide_groups(): + """Each slide-like group lands in its own section, with no trailing break. + + Mirrors the PowerPoint backend shape reported in #705, where every slide's + content is wrapped in its own group. The placeholder *count* was already + correct before the fix (N-1); only the positions were wrong, so this asserts + positions rather than a count alone. + """ + n_slides = 5 + doc = _pb_doc(n_slides) + for page_no in range(1, n_slides + 1): + group = doc.add_group(label=GroupLabel.UNSPECIFIED, name=f"slide-{page_no}") + doc.add_text( + label=DocItemLabel.TEXT, + text=f"Slide {page_no}", + prov=_pb_prov(page_no), + parent=group, + ) + + md = _pb_render(doc) + sections = _pb_sections(md) + + assert md.count(_PB) == n_slides - 1 + assert sections == [[f"Slide {page_no}"] for page_no in range(1, n_slides + 1)] + + +@pytest.mark.parametrize("with_group", [False, True]) +@pytest.mark.parametrize("gap_at", ["none", "start", "middle", "end"]) +def test_md_page_break_positions_matrix(with_group: bool, gap_at: str): + """Every page's text must land in its own section, group or not, gap or not. + + The matrix is what catches position bugs that a placeholder count cannot: a + document can carry the right number of breaks while attributing content to the + wrong page. + """ + pages = [1, 2, 3, 4] + if gap_at == "start": + pages = [2, 3, 4] + elif gap_at == "middle": + pages = [1, 2, 4] + elif gap_at == "end": + pages = [1, 2, 3] + + doc = _pb_doc(4) + for page_no in pages: + if with_group: + group = doc.add_group(label=GroupLabel.UNSPECIFIED, name=f"group-{page_no}") + doc.add_text( + label=DocItemLabel.TEXT, + text=f"Text {page_no}", + prov=_pb_prov(page_no), + parent=group, + ) + else: + doc.add_text(label=DocItemLabel.TEXT, text=f"Text {page_no}", prov=_pb_prov(page_no)) + + sections = _pb_sections(_pb_render(doc)) + + # One section per page that carries content, in order, each holding only its + # own text -- no page's content bleeds into a neighbouring section. + assert sections == [[f"Text {page_no}"] for page_no in pages] + + +@pytest.mark.parametrize("empty_pages", [1, 2]) +def test_md_page_break_group_straddling_empty_pages(empty_pages: int): + """A group whose children straddle empty pages keeps each child in its section.""" + first, second = 1, 2 + empty_pages + doc = _pb_doc(second + 1) + doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(first)) + group = doc.add_group(label=GroupLabel.UNSPECIFIED, name="straddler") + doc.add_text(label=DocItemLabel.TEXT, text="Child A", prov=_pb_prov(first + 1), parent=group) + doc.add_text(label=DocItemLabel.TEXT, text="Child B", prov=_pb_prov(second), parent=group) + doc.add_text(label=DocItemLabel.TEXT, text="Outro", prov=_pb_prov(second + 1)) + + sections = _pb_sections(_pb_render(doc)) + + assert sections == [["Intro"], ["Child A"], ["Child B"], ["Outro"]] + + +def test_md_page_break_adjacent_groups_with_gap(): + """Two adjacent groups separated by an empty page stay in their own sections.""" + doc = _pb_doc(4) + first = doc.add_group(label=GroupLabel.UNSPECIFIED, name="first") + doc.add_text(label=DocItemLabel.TEXT, text="First group", prov=_pb_prov(1), parent=first) + second = doc.add_group(label=GroupLabel.UNSPECIFIED, name="second") + doc.add_text(label=DocItemLabel.TEXT, text="Second group", prov=_pb_prov(3), parent=second) + + sections = _pb_sections(_pb_render(doc)) + + assert sections == [["First group"], ["Second group"]] + + +def test_page_break_boundary_emitted_once_per_transition(): + """No boundary is emitted twice by the iterator, before any dedup can hide it. + + ``get_parts()`` drops nodes whose ``self_ref`` it has already seen, so a + duplicated boundary is invisible in the rendered output. Asserting at the + iterator level keeps the group branch honest: it must advance ``prev_page_nr`` + past the boundary it emits, otherwise the group's first child emits the same + transition again and the output stays correct only by accident. + """ + doc = _pb_doc(3) + doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(1)) + group = doc.add_group(label=GroupLabel.UNSPECIFIED, name="group") + doc.add_text(label=DocItemLabel.TEXT, text="A", prov=_pb_prov(2), parent=group) + doc.add_text(label=DocItemLabel.TEXT, text="B", prov=_pb_prov(2), parent=group) + + breaks = [ + node + for node, _ in _iterate_items(doc=doc, layers=None, add_page_breaks=True) + if isinstance(node, _PageBreakNode) + ] + + assert [(b.prev_page, b.next_page) for b in breaks] == [(1, 2)] + assert len({b.self_ref for b in breaks}) == len(breaks) From d042be900274c61c7f85ee581e2d27af41ca7398 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Tue, 4 Aug 2026 10:34:42 +0800 Subject: [PATCH 2/5] test: cover nested groups and non-markdown serializers Add the group-inside-a-group case, which is where the recursion in _iterate_items and the my_visited set it shares with its caller actually interact, and a DocTags assertion so the other consumers of the shared iterator cannot regress silently. Both fail without the GroupItem broadening. Extend the iterator-level test to a document with three distinct boundaries, so its uniqueness assertion is no longer trivially true for a single-element list, and note in the matrix test that the gap expectations pin current semantics that #466 / #472 would change. --- docling_core/transforms/serializer/common.py | 13 ++-- test/test_serialization.py | 63 +++++++++++++++++--- 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/docling_core/transforms/serializer/common.py b/docling_core/transforms/serializer/common.py index d1e8c00e4..17ca21086 100644 --- a/docling_core/transforms/serializer/common.py +++ b/docling_core/transforms/serializer/common.py @@ -135,10 +135,15 @@ def _iterate_items( ), lvl, ) - # Advance past the boundary we just emitted, exactly as the - # DocItem branch below does. Without this, the group's first - # child re-emits the same boundary and only the self_ref - # dedup in get_parts() hides the duplicate. + # Advance past the boundary we just emitted, as the DocItem + # branch below does. Without this, the group's first child + # re-emits the same boundary and only the self_ref dedup in + # get_parts() hides the duplicate. + # + # Unlike that branch this does not need to seed prev_page_nr + # when it is still None: the group's children are re-yielded + # at this level anyway, so the DocItem branch seeds it from + # the first one. prev_page_nr = page_no break elif isinstance(item, DocItem) and item.prov: diff --git a/test/test_serialization.py b/test/test_serialization.py index 5e9d55da2..5ae846116 100644 --- a/test/test_serialization.py +++ b/test/test_serialization.py @@ -12,6 +12,7 @@ _iterate_items, _PageBreakNode, ) +from docling_core.transforms.serializer.doctags import DocTagsDocSerializer from docling_core.transforms.serializer.html import ( HTMLDocSerializer, HTMLMetaSerializer, @@ -1265,6 +1266,9 @@ def _pb_doc(n_pages: int) -> DoclingDocument: return doc +_PB = "" + + def _pb_render(doc: DoclingDocument) -> str: return ( MarkdownDocSerializer( @@ -1276,9 +1280,6 @@ def _pb_render(doc: DoclingDocument) -> str: ) -_PB = "" - - def _pb_sections(md: str) -> list[list[str]]: """Split rendered markdown into per-page sections of non-empty lines.""" return [[line for line in section.splitlines() if line.strip()] for section in md.split(_PB)] @@ -1337,6 +1338,11 @@ def test_md_page_break_positions_matrix(with_group: bool, gap_at: str): The matrix is what catches position bugs that a placeholder count cannot: a document can carry the right number of breaks while attributing content to the wrong page. + + Note the gap cases pin today's semantics, where a multi-page gap yields a single + boundary rather than one per page: with pages [1, 2, 4] this expects three + sections, not a ``pb-3-4`` as well. #466 / #472 would change that, and would need + these expectations updated along with them. """ pages = [1, 2, 3, 4] if gap_at == "start": @@ -1404,11 +1410,14 @@ def test_page_break_boundary_emitted_once_per_transition(): past the boundary it emits, otherwise the group's first child emits the same transition again and the output stays correct only by accident. """ - doc = _pb_doc(3) + doc = _pb_doc(4) doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(1)) - group = doc.add_group(label=GroupLabel.UNSPECIFIED, name="group") - doc.add_text(label=DocItemLabel.TEXT, text="A", prov=_pb_prov(2), parent=group) - doc.add_text(label=DocItemLabel.TEXT, text="B", prov=_pb_prov(2), parent=group) + first = doc.add_group(label=GroupLabel.UNSPECIFIED, name="first") + doc.add_text(label=DocItemLabel.TEXT, text="A", prov=_pb_prov(2), parent=first) + doc.add_text(label=DocItemLabel.TEXT, text="B", prov=_pb_prov(2), parent=first) + doc.add_text(label=DocItemLabel.TEXT, text="Middle", prov=_pb_prov(3)) + second = doc.add_group(label=GroupLabel.UNSPECIFIED, name="second") + doc.add_text(label=DocItemLabel.TEXT, text="C", prov=_pb_prov(4), parent=second) breaks = [ node @@ -1416,5 +1425,43 @@ def test_page_break_boundary_emitted_once_per_transition(): if isinstance(node, _PageBreakNode) ] - assert [(b.prev_page, b.next_page) for b in breaks] == [(1, 2)] + assert [(b.prev_page, b.next_page) for b in breaks] == [(1, 2), (2, 3), (3, 4)] + # Distinct transitions must stay distinguishable, or get_parts()'s dedup would + # swallow a legitimate break rather than a duplicate one. assert len({b.self_ref for b in breaks}) == len(breaks) + + +def test_md_page_break_nested_groups(): + """A boundary inside a nested group is emitted before the innermost group. + + This is the one shape where the recursion in ``_iterate_items`` and the + ``my_visited`` set it shares with its caller actually interact: the outer group's + lookahead walks into the inner group, and both scopes see the same transition. + """ + doc = _pb_doc(3) + doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(1)) + outer = doc.add_group(label=GroupLabel.UNSPECIFIED, name="outer") + doc.add_text(label=DocItemLabel.TEXT, text="Outer child", prov=_pb_prov(1), parent=outer) + inner = doc.add_group(label=GroupLabel.UNSPECIFIED, name="inner", parent=outer) + doc.add_text(label=DocItemLabel.TEXT, text="Inner child", prov=_pb_prov(2), parent=inner) + doc.add_text(label=DocItemLabel.TEXT, text="Outro", prov=_pb_prov(3)) + + sections = _pb_sections(_pb_render(doc)) + + assert sections == [["Intro", "Outer child"], ["Inner child"], ["Outro"]] + + +def test_page_break_before_group_across_serializers(): + """The fix lives in the shared iterator, so every serializer benefits. + + ``_iterate_items`` backs markdown, LaTeX, DocTags and DocLang alike; asserting on + DocTags too keeps the other consumers from regressing silently. + """ + doc = _pb_doc(2) + doc.add_text(label=DocItemLabel.TEXT, text="Page 1 text", prov=_pb_prov(1)) + group = doc.add_group(label=GroupLabel.KEY_VALUE_AREA, name="kv") + doc.add_text(label=DocItemLabel.TEXT, text="KV child", prov=_pb_prov(2), parent=group) + + doctags = DocTagsDocSerializer(doc=doc).serialize().text + + assert doctags.index("") < doctags.index("KV child") From 81dfeba1e4d838111ced94c3772e69ff8940549e Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Tue, 4 Aug 2026 18:21:45 +0800 Subject: [PATCH 3/5] DCO Remediation Commit for Max Hsu I, Max Hsu , hereby add my Signed-off-by to this commit: 3fa59725716d62b90d08f61b4514782f627828d0 I, Max Hsu , hereby add my Signed-off-by to this commit: d042be900274c61c7f85ee581e2d27af41ca7398 Signed-off-by: Max Hsu From e9233b8b283e5a3020d93d95591670e8a43ce888 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Wed, 5 Aug 2026 15:43:14 +0800 Subject: [PATCH 4/5] docs(test): say why a corpus cannot replace the boundary property test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the reasoning from #705: a corpus is a good net for regressions and a bad net for this defect, because real documents rarely place a group across an empty page, so a faulty stream never meets an input that would render differently. The reporter's own latent gap-expansion defect passed both the markdown-level assertions and a 25-document corpus; asserting the property is what caught it. Comment only — no test or source behaviour changes. Signed-off-by: Max Hsu --- test/test_serialization.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/test_serialization.py b/test/test_serialization.py index 5ae846116..ff3ac26da 100644 --- a/test/test_serialization.py +++ b/test/test_serialization.py @@ -1409,6 +1409,13 @@ def test_page_break_boundary_emitted_once_per_transition(): iterator level keeps the group branch honest: it must advance ``prev_page_nr`` past the boundary it emits, otherwise the group's first child emits the same transition again and the output stays correct only by accident. + + This is also why a document corpus cannot stand in for this test. A corpus is + a good net for regressions and a bad net for this particular defect: real + documents rarely place a group across an empty page, so a faulty stream never + meets an input that would render differently. Reported on #705, where a latent + gap-expansion defect passed both the markdown-level assertions and a + 25-document corpus, and only asserting the property caught it. """ doc = _pb_doc(4) doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(1)) From 27b746949ae3ffc43d9cad7bbeb3ea0dbc5efbc9 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Wed, 5 Aug 2026 22:48:55 +0800 Subject: [PATCH 5/5] docs(test): correct the corpus claim on the boundary property test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous wording said a faulty stream "never meets an input that would render differently". Measurements from @serboor on #705 refute that: across a 25-document corpus, a 287-page document rendered 7 pairs of consecutive placeholders on the faulty build where a page-by-page reference had none. The misplacement is visible in rendered markdown; a corpus can catch it. What a corpus cannot catch is the duplicate underneath it: 732 boundaries emitted, 722 distinct, and exactly 722 placeholders in the output, because get_parts()'s self_ref dedup collapses the extra one before it can render. Two of the affected documents were byte-identical to the reference while their stream was wrong. Both assertions therefore stay, for two different reasons — which is what the docstring now says instead of overstating the first one. Comment only — no test or source behaviour changes. Signed-off-by: Max Hsu --- test/test_serialization.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/test/test_serialization.py b/test/test_serialization.py index ff3ac26da..b9ae7f47b 100644 --- a/test/test_serialization.py +++ b/test/test_serialization.py @@ -1410,12 +1410,22 @@ def test_page_break_boundary_emitted_once_per_transition(): past the boundary it emits, otherwise the group's first child emits the same transition again and the output stays correct only by accident. - This is also why a document corpus cannot stand in for this test. A corpus is - a good net for regressions and a bad net for this particular defect: real - documents rarely place a group across an empty page, so a faulty stream never - meets an input that would render differently. Reported on #705, where a latent - gap-expansion defect passed both the markdown-level assertions and a - 25-document corpus, and only asserting the property caught it. + This is also why a document corpus cannot stand in for this test. Quoting + @serboor on #705, who ran one: a corpus is "a good net for regressions and a + bad net for this" -- but the two halves of the defect are not equally hidden, + and only one of them is the reason this test exists. + + The *misplacement* does reach the rendered output: across 25 documents, one + 287-page document rendered 7 pairs of consecutive placeholders on the faulty + build -- sections left empty because the break came out after the group -- + where a page-by-page reference had none. A corpus can catch that. + + The *duplicate* underneath it cannot reach the output at all: the same corpus + emitted 732 boundaries of which 722 were distinct, and the markdown contained + exactly 722 placeholders. ``get_parts()``'s dedup collapses the extra one, so + no markdown-level assertion can fail on it by construction -- including on the + two documents that were byte-identical to the reference while their stream was + wrong. Asserting on the stream is what changes that. """ doc = _pb_doc(4) doc.add_text(label=DocItemLabel.TEXT, text="Intro", prov=_pb_prov(1))