Fix TextNotFoundError on smart quotes (split TEXT_NODE in w:t) (#9)

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,11 +412,7 @@ def _parse_comment(self, comment_elem) -> Comment | None:
         except ValueError:
             date = None
 
-        # Extract text content from w:t elements
-        text_parts = []
-        for t_elem in comment_elem.getElementsByTagName("w:t"):
-            if t_elem.firstChild:
-                text_parts.append(t_elem.firstChild.data)
+        text_parts = [get_text_node_data(t_elem) for t_elem in comment_elem.getElementsByTagName("w:t")]
 
         return Comment(
             id=int(comment_id),

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

docx_editor/track_changes.py

@@ -27,6 +27,7 @@
     build_text_map,
     compute_paragraph_hash,
     find_in_text_map,
+    get_text_node_data,
 )
 
 
@@ -372,7 +373,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 +392,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 +545,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 +564,7 @@ def replace_text(self, find: str, replace_with: str, occurrence: int = 0, paragr
 
         # Site A: If inside <w:ins>, 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 +648,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 +674,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,12 +736,25 @@ 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."""
-        text = ""
-        for child in node.childNodes:
+        """Get text content of a w:t node by concatenating ALL child text nodes.
+
+        Thin wrapper around :func:`get_text_node_data` — kept as a method so
+        existing call sites (and external subclasses) don't have to change.
+        """
+        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``.
+
+        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:
-                text += child.data
-        return text
+                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 +1063,15 @@ 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)
+                _set_xml_space_preserve(first_node)
             elif not after_text:
-                first_node.firstChild.data = before_text
+                self._set_node_text(first_node, before_text)
+                _set_xml_space_preserve(first_node)
             else:
                 # Middle split
-                first_node.firstChild.data = before_text
+                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 = ""
@@ -1068,13 +1085,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 +1403,7 @@ def _insert_text(
             rPr_xml = rPr_elems[0].toxml()
 
         # Site C: If inside <w:ins>, 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 +1414,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 +1545,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),

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 ``<w:t>``'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,20 +345,30 @@ 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).
+        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
 
         Returns:
-            Concatenated text from all non-whitespace text nodes
+            Concatenated text content
         """
+        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:
-                # Skip whitespace-only text nodes (XML formatting)
-                if node.data.strip():
-                    text_parts.append(node.data)
+                if not node.data.strip() and not preserve_whitespace:
+                    continue
+                text_parts.append(node.data)
             elif node.nodeType == node.ELEMENT_NODE:
                 text_parts.append(self._get_element_text(node))
         return "".join(text_parts)
@@ -609,12 +629,11 @@ 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.
+            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")
 
         for node in nodes:
             if node.nodeType != node.ELEMENT_NODE:

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()

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("<w:body>")
+        _, _, end = tail.partition("</w:body>")
+        new = f"{head}<w:body>{body_xml}</w:body>{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 = (
+            '<w:p><w:del w:id="1" w:author="Test" w:date="2024-01-01T00:00:00Z">'
+            '<w:r><w:delText xml:space="preserve">foo bar</w:delText></w:r>'
+            "</w:del></w:p>"
+        )
+        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
+
+    def test_condense_preserves_whitespace_in_instrText(self, simple_docx, temp_dir):
+        unpack_document(simple_docx, temp_dir / "unpacked")
+        body = '<w:p><w:r><w:instrText xml:space="preserve"> HYPERLINK </w:instrText></w:r></w:p>'
+        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