Skip to content

fix(md): keep inline code spans inside GFM table cells - #4327

Merged
ceberam merged 2 commits into
docling-project:mainfrom
kevin9327:fix/md-codespan-in-tables
Sep 29, 2026
Merged

ceberam merged 2 commits into
docling-project:mainfrom
kevin9327:fix/md-codespan-in-tables

Conversation

@kevin9327

@kevin9327 kevin9327 commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

A GFM table cell that contains an inline code span (run \build` now`) used to close the table at the span. That split one table into two and dropped the text before the backticks.

Code spans now stay in the current table row, like emphasis and links already do, and the cell keeps its words around the span.

The table buffer has one contract now, stated next to _unescape_except_pipe: _close_table decodes each cell exactly once, a RawText keeps its whitespace and its pipe entities on the way in (so an escaped pipe survives the split), and literal text such as a code span is entity-encoded on the way in with its pipes as |. So \a | b`stays one cell and`&`stays the literal&`.

Preserving the span as a CodeItem inside the cell (RichTableCell, review point 1) is left for a separate change, as suggested in review; this one fixes the broken table.

Checklist:

  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

AI-assisted (Grok)

@github-actions

github-actions Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

✅ DCO Check Passed

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

@mergify

mergify Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 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)(?:\(.+\))?(!)?:

🟢 Require two reviewer for test updates

When test data is updated, we require two reviewers

  • #approved-reviews-by >= 2

@ghost

ghost commented Sep 20, 2026

Copy link
Copy Markdown

Hello,

It's been a very bad start from me I panicked I wanted to fix my problem and thought that AI will do it all by itself without reading properly the documents you sent me. I learned my mistake and hope that I will learn fast and contribute to your community.

@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ceberam ceberam added bug Something isn't working markdown issue related to markdown backend labels Sep 22, 2026

@ceberam ceberam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @kevin9327 for the well-described bug report and fix.
The direction of the fix (detecting self.in_table and keeping the span in the buffer) is right. There are, however, several correctness issues that must be resolved before merging, plus a deeper architectural point that changes the scope of the required solution (the issue 1 below). Please, let us know if you would like to address it in this PR, otherwise you could skip the point 1 and we will later handle it in a separate PR.

Blocking issues

1. The fix loses information that belongs in the document model

Beyond being a bug fix, this PR should be a fidelity improvement. Outside of a table, the backend emits a CodeItem for a CodeSpan, so a downstream HTML exporter can render `build` as <code>build</code>. Inside a table cell the current approach — concatenating the code span's raw text into the string buffer — throws that semantic away permanently: a TableCell with text="run build now" is indistinguishable from one that was always plain text.

The correct model already exists in the codebase: RichTableCell (used by the HTML, DOCX, and OpenDocument backends). It carries a ref pointing to a GroupItem whose children are the per-span TextItem / CodeItem nodes, so the full document structure is preserved. The fix should emit RichTableCell for any cell that contains at least one CodeSpan (and, while at it, for cells with bold, italic, or link spans — the same data loss applies to those today).

This requires replacing the md_table_buffer: list[str] per-row string accumulator with a structured per-cell accumulator that tracks spans with type information. When _close_table fires:

  • Cells whose span list is entirely plain text → TableCell (no change for the common case, no performance impact).
  • Cells with at least one code or formatted span → create a GroupItem parented to the TableItem, add each span as a child CodeItem or TextItem, then construct a RichTableCell(ref=group.get_ref(), text=plain_text, ...).

The _group_cell_elements helper in msword_backend.py and process_rich_table_cells in html_backend.py are good references for the grouping pattern.

2. Adjacent whitespace is silently dropped (regression introduced by this PR)

The RawText handler calls original_text.strip() before appending to the table buffer. For a cell like run \build` now`, Marko emits three inline nodes:

RawText  "| run "
CodeSpan "build"
RawText  " now | builds it |"

After stripping, the RawText segments become "| run" and "now | builds it |". The code span text is then concatenated without any separator, yielding "runbuildnow" instead of "run build now".

The test was written to accommodate this regression: lines 263–267 use in membership checks rather than an equality assertion, and the comment at line 263 explicitly says "spaces around the span may collapse". A regression test that knowingly accepts broken output is not a regression test.

Fixing this in the context of issue 1 above is straightforward: the structured cell accumulator records each RawText segment as-is (unstripped), and stripping happens only at final cell extraction — _split_table_row already calls cell.strip() on each cell boundary.

3. A pipe inside a code span breaks cell boundaries

A code span containing a literal | — e.g., `a | b` — should be part of the cell text, not a column separator. The current implementation appends the raw unescaped text (including the |) to the buffer; _split_table_row then splits on it, breaking the cell in two. The RawText table path avoids this problem by using _unescape_except_pipe so that HTML-entity-encoded pipes survive the split. A literal | arriving from a CodeSpan needs the same protection: before buffering, encode it as &#124; so _split_table_row does not treat it as a delimiter. This is handled naturally if the structured-accumulator approach from issue 1 is adopted, because cell boundaries are tracked structurally rather than by scanning for | in a flat string.

4. Inconsistent escaping and double-unescape

The RawText table path uses _unescape_except_pipe at write time, and _close_table applies a final unescape(cellval.strip()) on each cell. The new CodeSpan path calls plain unescape() at write time and the same unescape() fires again in _close_table, double-decoding the content. The HTML unescape function is idempotent for typical entities, but not for all sequences (e.g., &amp;amp; → &amp; → &). The two write paths should use the same escaping strategy, and the buffer contract (whether values are pre-escaped or raw) should be consistent and documented.


Non-blocking issues

5. Add the test case to the shared ground-truth fixture

The inline table with a code-span cell is a good example to include in tests/data/md/sources/inline_and_formatting.md, which already contains a table with bold and italic headers. Adding it there gives future contributors a visible input file and a machine-checked ground truth (both .md and .yaml) that verifies round-trip fidelity. The file is covered by test_convert_valid, which is in the yaml_filter set, so both the Markdown re-export and the full DoclingDocument structure are verified. The standalone test_convert_table_keeps_inline_code_spans test can then be kept as a focused regression guard or removed, depending on preference.

Important: regenerate the ground-truth files (DOCLING_GEN_TEST_DATA=1 uv run pytest) only after the fix is correct, so they reflect the proper output — including RichTableCell content — not the whitespace-collapsed intermediate state.

6. Inline comment narrates history rather than invariant

# Keep the span in the current row. Closing the table here used
# to split one GFM table into two and drop the text before the span.

The second and third lines describe what the old code did, which is not useful for a future maintainer deciding whether to modify this branch. Prefer something that describes the rule:

# A CodeSpan does not delimit cells; keep its content in the current buffer slot.

7. The _log.debug line is missing from the in_table branch

Every other inline element handler emits a debug log entry. The _log.debug(" - Code Span: %s", element.children) line should be moved to before the if self.in_table branch so it fires in both paths.

@djlongy

djlongy commented Sep 23, 2026

Copy link
Copy Markdown

Ran the current head (60b4ac7) against the table inputs collected in #3991. The code span now stays in the table (one TableItem), though the cell reads runbuildnow, which is review point 2.

The same close-the-table path still fires on other inline nodes, so these still split:

cell content result on this branch
SELECT A_B<br>, C_D<br>FROM T 2 tables, , C_D and FROM T | emitted as text
a<br/>b<br />c 2 tables, b and c | as text
CO<sub>2</sub> 3 tables, value cells detached
<kbd>Ctrl</kbd>+<kbd>C</kbd> 2 tables, cell empty
![i](x.png) cap 2 tables, cap | as text
<https://x.io> 2 tables, cell empty

With <br> in a header cell (\| Name<br>(unit) \| Value \|) the header becomes Name, the delimiter row is parsed as a data row and the first real data row is dropped. #3891 does not change this; its description says the table buffer path is untouched.

If it helps the structured-cell work from point 1: marko's GFM table elements (marko.ext.gfm.elements), registered on their own without the rest of the gfm extension, parse all of these as one table with typed cell children. Branch linked in #3991.

@ceberam

ceberam commented Sep 25, 2026

Copy link
Copy Markdown
Member

Sorry @djlongy , I did not understand how you would like to proceed. There are two options:

  • Focus this PR on the original issue (A GFM table cell that contains an inline code span used to close the table at the span). This would mean addressing my points 2-7.
  • Refactor the parsing of cells with emphasis with RichTableCell to preserve that information (point 1)

In my opinion, it would be easier to proceed with the first option to fix the broken table as soon as possible, and then tackle the refactoring to enhance the parsing of table cells in a separate PR.

@kevin9327

Copy link
Copy Markdown
Contributor Author

@ceberam Thanks for the thorough review. Going with the first option, as you suggest: this PR fixes the broken table (points 2 to 7); the RichTableCell representation (point 1) is left for a separate change.

  • 2, 3, 4: the table buffer now has one stated contract (comment next to _unescape_except_pipe): _close_table decodes each cell exactly once, a RawText keeps its whitespace and its pipe entities on the way in, and literal text such as a code span is entity-encoded on the way in with its pipes as &#124;. run \build` nowisrun build now; `a | b`stays one cell;`&`stays the literal&. The test asserts every cell of a table with those three rows (the in` checks are gone).
  • 5: inline_and_formatting.md gained a row with a code span, and its .md/.yaml ground truth was regenerated after the fix (DOCLING_GEN_TEST_DATA=1); the other ground-truth files are unchanged.
  • 6, 7: comment states the rule; the debug line runs in both paths.

@djlongy Thanks for running the corpus. The <br>, <sub>, <kbd>, image and autolink cases go through other inline handlers that still close the table; they are outside this change (the code-span path), and the structured-cell work would be the place to cover them together.

@kevin9327
kevin9327 force-pushed the fix/md-codespan-in-tables branch from 4590c89 to 5884d26 Compare September 28, 2026 10:06
@kevin9327

Copy link
Copy Markdown
Contributor Author

Rebased onto main (now 5884d26) to resolve the conflict with #4313 and #4371, and signed off both commits for DCO.

The conflict was in the RawText branch. #4313's backslash-escape handling is kept: a Literal still has its pipes spelled &#124; through _escape_pipes, and it now goes through the same whitespace-preserving _append_table_text path. _encode_table_literal reuses _escape_pipes instead of a second &#124; replacement. tests/test_backend_markdown.py passes (24), including the #4313 and #4371 cases. With md_backend.py from main, the two code-span tests still fail.

@ceberam ceberam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great, thanks for your contribution @kevin9327 🚀

dolfim-ibm
dolfim-ibm previously approved these changes Sep 28, 2026

@ceberam ceberam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@kevin9327 We had a rather long pipeline of PRs and we have merged them today. Unfortunately this has triggered some conflicts again on your PR. Could you please rebase it to main?

kevin9327 and others added 2 commits September 28, 2026 22:12
A CodeSpan in a table cell closed the table mid-row, so one GFM table
became two and the text before the span was dropped.

Signed-off-by: kevin9327 <5299031+kevin9327@users.noreply.github.com>
Review: a RawText segment was stripped before it joined the row, so
"run `build` now" became "runbuildnow"; a pipe inside a code span split
the cell; and the span's text was decoded once on the way in and again
in _close_table. The table buffer now has one contract: _close_table
decodes it exactly once, a RawText keeps its whitespace and its pipe
entities, and literal text such as a code span is entity-encoded with
its pipes as &docling-project#124;. The debug line fires in both paths, the comment
states the rule, the fixture gains a code-span row, and the test asserts
every cell, including a pipe and an entity inside a span.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: kevin9327 <5299031+kevin9327@users.noreply.github.com>
@kevin9327

Copy link
Copy Markdown
Contributor Author

Rebased onto main (d18232f). The only overlap was #4318, which also appends table-cell text in md_backend.py: the rebased branch keeps its original_text condition, so whitespace-only nodes still reach the cell, and uses this PR's _append_table_text helper. In inline_and_formatting.md both new table rows are kept, and the groundtruth was regenerated from the merged source; its diff against main is the same as before the rebase. tests/test_backend_markdown.py: 25 passed, ruff clean.

@ceberam
ceberam merged commit ae8db9a into docling-project:main Sep 29, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working markdown issue related to markdown backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants