ODT: round-5 review fixes

ROADMAP.md

@@ -140,7 +140,7 @@ Ranked, to beta (detail and remaining milestones in [docs/roadmap.md](docs/roadm
 ## Progress log
 
 - 2026-11-04: OpenDocument files are shown in one of two styles (gap 19): Microsoft Word style, the default, or LibreOffice style (File › Options › Advanced › OpenDocument files; `file.options` `{"odf": {"preset": "word"}}` or `"libreoffice"`; `WORDCRAFT_ODF=word` or `WORDCRAFT_ODF=libreoffice`). Custom stays for one's own value per case; `wordcraft`, the earlier id of Word's style, is still taken.
-- 2026-11-04: ODT element audit (gap 19, [docs/odt-element-audit.md](docs/odt-element-audit.md)): every element of the ODF 1.3 schema checked against 365 files and a document of our own for each one no file held; all open and save, and a save without edits keeps them (password, number, time and grid form controls included). Over the 220 files from other applications, 177 round-trip pixel-identical, 201 with the same text and 218 with the same page count (the latest measurement, after the groups and form-control work).
+- 2026-11-04: ODT element audit (gap 19, [docs/odt-element-audit.md](docs/odt-element-audit.md)): every element of the ODF 1.3 schema checked against 365 files and a document of our own for each one no file held that a text document can hold (the `presentation:`, `anim:` and `db:` elements only presentations and databases hold stay unseen); all open and save, and a save without edits keeps them (password, number, time and grid form controls included). Over the 220 files from other applications, 177 round-trip pixel-identical, 201 with the same text and 218 with the same page count (the latest measurement, after the groups and form-control work).
 - 2026-11-04: ODT checked against 220 files from other applications (Microsoft Office, OpenOffice.org, StarOffice, Calligra, Lotus Symphony; local only): every file LibreOffice opens opens again after a WordCraft save (a manifest entry was missing), 166 round-trip pixel-identical and 194 with the same text at the time (an earlier measurement; the element-audit entry above has the current figures); hidden sections stay hidden and unedited lists are written as read (gap 19).
 - 2026-11-03: ODT paragraphs holding bookmarks, notes, formulas and objects, and the page-anchored frames before a paragraph, are written as read while unchanged (gap 19); styles of different families sharing a name are kept apart. Over the corpus 123 of 133 files round-trip pixel-identical (118), 132 with the same text, and the attributes and values a save loses fall from 539 to under 315.
 - 2026-11-03: ODT paragraphs showing text boxes and shapes holding text are written as read while that text is unchanged (gap 19). Over the corpus 111 of 133 files now round-trip pixel-identical (103), the round-trip difference falls from 44k to 39k pixels, and the element kinds a save loses fall from 16 to 8.

crates/doc/src/section.rs

@@ -733,8 +733,9 @@ pub struct SectionProps {
     /// Distance from the page edge to the header / footer.
     pub header: f32,
     pub footer: f32,
-    /// The space kept between a header too tall for the top margin and the body, and between
-    /// the body and such a footer, points (ODF's header and footer spacing; Word keeps none).
+    /// The space kept between a header and the body, and between the body and a footer, points
+    /// (ODF's header and footer spacing; Word keeps none). The margin already holds the band and
+    /// its spacing, so it only moves the body once content and spacing reach past the margin.
     #[serde(skip_serializing_if = "is_zero")]
     pub header_gap: f32,
     #[serde(skip_serializing_if = "is_zero")]

crates/engine/src/cmd/citations/generated.rs

@@ -71,7 +71,7 @@ const MAX_WRAPPED: usize = 10_000;
 /// A generated list (index, bibliography, table of figures or authorities) held as a complex
 /// field that starts at body block `i` and runs over its entries: its field code and the block
 /// it ends in. `None` for anything else (a TOC has its own update).
-fn wrapped_list(s: &Session, i: usize) -> Option<(String, usize)> {
+fn wrapped_list(s: &Session, i: usize) -> Option<(String, usize, usize)> {
     let p = s.doc.body.get(i)?.as_para()?;
     let (k, off, instr) = p.objects_with_offsets().enumerate().find_map(|(k, (off, o))| match o {
         InlineObject::FieldStart { instr, .. } => {
@@ -90,13 +90,13 @@ fn wrapped_list(s: &Session, i: usize) -> Option<(String, usize)> {
     let mut depth = 0usize;
     for (j, b) in s.doc.body.iter().enumerate().skip(i).take(MAX_WRAPPED) {
         let Some(q) = b.as_para() else { continue };
-        for o in q.objects.iter().skip(if j == i { k } else { 0 }) {
+        for (at, o) in q.objects_with_offsets().skip(if j == i { k } else { 0 }) {
             match o {
                 InlineObject::FieldStart { .. } => depth += 1,
                 InlineObject::FieldEnd => {
                     depth = depth.saturating_sub(1);
                     if depth == 0 {
-                        return Some((instr, j));
+                        return Some((instr, j, at + wordcraft_doc::para::OBJ.len_utf8()));
                     }
                 }
                 _ => {}
@@ -195,18 +195,39 @@ pub fn update_generated(s: &mut Session) -> Result<(), CmdError> {
     while i < s.doc.body.len() {
         // A list held as a field around its entries (as Word and ODT files hold them): made the
         // field's own paragraph, as WordCraft inserts one, its old entries rebuilt below.
-        if let Some((instr, last)) = wrapped_list(s, i) {
-            let mut p = Paragraph::default();
+        // What follows its closing field end, and its first paragraph's properties and section
+        // break, are the document's and stay.
+        if let Some((instr, last, end)) = wrapped_list(s, i) {
+            // The entries between its first and last paragraphs go.
+            for _ in i.saturating_add(1)..last {
+                s.doc.remove_block(StoryRef::Body, &Path::top(i + 1))?;
+            }
+            // Its first paragraph keeps only the field; its last (or, ending in its first, the
+            // same) loses what is up to and through the field end, and goes when nothing is left
+            // of it and it ends no section.
+            let first_end = if last > i {
+                let q = s.doc.para_mut(StoryRef::Body, &Path::top(i + 1))?;
+                q.delete(0, end)?;
+                if q.is_empty() && q.section.is_none() {
+                    s.doc.remove_block(StoryRef::Body, &Path::top(i + 1))?;
+                }
+                s.doc.para_mut(StoryRef::Body, &Path::top(i))?.len()
+            } else {
+                end
+            };
+            let p = s.doc.para_mut(StoryRef::Body, &Path::top(i))?;
+            p.delete(0, first_end)?;
             p.insert_object(
                 0,
                 InlineObject::Field { instr, result: String::new(), locked: false, dirty: false, code: false, form: String::new() },
                 &Default::default(),
             )?;
-            for _ in i..last {
-                s.doc.remove_block(StoryRef::Body, &Path::top(i + 1))?;
-            }
-            if let Some(b) = s.doc.body.get_mut(i) {
-                *b = wordcraft_doc::para_block(p);
+            // What followed the field end in its own paragraph comes after the list, in a
+            // paragraph of its own that takes the section break.
+            let field = wordcraft_doc::para::OBJ.len_utf8();
+            if p.len() > field {
+                let rest = p.split_off(field)?;
+                s.doc.insert_block(StoryRef::Body, &Path::top(i + 1), Block::Para(rest))?;
             }
         }
         let kind = s.doc.body.get(i).and_then(|b| b.as_para()).and_then(list_kind);
@@ -410,6 +431,59 @@ mod tests {
             p.insert_object(n, InlineObject::FieldEnd, &props).unwrap();
         }
         let s = Session::new(d);
-        assert_eq!(wrapped_list(&s, 0), Some(("BIBLIOGRAPHY".to_string(), 1)));
+        assert_eq!(wrapped_list(&s, 0).map(|(instr, last, _)| (instr, last)), Some(("BIBLIOGRAPHY".to_string(), 1)));
+    }
+
+    /// A wrapped list as a session holds it: the field starting paragraph `first`'s text, ending
+    /// in paragraph `last` at byte `at`.
+    fn wrapped(text: &str, first: usize, last: usize, at: usize) -> Session {
+        let mut d = Document::from_text(text);
+        let props = CharProps::default();
+        if let Ok(p) = d.para_mut(StoryRef::Body, &Path::top(last)) {
+            p.insert_object(at, InlineObject::FieldEnd, &props).unwrap();
+        }
+        if let Ok(p) = d.para_mut(StoryRef::Body, &Path::top(first)) {
+            p.insert_object(0, InlineObject::FieldStart { instr: "BIBLIOGRAPHY".into(), locked: false, dirty: false }, &props).unwrap();
+        }
+        Session::new(d)
+    }
+
+    fn texts(s: &Session) -> Vec<String> {
+        s.doc.body.iter().filter_map(|b| b.as_para()).map(|p| p.plain_text()).collect()
+    }
+
+    /// What follows a wrapped list's closing field end, in its last paragraph or in its only one,
+    /// stays, after the list; the last paragraph's section break stays too.
+    #[test]
+    fn what_follows_a_wrapped_lists_end_stays() {
+        // Ending in a later paragraph, with text after the end and a section break.
+        let mut s = wrapped("Old one\nOld two\nOld three. After\nBelow", 0, 2, "Old three.".len());
+        if let Ok(p) = s.doc.para_mut(StoryRef::Body, &Path::top(2)) {
+            p.section = Some(Box::default());
+        }
+        if let Ok(p) = s.doc.para_mut(StoryRef::Body, &Path::top(0)) {
+            p.props.indent_left = Some(9.0);
+        }
+        update_generated(&mut s).unwrap();
+        let t = texts(&s);
+        assert!(t.iter().all(|t| !t.contains("Old")), "{t:?}");
+        assert!(t.iter().any(|t| t == " After"), "{t:?}");
+        assert_eq!(t.last().map(String::as_str), Some("Below"));
+        let after = s.doc.body.iter().filter_map(|b| b.as_para()).find(|p| p.plain_text() == " After");
+        assert!(after.is_some_and(|p| p.section.is_some()), "the section break went");
+        let first = s.doc.body.first().and_then(|b| b.as_para());
+        assert!(first.is_some_and(|p| p.props.indent_left == Some(9.0) && list_kind(p).is_some()), "{first:?}");
+        // An empty last paragraph without a section break goes.
+        let mut s = wrapped("Old one\nOld two\nBelow", 0, 1, "Old two".len());
+        update_generated(&mut s).unwrap();
+        // (The field's paragraph shows nothing: there are no sources to list.)
+        assert_eq!(texts(&s), ["", "Below"]);
+        // Ending in its own paragraph: what follows comes after the list.
+        let mut s = wrapped("Old entry. Then this\nBelow", 0, 0, "Old entry.".len());
+        update_generated(&mut s).unwrap();
+        let t = texts(&s);
+        assert!(t.first().is_some_and(|t| !t.contains("Old")), "{t:?}");
+        assert_eq!(t.get(1).map(String::as_str), Some(" Then this"), "{t:?}");
+        assert_eq!(t.last().map(String::as_str), Some("Below"));
     }
 }

crates/formats/src/odt/body/notes.rs

@@ -236,6 +236,8 @@ impl Body<'_> {
         let Some(n) = self.note.take() else { return };
         self.tables.truncate(n.tables);
         self.cells.truncate(n.cells);
+        self.formulas.truncate(n.cells);
+        self.cell_tops.truncate(n.cells);
         self.lists_saved.truncate(n.lists_saved);
         self.containers.truncate(n.containers + 1);
         let blocks = if self.containers.len() > n.containers { self.containers.pop().unwrap_or_default() } else { Vec::new() };
@@ -346,3 +348,34 @@ impl Body<'_> {
         self.xml_space_end = false;
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::odt::{Body, Pages, Styles, events};
+
+    /// The cell stacks after reading `xml`: (cells, formulas, cell tops).
+    fn stacks_after(xml: &str) -> (usize, usize, usize) {
+        let (styles, pages) = (Styles::default(), Pages::default());
+        let mut none = |_: &str| None;
+        let mut b = Body::new(&styles, &pages, &mut none);
+        b.in_text = true;
+        events(xml.as_bytes(), |ev| b.feed(ev));
+        (b.cells.len(), b.formulas.len(), b.cell_tops.len())
+    }
+
+    /// A formula cell a note or a deletion leaves open (malformed) goes with it: nothing of it is
+    /// left for the cell round the note, or for the next cell read.
+    #[test]
+    fn a_cell_a_note_or_deletion_leaves_open_takes_its_formula_with_it() {
+        // The cell, its row and its table end with tags that are none of theirs.
+        let open_cell = r#"<table:table><table:table-row><table:table-cell table:formula="ooow:1+1" office:value-type="float"><text:p>2</text:p></wcx:x></wcx:x></wcx:x>"#;
+        let in_note = format!(
+            "<office:text><table:table><table:table-row><table:table-cell><text:p>a<text:note text:note-class=\"footnote\"><text:note-citation>1</text:note-citation><text:note-body>{open_cell}</text:note-body></text:note></text:p></table:table-cell></table:table-row></table:table></office:text>"
+        );
+        assert_eq!(stacks_after(&in_note), (0, 0, 0));
+        let in_deletion = format!(
+            "<office:text><text:tracked-changes><text:changed-region text:id=\"c1\"><text:deletion><office:change-info><dc:creator>A</dc:creator></office:change-info>{open_cell}</text:deletion></text:changed-region></text:tracked-changes><text:p>b</text:p></office:text>"
+        );
+        assert_eq!(stacks_after(&in_deletion), (0, 0, 0));
+    }
+}

crates/formats/src/odt/body/tracked.rs

@@ -106,6 +106,8 @@ impl Body<'_> {
         }
         self.tables.truncate(d.tables);
         self.cells.truncate(d.cells);
+        self.formulas.truncate(d.cells);
+        self.cell_tops.truncate(d.cells);
         self.lists_saved.truncate(d.lists_saved);
         self.fmt.truncate(d.fmt);
         self.lists = d.lists;

crates/formats/src/odt/import.rs

@@ -583,6 +583,33 @@ fn kept_markup(blocks: &[FBlock], depth: usize, out: &mut Vec<(String, std::sync
     }
 }
 
+/// Drop the markup kept as read (of paragraphs and tables, as [`kept_markup`] finds it) that
+/// links to one of the package files in `missing`.
+fn unkeep_linking(blocks: &mut [FBlock], depth: usize, missing: &std::collections::BTreeSet<String>) {
+    if missing.is_empty() {
+        return;
+    }
+    for b in blocks {
+        match b {
+            FBlock::Para(p) => {
+                if p.kept.as_deref().is_some_and(|x| super::kept::links_to(x, missing)) {
+                    p.kept = None;
+                }
+            }
+            FBlock::Table(t) => {
+                if t.kept.as_deref().is_some_and(|x| super::kept::links_to(x, missing)) {
+                    t.kept = None;
+                }
+                if t.kept.is_none() && depth < crate::model::MAX_DEPTH {
+                    for c in t.rows.iter_mut().flatten() {
+                        unkeep_linking(&mut c.blocks, depth + 1, missing);
+                    }
+                }
+            }
+        }
+    }
+}
+
 /// What [`parse_tracked`] reads besides the flow.
 struct Parsed {
     flow: Flow,
@@ -795,7 +822,13 @@ fn parse_tracked(bytes: &[u8], paper: (f32, f32), profile: &OdfProfile) -> Resul
     for b in parts.blocks_mut() {
         kept_markup(b, 0, &mut linking);
     }
-    let kept_files = super::kept::files(&linking, &entries, &mut fetch);
+    let (kept_files, missing) = super::kept::files(&linking, &entries, &mut fetch);
+    // Markup linking to a file that couldn't be read is written from the model: as read, it
+    // would point at a file the saved package hasn't got.
+    unkeep_linking(&mut blocks, 0, &missing);
+    for b in parts.blocks_mut() {
+        unkeep_linking(b, 0, &missing);
+    }
     // Their manifest entries, and their folders' (an object's says what it is).
     for (path, _) in &kept_files {
         let dir = path.rsplit_once('/').map(|(d, _)| format!("{d}/"));

crates/formats/src/odt/kept.rs

@@ -324,24 +324,51 @@ const MAX_FILES: usize = 1000;
 /// The package files `defs` refer to (`xlink:href` to a path inside the package, as a style's
 /// fill picture; to a folder, as an embedded object in a header: the files in it), read with
 /// `fetch` (`entries`: the package's paths): a definition written as read needs them.
-pub(super) fn files(defs: &[(String, Arc<str>)], entries: &[String], fetch: &mut dyn FnMut(&str) -> Option<Vec<u8>>) -> Vec<(String, Vec<u8>)> {
+/// Also the paths linked to that are in the package but couldn't be read (over the picture
+/// budget, unreadable): markup linking to one can't be written as read.
+pub(super) fn files(
+    defs: &[(String, Arc<str>)],
+    entries: &[String],
+    fetch: &mut dyn FnMut(&str) -> Option<Vec<u8>>,
+) -> (Vec<(String, Vec<u8>)>, BTreeSet<String>) {
     let mut paths = BTreeSet::new();
     for (_, xml) in defs {
         hrefs(xml, &mut paths);
     }
     let mut out = Vec::new();
+    let mut missing = BTreeSet::new();
     for p in paths {
         if entries.contains(&p) {
-            out.extend(fetch(&p).map(|b| (p, b)));
+            match fetch(&p) {
+                Some(b) => out.push((p, b)),
+                None => {
+                    missing.insert(p);
+                }
+            }
         } else {
             // A folder (an object): the files in it.
             let dir = format!("{}/", p.trim_end_matches('/'));
             for e in entries.iter().filter(|e| e.starts_with(&dir) && !e.ends_with('/')).take(MAX_FILES) {
-                out.extend(fetch(e).map(|b| (e.clone(), b)));
+                match fetch(e) {
+                    Some(b) => out.push((e.clone(), b)),
+                    None => {
+                        missing.insert(p.clone());
+                    }
+                }
             }
         }
     }
-    out
+    (out, missing)
+}
+
+/// Whether markup `xml` links to one of the package files in `missing` (see [`files`]).
+pub(super) fn links_to(xml: &str, missing: &BTreeSet<String>) -> bool {
+    if missing.is_empty() {
+        return false;
+    }
+    let mut paths = BTreeSet::new();
+    hrefs(xml, &mut paths);
+    paths.iter().any(|p| missing.contains(p))
 }
 
 /// The package files `xml` links to (`xlink:href` to a path inside the package), added to

crates/formats/src/odt/package.rs

@@ -1057,7 +1057,7 @@ fn export_as(doc: &Document, mime: &str) -> Result<Vec<u8>, String> {
     let scripts = kept.and_then(|k| k.defs.get("top:office:scripts")).map(|d| d.to_string()).unwrap_or_default();
     let soft = soft_page_breaks(kept, &body);
     let content = format!(
-        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<office:document-content {NS}>{scripts}{}<office:body><office:text{soft}>\n{}{tracked}{var_decls}{user_decls}{decls}{other_decls}{body}</office:text></office:body></office:document-content>\n",
+        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<office:document-content {NS}>{scripts}{}<office:body><office:text{soft}>\n{}{tracked}{var_decls}{decls}{user_decls}{other_decls}{body}</office:text></office:body></office:document-content>\n",
         w.automatic_styles(),
         // The file's forms as read, with the document's controls.
         super::kept::merge_forms(kept.and_then(|k| k.defs.get("forms")).map(|d| &**d), super::forms::forms_xml(&w.forms, &hidden_controls(doc)))
@@ -1208,7 +1208,9 @@ fn page_borders(b: Option<wordcraft_doc::props::Borders>, margins: [f32; 4]) ->
     let mut attrs = String::new();
     for (i, (side, edge)) in [("top", b.top), ("bottom", b.bottom), ("left", b.left), ("right", b.right)].into_iter().enumerate() {
         let (Some(e), Some(m)) = (edge.filter(|e| e.is_visible()), out.get_mut(i)) else { continue };
-        let at = if e.space.is_finite() { e.space.clamp(0.0, *m) } else { 0.0 };
+        // A hostile margin (negative or not a number) leaves no room, so clamp cannot panic.
+        let room = if m.is_finite() { m.max(0.0) } else { 0.0 };
+        let at = if e.space.is_finite() { e.space.clamp(0.0, room) } else { 0.0 };
         let pad = (*m - at - e.width).max(0.0);
         *m = at;
         attrs.push_str(&format!(" fo:border-{side}=\"{}\" fo:padding-{side}=\"{}\"", super::writer::odf_border(Some(e)), pt(pad)));
@@ -1422,6 +1424,21 @@ fn font_kind(family: &str) -> (&'static str, &'static str) {
 
 #[cfg(test)]
 mod setting_tests {
+    /// A page border round a page whose margin is negative or not a number is written at no
+    /// distance, instead of panicking.
+    #[test]
+    fn a_page_border_round_a_hostile_margin_is_written_at_no_distance() {
+        let mut edge = wordcraft_doc::props::Border::single(1.0);
+        edge.space = 12.0;
+        let b = wordcraft_doc::props::Borders::all(edge);
+        let (attrs, out) = super::page_borders(Some(b), [-10.0, f32::NAN, 36.0, 5.0]);
+        assert!(attrs.contains("fo:border-top="), "{attrs}");
+        assert_eq!(out[0], 0.0);
+        assert_eq!(out[1], 0.0);
+        assert_eq!(out[2], 12.0);
+        assert_eq!(out[3], 5.0);
+    }
+
     /// A self-closed setting is replaced whole, leaving the item after it as it was.
     #[test]
     fn a_self_closed_setting_is_replaced_whole() {

crates/formats/src/odt/tests_more/lossless.rs

@@ -438,6 +438,16 @@ fn content_of(bytes: &[u8]) -> String {
     s
 }
 
+/// The declarations at the start of the text are written in the schema's order: variables,
+/// sequences, then user fields.
+#[test]
+fn the_text_declarations_are_written_in_the_schemas_order() {
+    let c = content_of(&export(&import(&package(&DECLS)).unwrap()).unwrap());
+    let at = |el: &str| c.find(el).unwrap_or_else(|| panic!("no {el} in {c}"));
+    assert!(at("<text:variable-decls") < at("<text:sequence-decls"), "{c}");
+    assert!(at("<text:sequence-decls") < at("<text:user-field-decls"), "{c}");
+}
+
 #[test]
 fn a_section_in_a_text_box_closes_in_it_whatever_is_edited() {
     // The paragraph closing the box's section edited (its wrapper gone): the section still
@@ -486,6 +496,23 @@ fn a_picture_kept_as_read_is_written_with_its_paragraph_only() {
     assert!(!content_of(&saved).contains("dot.png"));
 }
 
+/// A picture that can't be read (its entry is corrupt) isn't pointed at by markup kept as read:
+/// what showed it is written from the model, and the saved file links to nothing it hasn't got.
+#[test]
+fn markup_kept_as_read_never_links_to_a_picture_that_could_not_be_read() {
+    let mut bytes = package(&DRAWINGS);
+    let at = bytes.windows(DOT.len()).position(|w| w == DOT).unwrap();
+    // The first copy is `Pictures/dot.png`'s: one byte changed fails its checksum.
+    bytes[at + 20] ^= 0xff;
+    let d = import(&bytes).unwrap();
+    let saved = export(&d).unwrap();
+    let c = content_of(&saved);
+    let mut z = zip::ZipArchive::new(std::io::Cursor::new(saved.clone())).unwrap();
+    assert!(!c.contains("Pictures/dot.png") || z.by_name("Pictures/dot.png").is_ok(), "{c}");
+    // The text round it stays.
+    assert!(c.contains("A picture"), "{c}");
+}
+
 #[test]
 fn a_paragraph_with_comments_is_written_as_read_while_they_are() {
     let d = import(&package(&COMMENTS)).unwrap();