fix(html): keep non-li children of lists in document order - #4440
morten-lagabote wants to merge 3 commits into
Conversation
|
✅ DCO Check Passed Thanks @morten-lagabote, all your commits are properly signed off. 🎉 |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
ceberam
left a comment
There was a problem hiding this comment.
Thanks @morten-lagabote for the careful investigation and for putting together a working fix. The bug is real and the content loss is a genuine problem worth solving.
After reviewing the PR carefully, I think the approach of nesting non-<li> children under the preceding list item is not the right choice for Docling, for the following reasons:
- The spec-compliant DOM does not nest the
<p>under the preceding<li>. The HTML is already invalid. When the browser's HTML parser is inside a<ul>element, it is in a state expecting only<li>(or script-supporting tags). When it encounters an unexpected start tag like<p>, the specification executes "foster parenting" and error recovery rules. It will likely force-close the<ul>early, render the paragraph outside of it, and then open a new hidden list for the remaining items, or place<p>as a sibling of it. So "browsers render it inside the list, below the preceding item" is not accurate for a spec-compliant parser. - In
DoclingDocument's schema, aGroupList(list group) only acceptsListItemor nestedGroupListchildren. Appending aTextItemas a child of aListItemdoes not violate the schema per se, but it does mean the interleaved paragraph becomes a sub-item of the list item before it, which may be fine in the examples you posted (e.g., Lovdata.no pages) but can be a semantic distortion for others. The paragraph "About the first item" is not logically part of "First item"; it is independent content that happens to be misplaced in the source HTML. - The DOCX contract (issue #3896) already established a consistent convention: an interleaved block that is not a list item forces the current list to close, the block is emitted at the parent level, and a new list opens afterwards. This is the semantically correct interpretation, and it is exactly what spec-compliant HTML parsers (and the HTML5 tree construction algorithm) would produce anyway.
Instead, I would suggest implementing the following behavior in _handle_list:
- Content before the first
<li>: your existingtakewhile/leadinglogic is correct and should be kept. - Non-
<li>, non-sublist children between list items: close the current list group, emit the node via_walk_nodesat the enclosing parent level, then open a new list group for the items that follow. If the list is ordered, initialize the new group's start counter from the runninglist_item_counterso that numbering is preserved. - Nested
<ul>/<ol>children: continue to handle these as proper sub-lists (the existing behavior here is fine).
The _walk_nodes helper you introduced is a useful building block and can remain as-is. The test you added is also a good skeleton, it would just need its assertions updated to match the new expected structure.
Thank you again for identifying the issue and for the clear reproduction case. We look forward to a revised version.
Children of <ul>/<ol> other than <li> are invalid HTML, but common in CMS output. They were dropped, or added directly to the list group in the case of a nested list, which broke the numbering of the following items. Content before the first <li> is now emitted before the list, and any other non-<li> child is nested under the preceding list item, as browsers render it. Resolves docling-project#4424 Signed-off-by: Morten Dæhli Aslesen <morten@lagabote.no>
…ng after them Follow the convention of the DOCX backend (docling-project#3896) for the content between list items: the list group is closed, the content is emitted at the parent level, and the following items open a new list group whose numbering continues from the running counter. Nested lists stay sub-lists of the preceding item. Signed-off-by: Morten Dæhli Aslesen <morten@lagabote.no>
29da179 to
f3d3656
Compare
|
Thanks for the review, @ceberam. I pushed the changes you suggested (f3d3656):
One note on point 1, for the record: in the HTML tree construction algorithm a |
_handle_list returns every item it adds at the current level, so the content emitted around a split list stays in a table cell. A child that adds nothing, like a <br>, no longer closes the list. Signed-off-by: Morten Dæhli Aslesen <morten@lagabote.no>
|
Two follow-ups from my own review, in the last commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@ceberam is this addressing your request? |
ceberam
left a comment
There was a problem hiding this comment.
Thanks @morten-lagabote for refactoring the PR according to the contract.
It fixes the issue and I don't have any objection with the current implementation.
It would however help having a visual example of this edge case, so readers can understand how the Docling tree should be and how a markdown export looks like. The example that you brought when describing the issue #4424 is simple yet very illustrative:
<ul>
<li>First item</li>
<p>A paragraph placed directly inside the list.</p>
<li>Second item</li>
</ul>
<p>After.</p>It could be good to add it to one of our ground-truth test files, with a proper disclaimer that this is not valid HTML. The file tests/data/html/sources/html_nested_block_in_list_item.html would be the perfect place to append it.
Children of
<ul>/<ol>other than<li>are invalid HTML, but common in CMS output._handle_listonly iterated theli/ul/olchildren:<p>,<table>and other children were dropped, and a<ul>/<ol>child was added directly to the list group (the workaround referring to docling-core#357), which broke the numbering of the following items._handle_listnow walks all the children in DOM order, following the convention of the DOCX backend for interleaved blocks (#3896):<li>is emitted before the list;<ul>/<ol>stays a sub-list of the preceding list item;<br>, does not close the list._handle_listreturns every item it adds at the current level, so the content emitted around a split list stays in its container (e.g. a table cell).In the Lovdata page of the issue, the 7 paragraphs that were dropped are now kept, each between the items it separates.
The loop of
_walkmoves to_walk_nodes, which walks a subset of the children of an element, so these children go through the same code path as any other content. The signature of_walkis unchanged, so subclasses that override it keep working.Unchanged: an
<li>without text creates no list item.No change in the ground truth.
Issue resolved by this Pull Request:
Resolves #4424
Checklist: