From b12987f820d99692a0a6c4a3619da18ae7c919fc Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 15 May 2026 00:34:00 +0200 Subject: [PATCH 1/6] fix: handle multi-text-node w:t elements in tracked changes (#9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit minidom can split a single element's text across multiple TEXT_NODE children — Word documents with smart quotes (U+2018/U+2019) have been reported to land in this state. The read paths in replace_text, suggest_deletion, _insert_text, and _parse_revision only consulted firstChild.data, so any text adjacent to a smart quote raised TextNotFoundError. The corresponding write paths assigned to firstChild.data, which silently corrupted the remaining sibling text nodes. Add a _set_node_text helper that consolidates children to a single TEXT_NODE, document _get_node_text, and route all firstChild.data text reads/writes through these helpers. Element-level firstChild traversal (unwrap, restore deletion, ins-insertion point lookup) is unchanged. Tests cover the buggy code paths directly (paragraph=None branches and the cross-boundary in-ins path) and round-trip a real .docx built with smart-quote content through open → edit → save → reopen. --- docx_editor/track_changes.py | 54 +++++--- tests/test_track_changes.py | 250 +++++++++++++++++++++++++++++++---- 2 files changed, 259 insertions(+), 45 deletions(-) diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index 38cc3d1..e5834e8 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -372,7 +372,7 @@ def _rewrite_insert_at(self, paragraph, text_map: TextMap, char_pos: int, text: ins_ancestor = self._find_ancestor(run, "w:ins") if ins_ancestor: node_text = self._get_node_text(last_pos.node) - last_pos.node.firstChild.data = node_text + text + self._set_node_text(last_pos.node, node_text + text) _set_xml_space_preserve(last_pos.node) return @@ -391,7 +391,7 @@ def _rewrite_insert_at(self, paragraph, text_map: TextMap, char_pos: int, text: if ins_ancestor: node_text = self._get_node_text(pos.node) offset = pos.offset_in_node - pos.node.firstChild.data = node_text[:offset] + text + node_text[offset:] + self._set_node_text(pos.node, node_text[:offset] + text + node_text[offset:]) _set_xml_space_preserve(pos.node) return @@ -544,7 +544,7 @@ def replace_text(self, find: str, replace_with: str, occurrence: int = 0, paragr raise RevisionError("Could not find parent w:r element") # Get the full text content - full_text = elem.firstChild.data if elem.firstChild else "" + full_text = self._get_node_text(elem) start_idx = full_text.find(find) if start_idx == -1: @@ -563,7 +563,7 @@ def replace_text(self, find: str, replace_with: str, occurrence: int = 0, paragr # Site A: If inside , edit text in-place (no del/ins wrappers) if self._find_ancestor(run, "w:ins"): - elem.firstChild.data = before_text + replace_with + after_text + self._set_node_text(elem, before_text + replace_with + after_text) _set_xml_space_preserve(elem) return -1 @@ -647,7 +647,7 @@ def suggest_deletion(self, text: str, occurrence: int = 0, paragraph: str | None raise RevisionError("Could not find parent w:r element") # Get the full text content - full_text = elem.firstChild.data if elem.firstChild else "" + full_text = self._get_node_text(elem) start_idx = full_text.find(text) if start_idx == -1: @@ -673,7 +673,7 @@ def suggest_deletion(self, text: str, occurrence: int = 0, paragraph: str | None if ins_ancestor: remaining = before_text + after_text if remaining: - elem.firstChild.data = remaining + self._set_node_text(elem, remaining) _set_xml_space_preserve(elem) else: # Entire w:t text removed — check if other w:t nodes exist @@ -735,13 +735,32 @@ def _get_non_text_children_xml(self, run) -> str: return "".join(parts) def _get_node_text(self, node) -> str: - """Get text content of a w:t node.""" + """Get text content of a w:t node by concatenating ALL child text nodes. + + minidom may store the text of a single ```` element across + multiple TEXT_NODE children — e.g. when Word documents contain + smart quotes (U+2018/U+2019). Reading only ``firstChild.data`` + would return just the first fragment (issue #9). + """ text = "" for child in node.childNodes: if child.nodeType == child.TEXT_NODE: text += child.data return text + def _set_node_text(self, node, text: str) -> None: + """Replace all text content of a w:t/w:delText element with ``text``. + + Removes every existing TEXT_NODE child and appends a single new one + carrying the full content. Necessary because assigning to + ``firstChild.data`` would leave any sibling text nodes behind, + corrupting the document when the element holds split text (issue #9). + """ + for child in list(node.childNodes): + if child.nodeType == child.TEXT_NODE: + node.removeChild(child) + node.appendChild(node.ownerDocument.createTextNode(text)) + def _build_cross_boundary_parts(self, match: TextMapMatch) -> list[tuple[Element, str, str, str, str, int]]: """Build per-node data for a cross-boundary match. @@ -1049,12 +1068,12 @@ def _remove_from_insertion(self, positions: list) -> None: # Other w:t nodes exist — remove just this w:t (and run if empty) self._remove_wt_and_maybe_run(first_node) elif not before_text: - first_node.firstChild.data = after_text + self._set_node_text(first_node, after_text) elif not after_text: - first_node.firstChild.data = before_text + self._set_node_text(first_node, before_text) else: # Middle split - first_node.firstChild.data = before_text + self._set_node_text(first_node, before_text) run = self._find_ancestor(first_node, "w:r") if ins_elem and run: rPr_xml = "" @@ -1068,13 +1087,13 @@ def _remove_from_insertion(self, positions: list) -> None: # remove intermediate nodes entirely. # Only remove the w:t node; remove the run only if no w:t children remain. if before: - first_node.firstChild.data = before + self._set_node_text(first_node, before) _set_xml_space_preserve(first_node) else: self._remove_wt_and_maybe_run(first_node) if after: - last_node.firstChild.data = after + self._set_node_text(last_node, after) _set_xml_space_preserve(last_node) else: self._remove_wt_and_maybe_run(last_node) @@ -1386,7 +1405,7 @@ def _insert_text( rPr_xml = rPr_elems[0].toxml() # Site C: If inside , edit text in-place (safe for multi-w:t) - full_text = elem.firstChild.data if elem.firstChild else "" + full_text = self._get_node_text(elem) anchor_idx = full_text.find(anchor) if anchor_idx == -1: @@ -1397,9 +1416,9 @@ def _insert_text( if self._find_ancestor(run, "w:ins"): if position == "before": - elem.firstChild.data = before_text + text + anchor + after_text + self._set_node_text(elem, before_text + text + anchor + after_text) else: - elem.firstChild.data = before_text + anchor + text + after_text + self._set_node_text(elem, before_text + anchor + text + after_text) _set_xml_space_preserve(elem) return -1 @@ -1528,10 +1547,7 @@ def _parse_revision(self, elem, rev_type: Literal["insertion", "deletion"]) -> R else: text_elems = elem.getElementsByTagName("w:delText") - text_parts = [] - for t_elem in text_elems: - if t_elem.firstChild: - text_parts.append(t_elem.firstChild.data) + text_parts = [self._get_node_text(t_elem) for t_elem in text_elems] return Revision( id=int(rev_id), diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index b49649a..555fcf4 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -11,6 +11,19 @@ from docx_editor.track_changes import Revision, RevisionManager, _escape_xml +def _attach_text_node(mock_elem, text): + """Configure a MagicMock to look like a w:t element with TEXT_NODE child. + + Mirrors the real minidom structure so methods iterating ``childNodes`` + and filtering by ``TEXT_NODE`` (e.g. ``_get_node_text``) work. + """ + mock_elem.firstChild = MagicMock() + mock_elem.firstChild.data = text + # Make nodeType == TEXT_NODE evaluate True without depending on actual ints. + mock_elem.firstChild.nodeType = mock_elem.firstChild.TEXT_NODE + mock_elem.childNodes = [mock_elem.firstChild] + + class TestTrackedReplace: """Tests for tracked text replacement.""" @@ -799,8 +812,7 @@ def get_attr(name): # Mock text element with content mock_text_elem = MagicMock() - mock_text_elem.firstChild = MagicMock() - mock_text_elem.firstChild.data = "test content" + _attach_text_node(mock_text_elem, "test content") mock_elem.getElementsByTagName.return_value = [mock_text_elem] @@ -958,8 +970,7 @@ def test_replace_text_parent_traversal_loop(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_intermediate - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "hello world" + _attach_text_node(mock_elem, "hello world") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -994,8 +1005,7 @@ def test_suggest_deletion_parent_traversal_loop(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_intermediate - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "hello world" + _attach_text_node(mock_elem, "hello world") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1030,7 +1040,7 @@ def test_insert_text_parent_traversal_loop(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_intermediate - mock_elem.firstChild.data = "anchor" + _attach_text_node(mock_elem, "anchor") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1064,8 +1074,7 @@ def test_replace_text_with_rpr(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "hello world" + _attach_text_node(mock_elem, "hello world") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1101,8 +1110,7 @@ def test_suggest_deletion_with_rpr(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "hello world" + _attach_text_node(mock_elem, "hello world") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1146,7 +1154,7 @@ def _get_by_tag(tag): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild.data = "anchor" + _attach_text_node(mock_elem, "anchor") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1178,8 +1186,7 @@ def test_replace_text_returns_minus_one_when_no_ins_found(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "hello" + _attach_text_node(mock_elem, "hello") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1208,8 +1215,7 @@ def test_suggest_deletion_returns_minus_one_when_no_del_found(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "hello" + _attach_text_node(mock_elem, "hello") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1238,6 +1244,7 @@ def test_insert_text_returns_minus_one_when_no_ins_found(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run + _attach_text_node(mock_elem, "anchor") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1266,9 +1273,8 @@ def test_replace_text_text_mismatch_raises_error(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() # The matcher found "world" in this node, but actual content is different - mock_elem.firstChild.data = "different content" + _attach_text_node(mock_elem, "different content") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1290,9 +1296,8 @@ def test_suggest_deletion_text_mismatch_raises_error(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() # The matcher found "world" in this node, but actual content is different - mock_elem.firstChild.data = "different content" + _attach_text_node(mock_elem, "different content") mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1314,8 +1319,7 @@ def test_replace_text_with_before_text_only(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "prefix hello" # "hello" is at end + _attach_text_node(mock_elem, "prefix hello") # "hello" is at end mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1347,8 +1351,7 @@ def test_suggest_deletion_with_before_text_only(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "prefix hello" # "hello" is at end + _attach_text_node(mock_elem, "prefix hello") # "hello" is at end mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1380,8 +1383,7 @@ def test_suggest_deletion_with_after_text_only(self): mock_elem.nodeName = "w:t" mock_elem.parentNode = mock_run - mock_elem.firstChild = MagicMock() - mock_elem.firstChild.data = "hello suffix" # "hello" is at start + _attach_text_node(mock_elem, "hello suffix") # "hello" is at start mock_editor.find_all_nodes.return_value = [mock_elem] @@ -1643,3 +1645,199 @@ def test_restore_deletion_converts_rsiddel_to_rsidr(self): assert len(r_elems) == 1 assert r_elems[0].getAttribute("w:rsidR") == "00112233" assert not r_elems[0].hasAttribute("w:rsidDel") + + +def _split_wt_text_nodes(doc, target_text, tag="w:t"): + """Reach into the DOM and split an element's TEXT_NODE into multiple siblings. + + Simulates the minidom multi-child-text-node state reported in issue #9 — + Word documents with smart quotes (U+2018/U+2019) can land in this state. + Returns the modified element. + """ + dom = doc._document_editor.dom + for wt in dom.getElementsByTagName(tag): + full = "".join(c.data for c in wt.childNodes if c.nodeType == c.TEXT_NODE) + if target_text not in full: + continue + while wt.firstChild: + wt.removeChild(wt.firstChild) + idx = full.find(target_text) + before, after = full[:idx], full[idx + len(target_text) :] + owner = wt.ownerDocument + if before: + wt.appendChild(owner.createTextNode(before)) + wt.appendChild(owner.createTextNode(target_text)) + if after: + wt.appendChild(owner.createTextNode(after)) + return wt + raise AssertionError(f"No {tag} containing {target_text!r}") + + +class TestMultiTextNodeWtElements: + """Issue #9: w:t elements with multiple TEXT_NODE children (smart-quote split).""" + + # The buggy paths in replace_text/suggest_deletion/_insert_text (lines 547, + # 650, 1389) are only reached when paragraph=None (via _get_nth_match). + # When paragraph= is passed, control routes through _replace_across_nodes + # which uses _get_node_text and avoids the bug. + + def test_replace_without_paragraph_arg_succeeds(self, clean_workspace): + doc = Document.open(clean_workspace) + wt = _split_wt_text_nodes(doc, "quick brown fox") + assert len(wt.childNodes) > 1 + doc._revision_manager.replace_text("quick brown fox", "slow red turtle") + paragraphs = doc.list_paragraphs() + assert any("slow red turtle" in p for p in paragraphs) + assert not any("quick brown fox" in p for p in paragraphs) + doc.close() + + def test_delete_without_paragraph_arg_succeeds(self, clean_workspace): + doc = Document.open(clean_workspace) + _split_wt_text_nodes(doc, "quick brown fox") + doc._revision_manager.suggest_deletion("quick brown fox") + paragraphs = doc.list_paragraphs() + assert not any("quick brown fox" in p for p in paragraphs) + doc.close() + + def test_insert_without_paragraph_arg_succeeds(self, clean_workspace): + doc = Document.open(clean_workspace) + ref = find_ref(doc, "lazy dog") + doc.insert_before("lazy dog", "INS_TARGET ", paragraph=ref) + _split_wt_text_nodes(doc, "INS_TARGET") + doc._revision_manager.insert_text_before("INS_TARGET", "X_") + paragraphs = doc.list_paragraphs() + assert any("X_INS_TARGET" in p for p in paragraphs) + doc.close() + + def test_replace_inside_ins_writes_full_text(self, clean_workspace): + # Hits _replace_across_nodes' "all inside ins" path which previously + # used firstChild.data assignment. + doc = Document.open(clean_workspace) + ref = find_ref(doc, "lazy dog") + ref = doc.insert_before("lazy dog", "INS_TARGET ", paragraph=ref) + _split_wt_text_nodes(doc, "INS_TARGET") + doc.replace("INS_TARGET", "REPLACED", paragraph=ref) + paragraphs = doc.list_paragraphs() + assert any("REPLACED" in p for p in paragraphs) + assert not any("INS_TARGET" in p for p in paragraphs) + doc.close() + + def test_list_revisions_with_multi_text_node_delText(self, clean_workspace): + doc = Document.open(clean_workspace) + ref = find_ref(doc, "quick brown fox") + doc.delete("quick brown fox", paragraph=ref) + + dom = doc._document_editor.dom + del_texts = dom.getElementsByTagName("w:delText") + assert del_texts, "expected at least one w:delText after delete" + elem = del_texts[0] + full = "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE) + while elem.firstChild: + elem.removeChild(elem.firstChild) + mid = len(full) // 2 + elem.appendChild(elem.ownerDocument.createTextNode(full[:mid])) + elem.appendChild(elem.ownerDocument.createTextNode(full[mid:])) + + revisions = doc.list_revisions() + deletions = [r for r in revisions if r.type == "deletion"] + assert deletions + assert deletions[0].text == full + doc.close() + + def test_save_load_roundtrip_after_multi_node_edit(self, clean_workspace, tmp_path): + doc = Document.open(clean_workspace) + _split_wt_text_nodes(doc, "quick brown fox") + doc._revision_manager.replace_text("quick brown fox", "slow red turtle") + out = tmp_path / "edited.docx" + doc.save(out) + doc.close() + + doc2 = Document.open(out) + paragraphs = doc2.list_paragraphs() + assert any("slow red turtle" in p for p in paragraphs) + assert not any("quick brown fox" in p for p in paragraphs) + doc2.close() + + +def _build_smart_quote_docx(simple_docx, dest): + """Build a real .docx containing smart-quote text in a single . + + Repacks ``simple_docx`` with the second paragraph rewritten to contain + U+2018/U+2019 smart quotes, simulating the structure reported in + GitHub issue #9. + """ + import shutil + + from docx_editor.ooxml.pack import pack_document + from docx_editor.ooxml.unpack import unpack_document + + work = dest.parent / "_smart_quote_build" + if work.exists(): + shutil.rmtree(work) + unpack_document(simple_docx, work) + doc_xml = work / "word" / "document.xml" + xml = doc_xml.read_text(encoding="utf-8") + # Replace "The quick brown fox..." paragraph's text with one carrying + # smart quotes. The surrounding ... structure + # mirrors what Word emits. + target = "The quick brown fox jumps over the lazy dog." + replacement = "‘Library Bookshelves’ are in all Libraries." + assert target in xml, "fixture assumption broken: simple.docx changed" + xml = xml.replace(target, replacement) + doc_xml.write_text(xml, encoding="utf-8") + pack_document(work, dest) + shutil.rmtree(work) + return dest + + +class TestSmartQuoteEndToEnd: + """End-to-end: real .docx with smart quotes, full open/edit/save/reopen.""" + + def test_replace_around_smart_quotes(self, simple_docx, tmp_path): + src = tmp_path / "with_smart_quotes.docx" + _build_smart_quote_docx(simple_docx, src) + + doc = Document.open(src, force_recreate=True) + try: + # Force the multi-text-node state on the smart-quote w:t so we + # exercise the codepath issue #9 describes regardless of how + # the local minidom build represents the parsed text. + _split_wt_text_nodes(doc, "Library Bookshelves") + + doc._revision_manager.replace_text("Library Bookshelves", "Reading Rooms") + out = tmp_path / "edited.docx" + doc.save(out) + finally: + doc.close() + + doc2 = Document.open(out, force_recreate=True) + try: + paragraphs = doc2.list_paragraphs() + joined = " ".join(paragraphs) + assert "Reading Rooms" in joined + assert "Library Bookshelves" not in joined + # Smart quotes must survive the edit + assert "‘" in joined and "’" in joined + finally: + doc2.close() + + def test_delete_around_smart_quotes(self, simple_docx, tmp_path): + src = tmp_path / "with_smart_quotes.docx" + _build_smart_quote_docx(simple_docx, src) + + doc = Document.open(src, force_recreate=True) + try: + _split_wt_text_nodes(doc, "Library Bookshelves") + doc._revision_manager.suggest_deletion("Library Bookshelves") + out = tmp_path / "edited.docx" + doc.save(out) + finally: + doc.close() + + doc2 = Document.open(out, force_recreate=True) + try: + joined = " ".join(doc2.list_paragraphs()) + assert "Library Bookshelves" not in joined + assert "‘" in joined and "’" in joined + finally: + doc2.close() From de912e89f0e548480df1ae5f5dac44e11892481b Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 15 May 2026 01:01:57 +0200 Subject: [PATCH 2/6] fix: extend issue #9 multi-text-node fix to comments and xml:space Code review (gemini reviewer) flagged that the same firstChild.data bug class exists outside track_changes.py: - xml_editor.add_xml_space_to_t: inspected only firstChild.data when deciding whether to add xml:space="preserve". For a split into multiple TEXT_NODE children, trailing/leading whitespace sitting in a later fragment was missed, so Word would collapse those spaces. - comments._parse_comment: appended only firstChild.data to text_parts when reading comment text. Comment bodies containing characters that trigger node splitting (e.g. smart quotes) were returned truncated. Both sites now concatenate all TEXT_NODE children of each , matching the approach already used by RevisionManager._get_node_text. Tests construct the split-text-node state directly and verify the read paths return the full content. --- docx_editor/comments.py | 7 ++++--- docx_editor/xml_editor.py | 14 ++++++++------ tests/test_comments.py | 32 ++++++++++++++++++++++++++++++++ tests/test_xml_editor.py | 24 ++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 9 deletions(-) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 181bb68..8522de4 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -412,11 +412,12 @@ def _parse_comment(self, comment_elem) -> Comment | None: except ValueError: date = None - # Extract text content from w:t elements + # Extract text content from w:t elements. Concatenate all TEXT_NODE + # children — minidom can split a single across multiple text + # nodes (issue #9), so firstChild.data alone would truncate. text_parts = [] for t_elem in comment_elem.getElementsByTagName("w:t"): - if t_elem.firstChild: - text_parts.append(t_elem.firstChild.data) + text_parts.append("".join(c.data for c in t_elem.childNodes if c.nodeType == c.TEXT_NODE)) return Comment( id=int(comment_id), diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index a67e2c3..aa51169 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -609,12 +609,14 @@ def add_comment_extensible_date(elem) -> None: elem.setAttribute("w16cex:dateUtc", timestamp) def add_xml_space_to_t(elem) -> None: - # Add xml:space="preserve" to w:t if text has leading/trailing whitespace - if elem.firstChild and elem.firstChild.nodeType == elem.firstChild.TEXT_NODE: - text = elem.firstChild.data - if text and (text[0].isspace() or text[-1].isspace()): - if not elem.hasAttribute("xml:space"): - elem.setAttribute("xml:space", "preserve") + # Add xml:space="preserve" to w:t if text has leading/trailing whitespace. + # Concatenate all TEXT_NODE children — minidom can split a single + # across multiple text nodes (issue #9), so firstChild.data + # alone would miss whitespace that sits in a later fragment. + text = "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE) + if text and (text[0].isspace() or text[-1].isspace()): + if not elem.hasAttribute("xml:space"): + elem.setAttribute("xml:space", "preserve") for node in nodes: if node.nodeType != node.ELEMENT_NODE: diff --git a/tests/test_comments.py b/tests/test_comments.py index 005ca9b..0be8d11 100644 --- a/tests/test_comments.py +++ b/tests/test_comments.py @@ -998,3 +998,35 @@ def test_add_comment_intermediate_parent_traversal(self, clean_workspace): assert isinstance(comment_id, int) doc.close() + + +class TestSplitTextNodeComment: + """Issue #9: comments with multi-TEXT_NODE w:t must round-trip full text.""" + + def test_list_comments_full_text_with_split_w_t(self, clean_workspace): + doc = Document.open(clean_workspace) + try: + doc.add_comment("fox", "Hello World") + except TextNotFoundError: + pytest.skip("Anchor text not found in document") + + editor = doc._comment_manager._get_editor(doc._comment_manager.comments_path) + wts = editor.dom.getElementsByTagName("w:t") + target = None + for wt in wts: + full = "".join(c.data for c in wt.childNodes if c.nodeType == c.TEXT_NODE) + if "Hello World" in full: + target = wt + break + assert target is not None, "comment w:t not found" + + while target.firstChild: + target.removeChild(target.firstChild) + owner = target.ownerDocument + target.appendChild(owner.createTextNode("Hello ")) + target.appendChild(owner.createTextNode("World")) + + comments = doc.list_comments() + texts = [c.text for c in comments] + assert "Hello World" in texts + doc.close() diff --git a/tests/test_xml_editor.py b/tests/test_xml_editor.py index 9a2365a..6c8f7fa 100644 --- a/tests/test_xml_editor.py +++ b/tests/test_xml_editor.py @@ -436,6 +436,30 @@ def test_inject_xml_space_for_whitespace(self, docx_xml_file): new_t = nodes[0].getElementsByTagName("w:t")[0] assert new_t.getAttribute("xml:space") == "preserve" + def test_inject_xml_space_for_split_text_nodes(self, docx_xml_file): + """Issue #9: whitespace in a non-first TEXT_NODE must still trigger preserve. + + Simulates a w:t parsed into multiple TEXT_NODE children (as happens with + smart quotes on some minidom builds): the leading non-whitespace fragment + is followed by a trailing space fragment. Pre-fix this lost the preserve + attribute because only firstChild.data was inspected. + """ + editor = DocxXMLEditor(docx_xml_file, rsid="00AABBCC", author="Test") + p_elem = editor.get_node("w:p", contains="Hello") + + nodes = editor.insert_after(p_elem, "placeholder") + new_t = nodes[0].getElementsByTagName("w:t")[0] + # Replace the single TEXT_NODE with two siblings: "word" + " " (trailing space) + while new_t.firstChild: + new_t.removeChild(new_t.firstChild) + owner = new_t.ownerDocument + new_t.appendChild(owner.createTextNode("word")) + new_t.appendChild(owner.createTextNode(" ")) + # Re-run injection on the wrapped run + editor._inject_attributes_to_nodes([nodes[0]]) + + assert new_t.getAttribute("xml:space") == "preserve" + def test_inject_tracked_change_attrs(self, docx_xml_file): """Test w:ins gets proper attributes.""" editor = DocxXMLEditor(docx_xml_file, rsid="00AABBCC", author="TestAuthor") From a8c8df5d5747efaa4c6f35296270c5c698e60f21 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 15 May 2026 01:21:31 +0200 Subject: [PATCH 3/6] fix: preserve split-text-node whitespace in pack and find_all_nodes (#9) Adversarial review iteration 2 surfaced two more second-order consequences of issue #9 (minidom can split a single across multiple TEXT_NODE children). - ooxml.pack.condense_xml only skipped elements with tag suffix ":t". Tracked deletions (w:delText) and field instructions (w:instrText) were unprotected, so a whitespace-only TEXT_NODE created by a split would be stripped on save, corrupting tracked changes that contain spaces near smart quotes. Extend the suffix check to cover both. - XMLEditor._get_element_text unconditionally dropped any whitespace- only TEXT_NODE. That's fine for inter-element pretty-print whitespace, but it also dropped the lone-space fragment of a split like ["Library", " ", "Bookshelves"], silently breaking the `contains=` search used by get_node/find_all_nodes. Only skip a whitespace TEXT_NODE when it has no meaningful-text TEXT_NODE sibling. --- docx_editor/ooxml/pack.py | 6 ++-- docx_editor/xml_editor.py | 18 ++++++++---- tests/test_ooxml.py | 62 +++++++++++++++++++++++++++++++++++++++ tests/test_xml_editor.py | 18 ++++++++++++ 4 files changed, 97 insertions(+), 7 deletions(-) diff --git a/docx_editor/ooxml/pack.py b/docx_editor/ooxml/pack.py index 8256333..ddc34b0 100644 --- a/docx_editor/ooxml/pack.py +++ b/docx_editor/ooxml/pack.py @@ -120,8 +120,10 @@ def condense_xml(xml_file: Path) -> None: # Process each element to remove whitespace and comments for element in dom.getElementsByTagName("*"): - # Skip w:t elements and their processing (preserve text content) - if element.tagName.endswith(":t"): + # Skip text-bearing OOXML elements: w:t, w:delText, w:instrText (and their + # namespace variants). Their content is significant — including whitespace + # fragments that minidom may split off as their own TEXT_NODE (issue #9). + if element.tagName.endswith((":t", ":delText", ":instrText")): continue # Remove whitespace-only text nodes and comment nodes diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index aa51169..bc72cfa 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -335,20 +335,28 @@ def find_all_nodes( def _get_element_text(self, elem) -> str: """Recursively extract all text content from an element. - Skips text nodes that contain only whitespace (spaces, tabs, newlines). + Skips whitespace-only TEXT_NODEs only when they sit between element + siblings (i.e. pretty-print indentation). When an element holds only + TEXT_NODE children — as a ```` split across multiple text nodes + by minidom does (issue #9) — every fragment is preserved, including + ones that happen to be pure whitespace. Args: elem: DOM element to extract text from Returns: - Concatenated text from all non-whitespace text nodes + Concatenated text content """ + # A whitespace-only TEXT_NODE is "real content" when it sits alongside + # a non-whitespace TEXT_NODE sibling (the smart-quote split case). + # Otherwise it's pretty-print indentation and should be skipped. + has_meaningful_text_sibling = any(n.nodeType == n.TEXT_NODE and n.data.strip() for n in elem.childNodes) text_parts = [] for node in elem.childNodes: if node.nodeType == node.TEXT_NODE: - # Skip whitespace-only text nodes (XML formatting) - if node.data.strip(): - text_parts.append(node.data) + if not node.data.strip() and not has_meaningful_text_sibling: + continue + text_parts.append(node.data) elif node.nodeType == node.ELEMENT_NODE: text_parts.append(self._get_element_text(node)) return "".join(text_parts) diff --git a/tests/test_ooxml.py b/tests/test_ooxml.py index 2e317c6..acfbdfc 100644 --- a/tests/test_ooxml.py +++ b/tests/test_ooxml.py @@ -90,3 +90,65 @@ def test_pack_excludes_root_meta_json(self, simple_docx, temp_dir): names = zf.namelist() assert "meta.json" not in names assert "word/meta.json" in names + + +def _read_doc_xml(docx_path): + """Open a packed .docx and return its word/document.xml content as text.""" + import zipfile + + with zipfile.ZipFile(docx_path) as zf: + return zf.read("word/document.xml").decode("utf-8") + + +class TestCondenseXmlPreservesTextContent: + """Issue #9: condense_xml must not strip whitespace fragments from text-bearing + OOXML elements (w:t, w:delText, w:instrText). Pre-fix only :t was protected.""" + + def _write_xml(self, base_dir, body_xml): + """Replace word/document.xml's body with body_xml, returning the dir.""" + doc_xml = base_dir / "word" / "document.xml" + original = doc_xml.read_text(encoding="utf-8") + head, _, tail = original.partition("") + _, _, end = tail.partition("") + new = f"{head}{body_xml}{end}" + doc_xml.write_text(new, encoding="utf-8") + return base_dir + + def test_condense_preserves_whitespace_in_delText(self, simple_docx, temp_dir): + unpack_document(simple_docx, temp_dir / "unpacked") + body = ( + '' + 'foo bar' + "" + ) + self._write_xml(temp_dir / "unpacked", body) + + # Inject a multi-TEXT_NODE state into the w:delText, including a lone space. + import defusedxml.minidom + + doc_xml = temp_dir / "unpacked" / "word" / "document.xml" + dom = defusedxml.minidom.parseString(doc_xml.read_text(encoding="utf-8")) + delText = dom.getElementsByTagName("w:delText")[0] + while delText.firstChild: + delText.removeChild(delText.firstChild) + owner = delText.ownerDocument + delText.appendChild(owner.createTextNode("foo")) + delText.appendChild(owner.createTextNode(" ")) + delText.appendChild(owner.createTextNode("bar")) + doc_xml.write_bytes(dom.toxml(encoding="UTF-8")) + + out = temp_dir / "out.docx" + pack_document(temp_dir / "unpacked", out) + repacked = _read_doc_xml(out) + # The space must survive condensation + assert "foo bar" in repacked or "foo HYPERLINK ' + self._write_xml(temp_dir / "unpacked", body) + + out = temp_dir / "out.docx" + pack_document(temp_dir / "unpacked", out) + repacked = _read_doc_xml(out) + assert " HYPERLINK " in repacked diff --git a/tests/test_xml_editor.py b/tests/test_xml_editor.py index 6c8f7fa..726269c 100644 --- a/tests/test_xml_editor.py +++ b/tests/test_xml_editor.py @@ -245,6 +245,24 @@ def test_get_element_text_nested(self, tmp_path): text = editor._get_element_text(outer) assert text == "Hello World" + def test_get_element_text_preserves_split_whitespace(self, tmp_path): + """Issue #9: a whitespace fragment between two non-whitespace TEXT_NODEs + must be preserved (it's content, not pretty-print indentation).""" + xml_content = 'placeholder' + xml_path = tmp_path / "split.xml" + xml_path.write_text(xml_content) + editor = XMLEditor(xml_path) + item = editor.dom.getElementsByTagName("item")[0] + # Replace the single TEXT_NODE with three TEXT_NODE siblings + while item.firstChild: + item.removeChild(item.firstChild) + owner = item.ownerDocument + item.appendChild(owner.createTextNode("Library")) + item.appendChild(owner.createTextNode(" ")) + item.appendChild(owner.createTextNode("Bookshelves")) + + assert editor._get_element_text(item) == "Library Bookshelves" + class TestXMLEditorGetNextRid: """Tests for XMLEditor.get_next_rid method.""" From 8396aafe3297765787cdf98614344a8f96808194 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 15 May 2026 13:46:51 +0200 Subject: [PATCH 4/6] test: tighten condense_xml delText assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The disjunct in test_condense_preserves_whitespace_in_delText was a near-tautology — the right-hand side evaluated True whenever the space had been stripped, so the test would have silently passed against a regression. The stricter form fails fast if the space disappears from the repacked archive. Skipped on this iteration: - xml_editor._get_element_text edge case where a lone whitespace-only TEXT_NODE child is dropped (pre-existing behavior, not introduced by this PR; xml:space="preserve" guard would be the proper fix) - pytest.skip pattern in TestSplitTextNodeComment (matches the rest of test_comments.py — consistent with project style) --- tests/test_ooxml.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_ooxml.py b/tests/test_ooxml.py index acfbdfc..60071b3 100644 --- a/tests/test_ooxml.py +++ b/tests/test_ooxml.py @@ -141,7 +141,7 @@ def test_condense_preserves_whitespace_in_delText(self, simple_docx, temp_dir): pack_document(temp_dir / "unpacked", out) repacked = _read_doc_xml(out) # The space must survive condensation - assert "foo bar" in repacked or "foo Date: Fri, 15 May 2026 13:57:03 +0200 Subject: [PATCH 5/6] fix: preserve whitespace in _remove_from_insertion single-node branch (#9) CodeRabbit flagged an asymmetry: the multi-node branch of _remove_from_insertion pairs every _set_node_text() write with _set_xml_space_preserve(), but the single-node branch did not. If the surviving text after a deletion has leading or trailing whitespace (common when only part of an insertion is removed), Word would silently collapse it on render. Aligning the single-node branch with the multi-node pattern. --- docx_editor/track_changes.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index e5834e8..f7ddff6 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -1069,11 +1069,14 @@ def _remove_from_insertion(self, positions: list) -> None: self._remove_wt_and_maybe_run(first_node) elif not before_text: self._set_node_text(first_node, after_text) + _set_xml_space_preserve(first_node) elif not after_text: self._set_node_text(first_node, before_text) + _set_xml_space_preserve(first_node) else: # Middle split self._set_node_text(first_node, before_text) + _set_xml_space_preserve(first_node) run = self._find_ancestor(first_node, "w:r") if ins_elem and run: rPr_xml = "" From b0111a00fad83dd9cb0a29a5544010dd1ec36380 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Fri, 15 May 2026 21:21:53 +0200 Subject: [PATCH 6/6] refactor: polish issue #9 fix (DRY, xml:space guard, focused unit test) Addresses the three LOW items the previous reviewer pass flagged but that had been deferred: * Extract the "concat all TEXT_NODE children" pattern into a single module-level helper `xml_editor.get_text_node_data`. Replace the four inline copies (build_text_map, add_xml_space_to_t, _parse_comment, RevisionManager._get_node_text) with calls into it. The method on RevisionManager stays as a thin delegate so call sites and any external subclasses remain unchanged. * `_get_element_text` now treats a whitespace-only TEXT_NODE child as content when its parent carries `xml:space="preserve"`, not only when it sits alongside a meaningful-text sibling. Closes the edge case where a lone-space ` ` was incorrectly returned as the empty string by `contains=` search. * New focused unit test for `_set_node_text` asserts the contract directly: starting from a multi-TEXT_NODE state, ends with exactly one TEXT_NODE carrying the new content. Catches future "simplifications" that re-introduce the `firstChild.data =` pattern without an integration regression. * New test covering the `xml:space="preserve"` branch of `_get_element_text`. --- docx_editor/comments.py | 9 ++----- docx_editor/track_changes.py | 13 +++------- docx_editor/xml_editor.py | 47 +++++++++++++++++++++--------------- tests/test_track_changes.py | 17 +++++++++++++ tests/test_xml_editor.py | 16 ++++++++++++ 5 files changed, 67 insertions(+), 35 deletions(-) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 8522de4..6d40b11 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -10,7 +10,7 @@ from pathlib import Path from .exceptions import CommentError, TextNotFoundError -from .xml_editor import DocxXMLEditor, _generate_hex_id +from .xml_editor import DocxXMLEditor, _generate_hex_id, get_text_node_data # Path to template files TEMPLATE_DIR = Path(__file__).parent / "ooxml" / "templates" @@ -412,12 +412,7 @@ def _parse_comment(self, comment_elem) -> Comment | None: except ValueError: date = None - # Extract text content from w:t elements. Concatenate all TEXT_NODE - # children — minidom can split a single across multiple text - # nodes (issue #9), so firstChild.data alone would truncate. - text_parts = [] - for t_elem in comment_elem.getElementsByTagName("w:t"): - text_parts.append("".join(c.data for c in t_elem.childNodes if c.nodeType == c.TEXT_NODE)) + text_parts = [get_text_node_data(t_elem) for t_elem in comment_elem.getElementsByTagName("w:t")] return Comment( id=int(comment_id), diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index f7ddff6..5d9262d 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -27,6 +27,7 @@ build_text_map, compute_paragraph_hash, find_in_text_map, + get_text_node_data, ) @@ -737,16 +738,10 @@ def _get_non_text_children_xml(self, run) -> str: def _get_node_text(self, node) -> str: """Get text content of a w:t node by concatenating ALL child text nodes. - minidom may store the text of a single ```` element across - multiple TEXT_NODE children — e.g. when Word documents contain - smart quotes (U+2018/U+2019). Reading only ``firstChild.data`` - would return just the first fragment (issue #9). + Thin wrapper around :func:`get_text_node_data` — kept as a method so + existing call sites (and external subclasses) don't have to change. """ - text = "" - for child in node.childNodes: - if child.nodeType == child.TEXT_NODE: - text += child.data - return text + return get_text_node_data(node) def _set_node_text(self, node, text: str) -> None: """Replace all text content of a w:t/w:delText element with ``text``. diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index bc72cfa..75aa383 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -131,6 +131,20 @@ def _is_inside_element(node, tag_name: str) -> bool: return False +def get_text_node_data(elem) -> str: + """Concatenate all direct TEXT_NODE children of ``elem`` into one string. + + minidom can split a single ````'s text across multiple TEXT_NODE + children — reproducibly when smart quotes (U+2018/U+2019) are present + (issue #9). Anywhere code naively read ``elem.firstChild.data`` would + only see the first fragment. This helper returns the complete string. + + Does NOT recurse into element children. For recursive extraction across + a subtree see ``XMLEditor._get_element_text``. + """ + return "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE) + + def build_text_map(paragraph) -> TextMap: """Build a text map for a paragraph element. @@ -150,11 +164,7 @@ def build_text_map(paragraph) -> TextMap: continue inside_ins = _is_inside_element(node, "w:ins") - # Get text content from the text node - node_text = "" - for child in node.childNodes: - if child.nodeType == child.TEXT_NODE: - node_text += child.data + node_text = get_text_node_data(node) for i, char in enumerate(node_text): text_chars.append(char) @@ -335,11 +345,14 @@ def find_all_nodes( def _get_element_text(self, elem) -> str: """Recursively extract all text content from an element. - Skips whitespace-only TEXT_NODEs only when they sit between element - siblings (i.e. pretty-print indentation). When an element holds only - TEXT_NODE children — as a ```` split across multiple text nodes - by minidom does (issue #9) — every fragment is preserved, including - ones that happen to be pure whitespace. + A whitespace-only TEXT_NODE child is treated as content (rather than + pretty-print indentation) when any of: + * ``elem`` carries ``xml:space="preserve"`` — the document explicitly + marks its whitespace significant. + * Another TEXT_NODE sibling has non-whitespace content — the lone + whitespace is a fragment of a split text node (issue #9). + Otherwise the whitespace TEXT_NODE is discarded as inter-element + formatting. Args: elem: DOM element to extract text from @@ -347,14 +360,13 @@ def _get_element_text(self, elem) -> str: Returns: Concatenated text content """ - # A whitespace-only TEXT_NODE is "real content" when it sits alongside - # a non-whitespace TEXT_NODE sibling (the smart-quote split case). - # Otherwise it's pretty-print indentation and should be skipped. - has_meaningful_text_sibling = any(n.nodeType == n.TEXT_NODE and n.data.strip() for n in elem.childNodes) + preserve_whitespace = elem.getAttribute("xml:space") == "preserve" or any( + n.nodeType == n.TEXT_NODE and n.data.strip() for n in elem.childNodes + ) text_parts = [] for node in elem.childNodes: if node.nodeType == node.TEXT_NODE: - if not node.data.strip() and not has_meaningful_text_sibling: + if not node.data.strip() and not preserve_whitespace: continue text_parts.append(node.data) elif node.nodeType == node.ELEMENT_NODE: @@ -618,10 +630,7 @@ def add_comment_extensible_date(elem) -> None: def add_xml_space_to_t(elem) -> None: # Add xml:space="preserve" to w:t if text has leading/trailing whitespace. - # Concatenate all TEXT_NODE children — minidom can split a single - # across multiple text nodes (issue #9), so firstChild.data - # alone would miss whitespace that sits in a later fragment. - text = "".join(c.data for c in elem.childNodes if c.nodeType == c.TEXT_NODE) + text = get_text_node_data(elem) if text and (text[0].isspace() or text[-1].isspace()): if not elem.hasAttribute("xml:space"): elem.setAttribute("xml:space", "preserve") diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index 555fcf4..bff0fa0 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1681,6 +1681,23 @@ class TestMultiTextNodeWtElements: # When paragraph= is passed, control routes through _replace_across_nodes # which uses _get_node_text and avoids the bug. + def test_set_node_text_consolidates_split_nodes(self, clean_workspace): + """Direct contract test for _set_node_text: starts from a multi-TEXT_NODE + state, ends with exactly one TEXT_NODE carrying the full new content. + Guards against future "simplifications" that would re-introduce the + firstChild.data assignment pattern.""" + doc = Document.open(clean_workspace) + wt = _split_wt_text_nodes(doc, "quick brown fox") + text_nodes_before = [c for c in wt.childNodes if c.nodeType == c.TEXT_NODE] + assert len(text_nodes_before) > 1 + + doc._revision_manager._set_node_text(wt, "consolidated") + + text_nodes_after = [c for c in wt.childNodes if c.nodeType == c.TEXT_NODE] + assert len(text_nodes_after) == 1 + assert text_nodes_after[0].data == "consolidated" + doc.close() + def test_replace_without_paragraph_arg_succeeds(self, clean_workspace): doc = Document.open(clean_workspace) wt = _split_wt_text_nodes(doc, "quick brown fox") diff --git a/tests/test_xml_editor.py b/tests/test_xml_editor.py index 726269c..ded030b 100644 --- a/tests/test_xml_editor.py +++ b/tests/test_xml_editor.py @@ -263,6 +263,22 @@ def test_get_element_text_preserves_split_whitespace(self, tmp_path): assert editor._get_element_text(item) == "Library Bookshelves" + def test_get_element_text_honors_xml_space_preserve(self, tmp_path): + """A lone whitespace TEXT_NODE inside an element marked + xml:space="preserve" is significant content and must be returned. + + Without the attribute it would be treated as pretty-print indent and + stripped — matching the pre-existing behavior.""" + xml_content = ' ' + xml_path = tmp_path / "preserve.xml" + xml_path.write_text(xml_content) + editor = XMLEditor(xml_path) + + keep = editor.dom.getElementsByTagName("keep")[0] + drop = editor.dom.getElementsByTagName("drop")[0] + assert editor._get_element_text(keep) == " " + assert editor._get_element_text(drop) == "" + class TestXMLEditorGetNextRid: """Tests for XMLEditor.get_next_rid method."""