From 487a7ccb86b3db818fb3b417d4a8c4bf6897e119 Mon Sep 17 00:00:00 2001 From: luisvaca Date: Thu, 15 Oct 2026 17:58:19 -0600 Subject: [PATCH 1/4] #2289: comments in headers, footers, notes and text boxes are listed, get balloons and are reached by Next/Previous comment_list scanned the body only, so a comment anchored anywhere else was listed with no anchor: no balloon beside its text, Go to did nothing, and Next/Previous Comment never reached it. It now scans every story except comment text, body first and then each part in Pos order, which is the order nav_comment relies on. Reuses tools::all_stories. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 2 + crates/engine/src/cmd/review.rs | 100 ++++++++++++++++++++++++++++++-- crates/engine/src/cmd/tools.rs | 2 +- 3 files changed, 97 insertions(+), 7 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 39675b4fe..894ba0aa4 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -234,6 +234,7 @@ Imported Word 97–2003 `.doc`/`.dot` files now ask for a supported DOCX target | 2026-10-14 | Page-break-only list paragraphs no longer print stray bullets or numbers. The next item keeps its number, matching the reference export. | | 2026-10-12 | Backspace at the start of the paragraph after a table nested in a cell moves into the nested table's last cell, no longer into the last cell of the table around it | | 2026-10-14 | Grouped `flowChartOr` shapes draw their circle and crossing spokes, as in Word | +| 2026-10-15 | Comments in headers, footers, notes and text boxes are listed with their place in the document, get a balloon beside their text, and Next, Previous and Go to reach them, after the body's comments (#2289) | | 2026-10-14 | Deleting a comment removes its anchors from headers, footers, notes and text boxes too, not just the body, and deletes the whole thread below it, replies to replies included; no anchor or reply is left pointing at a deleted comment (#1345) | | 2026-10-14 | Legacy DOC DIB conversion checks the BMP header and copy before allocation, uses the caller's decode allowance, and caps PNG output while encoding; a BMP fallback is kept when it fits. A generated 1-pixel regression covers 57/58/512-byte limits. Resource-exhaustion consequences remain unmeasured. Related: #1334 and #1554 | | 2026-10-15 | Draw: Ink to Shape makes two-stroke arrows and arrows with a closed triangle head. A line plus a separate head stroke (an open V or a small closed triangle, drawn after the line or before it) whose apex sits at one of the line's ends becomes one straight arrow connector, and the head stroke goes, in one undo step; this works while drawing with Ink to Shape on and on lasso-selected strokes with Convert to Shapes. A shaft with a small triangle drawn round its tip in the same stroke is an arrow too; a triangle without a shaft stays a triangle. Our own design (Word for Mac has no Ink to Shape). | @@ -583,6 +584,7 @@ Footer page-table height and body wrapping are corrected. Other pagination gaps | 2026-10-14 | trivial | Field instructions spanning paragraphs no longer add blank lines during pagination; empty result paragraphs still take a line. | | 2026-10-14 | trivial | A list paragraph containing only a page break has no visible label; it still advances the list counter. Ordinary empty list items keep their label. | | 2026-10-14 | trivial | Floating-table overlap checks use visible line content; empty line-break lines no longer move a table | +| 2026-10-15 | trivial | Progress log: comments outside the body in the list, balloons and Next/Previous (#2289) | | 2026-10-14 | trivial | Progress log: deleting a comment leaves no dangling anchors or replies (#1345) | | 2026-10-14 | minor | Legacy DOC DIB conversion byte limits only; related #1334/#1554, stage and readiness estimates unchanged | | 2026-10-15 | trivial | Progress log: Ink to Shape two-stroke and closed-head arrows | diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index 904bd8ddc..d54a1be84 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -285,14 +285,22 @@ fn delete_comment(s: &mut Session, v: &Value) -> CmdResult { Ok(json!({"deleted": ids})) } -/// Comments in document order with their anchors. +/// Comments in document order with their anchors: the body's, then those in headers, footers, +/// notes and text boxes, story by story (#2289). That is `Pos` order, which Next and Previous +/// rely on. Comment text itself isn't scanned. pub fn comment_list(s: &Session) -> Vec<(u32, Option)> { let mut found: Vec<(u32, Option)> = Vec::new(); - for path in s.doc.para_paths(StoryRef::Body) { - let Some(p) = s.doc.para(StoryRef::Body, &path) else { continue }; - for o in p.object_offsets() { - if let Some(InlineObject::CommentStart { id }) = p.object_at(o) { - found.push((*id, Some(Pos { story: StoryRef::Body, path: path.clone(), off: o }))); + let stories = super::tools::all_stories(&s.doc).into_iter().filter(|st| match st { + StoryRef::Body => true, + StoryRef::Part(id) => s.doc.parts.get(id).is_some_and(|p| p.kind != wordcraft_doc::PartKind::Comment), + }); + for story in stories { + for path in s.doc.para_paths(story) { + let Some(p) = s.doc.para(story, &path) else { continue }; + for o in p.object_offsets() { + if let Some(InlineObject::CommentStart { id }) = p.object_at(o) { + found.push((*id, Some(Pos { story, path: path.clone(), off: o }))); + } } } } @@ -1747,6 +1755,86 @@ mod tests { assert!(started.elapsed() < std::time::Duration::from_secs(5), "took {:?}", started.elapsed()); } + /// A body comment, then comments in a header, a footer and a text box, each on its story's + /// whole text. Returns their ids and stories in that order. + fn comments_in_every_story(s: &mut Session) -> Vec<(u64, wordcraft_doc::StoryRef)> { + use wordcraft_doc::StoryRef; + s.view.proofing = false; + s.run("document.setText", &json!({"text": "Body text with a comment on it."})).unwrap(); + s.run("select.text", &json!({"text": "comment"})).unwrap(); + let mut out = vec![(s.run("review.newComment", &json!({"text": "body"})).unwrap()["id"].as_u64().unwrap(), StoryRef::Body)]; + for (cmd, text) in [("insert.header", "header"), ("insert.footer", "footer")] { + s.run("insert.closeHeader", &json!({})).unwrap(); + let story = StoryRef::Part(s.run(cmd, &json!({"text": text})).unwrap()["story"].as_u64().unwrap() as u32); + s.run("select.all", &json!({})).unwrap(); + out.push((s.run("review.newComment", &json!({"text": text})).unwrap()["id"].as_u64().unwrap(), story)); + } + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run("caret.docEnd", &json!({})).unwrap(); + s.run("insert.textBox", &json!({"text": "boxed"})).unwrap(); + let story = s.sel.focus.story; + s.run("select.all", &json!({})).unwrap(); + out.push((s.run("review.newComment", &json!({"text": "text box"})).unwrap()["id"].as_u64().unwrap(), story)); + s.run("caret.docStart", &json!({})).unwrap(); + out + } + + /// #2289: comments in headers, footers and text boxes are listed with their anchors, after + /// the body's, rather than with no anchor. + #[test] + fn comments_outside_the_body_are_listed_with_their_anchors() { + let mut s = Session::new(Document::new()); + let want = comments_in_every_story(&mut s); + let listed = s.run("review.comments", &json!({})).unwrap(); + let got: Vec<(u64, wordcraft_doc::StoryRef)> = listed + .as_array() + .unwrap() + .iter() + .map(|c| (c["id"].as_u64().unwrap(), serde_json::from_value(c["anchor"]["story"].clone()).expect("every comment has an anchor"))) + .collect(); + assert_eq!(got, want); + } + + /// #2289: Next and Previous Comment go through every story's comments and wrap around. + #[test] + fn next_and_previous_comment_reach_comments_outside_the_body() { + let mut s = Session::new(Document::new()); + let want: Vec = comments_in_every_story(&mut s).into_iter().map(|(_, st)| st).collect(); + let mut forward = Vec::new(); + for _ in 0..=want.len() { + s.run("review.nextComment", &json!({})).unwrap(); + forward.push(s.sel.focus.story); + } + let mut wrapped = want.clone(); + wrapped.push(want[0]); + assert_eq!(forward, wrapped, "Next visits each story's comment, then wraps to the first"); + let mut back = Vec::new(); + for _ in 0..want.len() { + s.run("review.previousComment", &json!({})).unwrap(); + back.push(s.sel.focus.story); + } + let mut rev = want.clone(); + rev.reverse(); + assert_eq!(back, rev, "Previous from the first comment wraps to the last"); + } + + /// Text inside a comment isn't an anchor: a comment marker in a comment's own story is not + /// listed as that comment's place in the document. + #[test] + fn comment_list_skips_comment_text() { + let mut s = Session::new(Document::new()); + s.view.proofing = false; + s.run("document.setText", &json!({"text": "hello"})).unwrap(); + s.run("select.all", &json!({})).unwrap(); + let id = s.run("review.newComment", &json!({"text": "note"})).unwrap()["id"].as_u64().unwrap() as u32; + let part = s.doc.comments.get(&id).unwrap().part; + let at = wordcraft_doc::Pos { story: wordcraft_doc::StoryRef::Part(part), path: wordcraft_doc::Path::top(0), off: 0 }; + s.doc.insert_object(&at, wordcraft_doc::InlineObject::CommentStart { id }, &Default::default()).unwrap(); + let list = super::comment_list(&s); + assert_eq!(list.iter().filter(|(x, _)| *x == id).count(), 1); + assert_eq!(list.first().and_then(|(_, p)| p.as_ref()).map(|p| p.story), Some(wordcraft_doc::StoryRef::Body)); + } + #[test] fn rtf_tracked_changes_accept_and_reject_after_opening_and_round_trip() { // Gap 19: an RTF file's tracked insertions and deletions open as tracked changes, also diff --git a/crates/engine/src/cmd/tools.rs b/crates/engine/src/cmd/tools.rs index b02408dd2..76243df5b 100644 --- a/crates/engine/src/cmd/tools.rs +++ b/crates/engine/src/cmd/tools.rs @@ -1586,7 +1586,7 @@ impl Hidden { } /// Every story of the document: the body, then headers, footers, notes, comments and text boxes. -fn all_stories(doc: &Document) -> Vec { +pub(crate) fn all_stories(doc: &Document) -> Vec { std::iter::once(StoryRef::Body).chain(doc.parts.keys().map(|k| StoryRef::Part(*k))).collect() } From 80f6ba6a25f8565a7e2f5380bef1dd1bd4df5e8f Mon Sep 17 00:00:00 2001 From: luisvaca Date: Thu, 15 Oct 2026 19:36:18 -0600 Subject: [PATCH 2/4] #2289: comment navigation follows the stories a reader sees, in reading order Review of the first commit found, and ui_shot runs confirmed, three bugs in it: - Next Comment moved the caret into a removed header or a deleted footnote's text: a story nothing shows, so typing went nowhere visible. - Next visited headers and footers by part id (creation order), so a footer made first came before the header. - Comments in footnotes were claimed but untested. Document::shown_stories lists the stories a reader sees, in reading order: the body with the notes and text boxes it shows, then the headers and footers the sections use with theirs. It reuses the reachable walk that counted_stories uses. comment_list scans those stories (and returns at once when the document has no comments: the balloons ask every frame), and nav_comment orders by a story's place in that list instead of by Pos. tools::all_stories is private again. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 2 +- crates/doc/src/lib.rs | 39 +++++++- crates/engine/src/cmd/review.rs | 158 ++++++++++++++++++++++---------- crates/engine/src/cmd/tools.rs | 2 +- 4 files changed, 146 insertions(+), 55 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 894ba0aa4..c721920a1 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -234,7 +234,7 @@ Imported Word 97–2003 `.doc`/`.dot` files now ask for a supported DOCX target | 2026-10-14 | Page-break-only list paragraphs no longer print stray bullets or numbers. The next item keeps its number, matching the reference export. | | 2026-10-12 | Backspace at the start of the paragraph after a table nested in a cell moves into the nested table's last cell, no longer into the last cell of the table around it | | 2026-10-14 | Grouped `flowChartOr` shapes draw their circle and crossing spokes, as in Word | -| 2026-10-15 | Comments in headers, footers, notes and text boxes are listed with their place in the document, get a balloon beside their text, and Next, Previous and Go to reach them, after the body's comments (#2289) | +| 2026-10-15 | Comments in headers, footers, notes and text boxes are listed with their place in the document, get a balloon beside their text, and Next, Previous and Go to reach them in reading order: the body with its notes and text boxes, then headers and footers. A comment left in a removed header or a deleted note's text isn't navigated to (#2289) | | 2026-10-14 | Deleting a comment removes its anchors from headers, footers, notes and text boxes too, not just the body, and deletes the whole thread below it, replies to replies included; no anchor or reply is left pointing at a deleted comment (#1345) | | 2026-10-14 | Legacy DOC DIB conversion checks the BMP header and copy before allocation, uses the caller's decode allowance, and caps PNG output while encoding; a BMP fallback is kept when it fits. A generated 1-pixel regression covers 57/58/512-byte limits. Resource-exhaustion consequences remain unmeasured. Related: #1334 and #1554 | | 2026-10-15 | Draw: Ink to Shape makes two-stroke arrows and arrows with a closed triangle head. A line plus a separate head stroke (an open V or a small closed triangle, drawn after the line or before it) whose apex sits at one of the line's ends becomes one straight arrow connector, and the head stroke goes, in one undo step; this works while drawing with Ink to Shape on and on lasso-selected strokes with Convert to Shapes. A shaft with a small triangle drawn round its tip in the same stroke is an arrow too; a triangle without a shaft stays a triangle. Our own design (Word for Mac has no Ink to Shape). | diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index c41a14b2b..a82a15b28 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -1079,10 +1079,33 @@ impl Document { /// The body, then the text boxes, footnotes and endnotes it shows (nested ones too), in /// document order. Headers, footers, comments and stories nothing points at aren't included. pub fn counted_stories(&self) -> Vec { - self.reachable(vec![StoryRef::Body], &|o| match o { - InlineObject::NoteRef { id, .. } => vec![(*id, &[PartKind::Footnote, PartKind::Endnote][..])], - o => o.text_boxes().into_iter().map(|id| (id, &[PartKind::TextBox][..])).collect(), - }) + self.reachable(vec![StoryRef::Body], ¬es_and_boxes) + } + + /// The stories a reader sees, in reading order: `counted_stories`, then the headers and + /// footers the sections use, each followed by the notes and text boxes it shows. Stories + /// nothing points at any more (a removed header, a deleted note's text) and comment text + /// aren't included. + pub fn shown_stories(&self) -> Vec { + let mut out = self.counted_stories(); + let mut seen: std::collections::BTreeSet = out.iter().copied().collect(); + let mut roots = Vec::new(); + let mut root_seen = std::collections::BTreeSet::new(); + for (_, sec) in self.sections() { + for set in [&sec.headers, &sec.footers] { + for id in [set.default, set.first, set.even].into_iter().flatten() { + if self.parts.get(&id).is_some_and(|p| matches!(p.kind, PartKind::Header | PartKind::Footer)) && root_seen.insert(id) { + roots.push(StoryRef::Part(id)); + } + } + } + } + for st in self.reachable(roots, ¬es_and_boxes) { + if seen.insert(st) { + out.push(st); + } + } + out } /// Drop the text box stories no box shows any more (a deleted or cut box's text), nested @@ -1452,6 +1475,14 @@ pub fn count_words(s: &str) -> usize { /// How deep in nested tables [`Document::para_paths`] goes: cells below this are skipped. pub(crate) const PARA_WALK_DEPTH: usize = 16; +/// The note and text box stories an object shows, for `Document::reachable`. +fn notes_and_boxes(o: &InlineObject) -> Vec<(u32, &'static [PartKind])> { + match o { + InlineObject::NoteRef { id, .. } => vec![(*id, &[PartKind::Footnote, PartKind::Endnote][..])], + o => o.text_boxes().into_iter().map(|id| (id, &[PartKind::TextBox][..])).collect(), + } +} + fn walk(bl: &Blocks, prefix: &mut Vec, out: &mut Vec, depth: usize) { if depth > PARA_WALK_DEPTH { return; diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index d54a1be84..ed92382ef 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -285,16 +285,16 @@ fn delete_comment(s: &mut Session, v: &Value) -> CmdResult { Ok(json!({"deleted": ids})) } -/// Comments in document order with their anchors: the body's, then those in headers, footers, -/// notes and text boxes, story by story (#2289). That is `Pos` order, which Next and Previous -/// rely on. Comment text itself isn't scanned. +/// Comments in reading order with their anchors: every story the reader sees, in +/// `Document::shown_stories` order (#2289). A comment anchored only in a story nothing shows +/// (a removed header) has no anchor. pub fn comment_list(s: &Session) -> Vec<(u32, Option)> { let mut found: Vec<(u32, Option)> = Vec::new(); - let stories = super::tools::all_stories(&s.doc).into_iter().filter(|st| match st { - StoryRef::Body => true, - StoryRef::Part(id) => s.doc.parts.get(id).is_some_and(|p| p.kind != wordcraft_doc::PartKind::Comment), - }); - for story in stories { + // The balloons ask every frame; most documents have no comments. + if s.doc.comments.is_empty() { + return found; + } + for story in s.doc.shown_stories() { for path in s.doc.para_paths(story) { let Some(p) = s.doc.para(story, &path) else { continue }; for o in p.object_offsets() { @@ -330,8 +330,16 @@ fn list_comments(s: &mut Session, _: &Value) -> CmdResult { fn nav_comment(s: &mut Session, dir: i32) -> CmdResult { let list: Vec = comment_list(s).into_iter().filter_map(|(_, p)| p).collect(); - let caret = s.sel.focus.clone(); - let t = if dir > 0 { list.iter().find(|p| **p > caret).or(list.first()) } else { list.iter().rev().find(|p| **p < caret).or(list.last()) }; + // Reading order: the story's place in `shown_stories`, then the place in the story. Part ids + // follow creation, not reading, so `Pos` order won't do across stories. + let rank: BTreeMap = s.doc.shown_stories().into_iter().enumerate().map(|(i, st)| (st, i)).collect(); + let key = |p: &Pos| rank.get(&p.story).map(|i| (*i, p.path.clone(), p.off)); + let caret = key(&s.sel.focus); + let t = if dir > 0 { + list.iter().find(|p| caret.is_some() && key(p) > caret).or(list.first()) + } else { + list.iter().rev().find(|p| caret.is_some() && key(p) < caret).or(list.last()) + }; if let Some(p) = t { s.sel = Selection::caret(p.clone()); } @@ -1755,69 +1763,121 @@ mod tests { assert!(started.elapsed() < std::time::Duration::from_secs(5), "took {:?}", started.elapsed()); } - /// A body comment, then comments in a header, a footer and a text box, each on its story's - /// whole text. Returns their ids and stories in that order. + /// A comment on the whole of the story the caret is in; its id and that story. + fn comment_here(s: &mut Session, text: &str) -> (u64, wordcraft_doc::StoryRef) { + let story = s.sel.focus.story; + s.run("select.all", &json!({})).unwrap(); + (s.run("review.newComment", &json!({"text": text})).unwrap()["id"].as_u64().unwrap(), story) + } + + /// Comments in a header, a footer, a text box and a footnote, made in that order, then one + /// on a word of the body. Returns each comment's id and story in reading order, which isn't + /// the order they were made: the body, the text box and footnote it shows, then the header + /// and the footer. fn comments_in_every_story(s: &mut Session) -> Vec<(u64, wordcraft_doc::StoryRef)> { - use wordcraft_doc::StoryRef; s.view.proofing = false; s.run("document.setText", &json!({"text": "Body text with a comment on it."})).unwrap(); - s.run("select.text", &json!({"text": "comment"})).unwrap(); - let mut out = vec![(s.run("review.newComment", &json!({"text": "body"})).unwrap()["id"].as_u64().unwrap(), StoryRef::Body)]; - for (cmd, text) in [("insert.header", "header"), ("insert.footer", "footer")] { - s.run("insert.closeHeader", &json!({})).unwrap(); - let story = StoryRef::Part(s.run(cmd, &json!({"text": text})).unwrap()["story"].as_u64().unwrap() as u32); - s.run("select.all", &json!({})).unwrap(); - out.push((s.run("review.newComment", &json!({"text": text})).unwrap()["id"].as_u64().unwrap(), story)); - } + s.run("insert.header", &json!({"text": "header"})).unwrap(); + let header = comment_here(s, "header"); + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run("insert.footer", &json!({"text": "footer"})).unwrap(); + let footer = comment_here(s, "footer"); s.run("insert.closeHeader", &json!({})).unwrap(); s.run("caret.docEnd", &json!({})).unwrap(); s.run("insert.textBox", &json!({"text": "boxed"})).unwrap(); - let story = s.sel.focus.story; - s.run("select.all", &json!({})).unwrap(); - out.push((s.run("review.newComment", &json!({"text": "text box"})).unwrap()["id"].as_u64().unwrap(), story)); + let text_box = comment_here(s, "text box"); + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run("caret.docEnd", &json!({})).unwrap(); + s.run("references.footnote", &json!({"text": "noted"})).unwrap(); + let note = comment_here(s, "footnote"); + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run("select.text", &json!({"text": "comment"})).unwrap(); + let body = (s.run("review.newComment", &json!({"text": "body"})).unwrap()["id"].as_u64().unwrap(), wordcraft_doc::StoryRef::Body); s.run("caret.docStart", &json!({})).unwrap(); - out + vec![body, text_box, note, header, footer] } - /// #2289: comments in headers, footers and text boxes are listed with their anchors, after - /// the body's, rather than with no anchor. + /// `review.comments` as (id, the anchor's story). + fn listed(s: &mut Session) -> Vec<(u64, Option)> { + let list = s.run("review.comments", &json!({})).unwrap(); + list.as_array().unwrap().iter().map(|c| (c["id"].as_u64().unwrap(), serde_json::from_value(c["anchor"]["story"].clone()).ok())).collect() + } + + /// The stories Next Comment visits, `n` presses from the caret. + fn next_stories(s: &mut Session, n: usize) -> Vec { + (0..n) + .map(|_| { + s.run("review.nextComment", &json!({})).unwrap(); + s.sel.focus.story + }) + .collect() + } + + /// #2289: comments in headers, footers, text boxes and footnotes are listed with their + /// anchors, in reading order rather than the order their stories were made. #[test] fn comments_outside_the_body_are_listed_with_their_anchors() { let mut s = Session::new(Document::new()); - let want = comments_in_every_story(&mut s); - let listed = s.run("review.comments", &json!({})).unwrap(); - let got: Vec<(u64, wordcraft_doc::StoryRef)> = listed - .as_array() - .unwrap() - .iter() - .map(|c| (c["id"].as_u64().unwrap(), serde_json::from_value(c["anchor"]["story"].clone()).expect("every comment has an anchor"))) - .collect(); - assert_eq!(got, want); + let want: Vec<_> = comments_in_every_story(&mut s).into_iter().map(|(id, st)| (id, Some(st))).collect(); + assert_eq!(listed(&mut s), want); } - /// #2289: Next and Previous Comment go through every story's comments and wrap around. + /// #2289: Next and Previous Comment go through every story's comments in reading order and + /// wrap around. #[test] fn next_and_previous_comment_reach_comments_outside_the_body() { let mut s = Session::new(Document::new()); - let want: Vec = comments_in_every_story(&mut s).into_iter().map(|(_, st)| st).collect(); - let mut forward = Vec::new(); - for _ in 0..=want.len() { - s.run("review.nextComment", &json!({})).unwrap(); - forward.push(s.sel.focus.story); - } + let want: Vec<_> = comments_in_every_story(&mut s).into_iter().map(|(_, st)| st).collect(); let mut wrapped = want.clone(); wrapped.push(want[0]); - assert_eq!(forward, wrapped, "Next visits each story's comment, then wraps to the first"); - let mut back = Vec::new(); - for _ in 0..want.len() { - s.run("review.previousComment", &json!({})).unwrap(); - back.push(s.sel.focus.story); - } + assert_eq!(next_stories(&mut s, want.len() + 1), wrapped, "Next visits each comment, then wraps to the first"); + let back: Vec<_> = (0..want.len()) + .map(|_| { + s.run("review.previousComment", &json!({})).unwrap(); + s.sel.focus.story + }) + .collect(); let mut rev = want.clone(); rev.reverse(); assert_eq!(back, rev, "Previous from the first comment wraps to the last"); } + /// #2289: a header comes before the footer whichever was made first. + #[test] + fn next_comment_reaches_the_header_before_the_footer() { + let mut s = Session::new(Document::new()); + s.view.proofing = false; + s.run("document.setText", &json!({"text": "Body"})).unwrap(); + s.run("insert.footer", &json!({"text": "footer"})).unwrap(); + let footer = comment_here(&mut s, "footer").1; + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run("insert.header", &json!({"text": "header"})).unwrap(); + let header = comment_here(&mut s, "header").1; + s.run("insert.closeHeader", &json!({})).unwrap(); + assert_eq!(next_stories(&mut s, 2), vec![header, footer]); + } + + /// #2289: a comment left in a story nothing shows any more (a removed header, a deleted + /// footnote's text) has no anchor, and Next never takes the caret into that hidden story. + #[test] + fn comments_in_hidden_stories_are_not_navigated_to() { + let mut s = Session::new(Document::new()); + let all = comments_in_every_story(&mut s); + let (note, header) = (all[2], all[3]); + s.run("insert.removeHeader", &json!({})).unwrap(); + s.run("caret.docEnd", &json!({})).unwrap(); + s.run("text.backspace", &json!({})).unwrap(); + assert!(!s.doc.shown_stories().contains(¬e.1), "the footnote's reference is gone"); + let list = listed(&mut s); + for hidden in [note, header] { + assert!(list.contains(&(hidden.0, None)), "comment {} sits in a hidden story: {list:?}", hidden.0); + } + let shown = [all[0].1, all[1].1, all[4].1]; + for st in next_stories(&mut s, all.len()) { + assert!(shown.contains(&st), "Next went to hidden {st:?}"); + } + } + /// Text inside a comment isn't an anchor: a comment marker in a comment's own story is not /// listed as that comment's place in the document. #[test] diff --git a/crates/engine/src/cmd/tools.rs b/crates/engine/src/cmd/tools.rs index 76243df5b..b02408dd2 100644 --- a/crates/engine/src/cmd/tools.rs +++ b/crates/engine/src/cmd/tools.rs @@ -1586,7 +1586,7 @@ impl Hidden { } /// Every story of the document: the body, then headers, footers, notes, comments and text boxes. -pub(crate) fn all_stories(doc: &Document) -> Vec { +fn all_stories(doc: &Document) -> Vec { std::iter::once(StoryRef::Body).chain(doc.parts.keys().map(|k| StoryRef::Part(*k))).collect() } From 260014167d5930febff9182e875fa207c721a1b9 Mon Sep 17 00:00:00 2001 From: luisvaca Date: Mon, 19 Oct 2026 23:31:20 -0600 Subject: [PATCH 3/4] #2289: skip comments in header versions not in use and in linked text boxes' own text A first-page or even-page header or footer is shown only while Different First Page or Different Odd & Even is on, and a text box linked from another shows the first box's text instead of its own. Comments in those stories were still listed and reached by Next and Previous, and typing then went into text nobody sees. shown_stories now picks each section's versions the way layout does and leaves out linked boxes' own text. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 2 +- crates/doc/src/lib.rs | 29 ++++++++++---- crates/engine/src/cmd/review.rs | 69 +++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 9 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index c721920a1..eb583100f 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -234,7 +234,7 @@ Imported Word 97–2003 `.doc`/`.dot` files now ask for a supported DOCX target | 2026-10-14 | Page-break-only list paragraphs no longer print stray bullets or numbers. The next item keeps its number, matching the reference export. | | 2026-10-12 | Backspace at the start of the paragraph after a table nested in a cell moves into the nested table's last cell, no longer into the last cell of the table around it | | 2026-10-14 | Grouped `flowChartOr` shapes draw their circle and crossing spokes, as in Word | -| 2026-10-15 | Comments in headers, footers, notes and text boxes are listed with their place in the document, get a balloon beside their text, and Next, Previous and Go to reach them in reading order: the body with its notes and text boxes, then headers and footers. A comment left in a removed header or a deleted note's text isn't navigated to (#2289) | +| 2026-10-15 | Comments in headers, footers, notes and text boxes are listed with their place in the document, get a balloon beside their text, and Next, Previous and Go to reach them in reading order: the body with its notes and text boxes, then headers and footers. A comment in a story nothing shows (a removed header, a deleted note's text, a first-page or even-page header whose option is off, a linked text box's own text) isn't navigated to (#2289) | | 2026-10-14 | Deleting a comment removes its anchors from headers, footers, notes and text boxes too, not just the body, and deletes the whole thread below it, replies to replies included; no anchor or reply is left pointing at a deleted comment (#1345) | | 2026-10-14 | Legacy DOC DIB conversion checks the BMP header and copy before allocation, uses the caller's decode allowance, and caps PNG output while encoding; a BMP fallback is kept when it fits. A generated 1-pixel regression covers 57/58/512-byte limits. Resource-exhaustion consequences remain unmeasured. Related: #1334 and #1554 | | 2026-10-15 | Draw: Ink to Shape makes two-stroke arrows and arrows with a closed triangle head. A line plus a separate head stroke (an open V or a small closed triangle, drawn after the line or before it) whose apex sits at one of the line's ends becomes one straight arrow connector, and the head stroke goes, in one undo step; this works while drawing with Ink to Shape on and on lasso-selected strokes with Convert to Shapes. A shaft with a small triangle drawn round its tip in the same stroke is an arrow too; a triangle without a shaft stays a triangle. Our own design (Word for Mac has no Ink to Shape). | diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index a82a15b28..86fcaf45d 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -1082,25 +1082,38 @@ impl Document { self.reachable(vec![StoryRef::Body], ¬es_and_boxes) } - /// The stories a reader sees, in reading order: `counted_stories`, then the headers and - /// footers the sections use, each followed by the notes and text boxes it shows. Stories - /// nothing points at any more (a removed header, a deleted note's text) and comment text - /// aren't included. + /// The stories a reader sees, in reading order: the body, then the headers and footers in + /// use, each followed by the notes and text boxes it shows. Hidden stories (a removed header, + /// a first-page or even-page header whose option is off, a linked text box's own text) and + /// comment text aren't included. pub fn shown_stories(&self) -> Vec { - let mut out = self.counted_stories(); + let followers: std::collections::BTreeSet = self.text_box_chains().into_iter().flat_map(|c| c.into_iter().skip(1)).collect(); + let follow = |o: &InlineObject| { + let mut v = notes_and_boxes(o); + v.retain(|(id, _)| !followers.contains(id)); + v + }; + let mut out = self.reachable(vec![StoryRef::Body], &follow); let mut seen: std::collections::BTreeSet = out.iter().copied().collect(); let mut roots = Vec::new(); let mut root_seen = std::collections::BTreeSet::new(); + let (mut headers, mut footers) = (section::HeaderSet::default(), section::HeaderSet::default()); for (_, sec) in self.sections() { - for set in [&sec.headers, &sec.footers] { - for id in [set.default, set.first, set.even].into_iter().flatten() { + // Unset versions are inherited from the previous section. + for (set, own) in [(&mut headers, &sec.headers), (&mut footers, &sec.footers)] { + set.default = own.default.or(set.default); + set.first = own.first.or(set.first); + set.even = own.even.or(set.even); + let first = set.first.filter(|_| sec.title_page); + let even = set.even.filter(|_| self.settings.even_odd_headers); + for id in [set.default, first, even].into_iter().flatten() { if self.parts.get(&id).is_some_and(|p| matches!(p.kind, PartKind::Header | PartKind::Footer)) && root_seen.insert(id) { roots.push(StoryRef::Part(id)); } } } } - for st in self.reachable(roots, ¬es_and_boxes) { + for st in self.reachable(roots, &follow) { if seen.insert(st) { out.push(st); } diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index ed92382ef..abe9de874 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -1878,6 +1878,75 @@ mod tests { } } + /// A body comment, then, with `option` on, a comment in the header of page `page`; `option` + /// is turned off again. Returns the header comment's id and story. + fn comment_in_header_version(s: &mut Session, option: &str, page: usize) -> (u64, wordcraft_doc::StoryRef) { + s.view.proofing = false; + s.run("document.setText", &json!({"text": "Body text with a comment on it."})).unwrap(); + s.run("select.text", &json!({"text": "comment"})).unwrap(); + s.run("review.newComment", &json!({"text": "body"})).unwrap(); + s.run("caret.docEnd", &json!({})).unwrap(); + s.run("insert.pageBreak", &json!({})).unwrap(); + s.run(option, &json!({"value": true})).unwrap(); + s.run("insert.editHeader", &json!({"page": page})).unwrap(); + s.run("text.insert", &json!({"text": "header"})).unwrap(); + let header = comment_here(s, "header"); + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run(option, &json!({"value": false})).unwrap(); + s.run("caret.docStart", &json!({})).unwrap(); + header + } + + /// A first-page or even-page header is hidden while its option is off: its comments have no + /// anchor and Next skips them. + #[test] + fn comments_in_header_versions_whose_option_is_off_are_not_navigated_to() { + for (option, page, slot) in [("layout.differentFirstPage", 0, "first"), ("layout.differentOddEven", 1, "even")] { + let mut s = Session::new(Document::new()); + let (id, story) = comment_in_header_version(&mut s, option, page); + let headers = s.doc.sections().first().map(|(_, sec)| sec.headers).unwrap(); + let want = if slot == "first" { headers.first } else { headers.even }; + assert_eq!(want.map(wordcraft_doc::StoryRef::Part), Some(story), "{option}: the comment is in the {slot} header"); + assert!(listed(&mut s).contains(&(id, None)), "{option} off: no anchor"); + for st in next_stories(&mut s, 3) { + assert_eq!(st, wordcraft_doc::StoryRef::Body, "{option} off: Next went to the hidden {slot} header"); + } + s.run(option, &json!({"value": true})).unwrap(); + assert!(listed(&mut s).contains(&(id, Some(story))), "{option} on: anchored again"); + } + } + + /// A linked text box shows the first box's text: comments in its own text have no anchor and + /// Next skips them. + #[test] + fn comments_in_a_linked_text_box_own_text_are_not_navigated_to() { + let mut s = Session::new(Document::new()); + s.view.proofing = false; + s.run("document.setText", &json!({"text": "Body text with a comment on it."})).unwrap(); + s.run("select.text", &json!({"text": "comment"})).unwrap(); + s.run("review.newComment", &json!({"text": "body"})).unwrap(); + s.run("caret.docEnd", &json!({})).unwrap(); + s.run("insert.textBox", &json!({"text": "first box"})).unwrap(); + let first = s.sel.focus.story; + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run("caret.docEnd", &json!({})).unwrap(); + // Only a blank box can be linked to. + s.run("insert.textBox", &json!({"text": " "})).unwrap(); + let (id, follower) = comment_here(&mut s, "follower"); + s.run("insert.closeHeader", &json!({})).unwrap(); + s.run("caret.set", &json!({"pos": {"story": first, "path": [0], "off": 0}})).unwrap(); + s.run("shape.link", &json!({"target": 1})).unwrap(); + assert_eq!(s.doc.text_box_chains().first().and_then(|c| c.get(1)).copied().map(wordcraft_doc::StoryRef::Part), Some(follower)); + s.run("caret.docStart", &json!({})).unwrap(); + assert!(listed(&mut s).contains(&(id, None)), "linked: no anchor"); + for st in next_stories(&mut s, 3) { + assert_ne!(st, follower, "Next went into the linked box's own text"); + } + s.run("caret.set", &json!({"pos": {"story": first, "path": [0], "off": 0}})).unwrap(); + s.run("shape.breakLink", &json!({})).unwrap(); + assert!(listed(&mut s).contains(&(id, Some(follower))), "unlinked: anchored again"); + } + /// Text inside a comment isn't an anchor: a comment marker in a comment's own story is not /// listed as that comment's place in the document. #[test] From fffd48c885090e282a0bb25c63c39a05f9b08539 Mon Sep 17 00:00:00 2001 From: luisvaca Date: Tue, 20 Oct 2026 03:33:15 -0600 Subject: [PATCH 4/4] #2289: the first-page header comes before the default header in comment order With Different First Page on, a section's default header was listed and reached by Next before its first-page header, so Next went to page 2 before page 1. A section's header and footer versions now go in the order of the first page each one shows on. Co-Authored-By: Claude Opus 5.5 --- crates/doc/src/lib.rs | 4 +++- crates/engine/src/cmd/review.rs | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 86fcaf45d..d86dcaf53 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -1106,7 +1106,9 @@ impl Document { set.even = own.even.or(set.even); let first = set.first.filter(|_| sec.title_page); let even = set.even.filter(|_| self.settings.even_odd_headers); - for id in [set.default, first, even].into_iter().flatten() { + // By the first page each one shows on: page 1, then page 2 (even), then page 3. + let order = if sec.title_page { [first, even, set.default] } else { [set.default, even, None] }; + for id in order.into_iter().flatten() { if self.parts.get(&id).is_some_and(|p| matches!(p.kind, PartKind::Header | PartKind::Footer)) && root_seen.insert(id) { roots.push(StoryRef::Part(id)); } diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index abe9de874..52c41621a 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -1916,6 +1916,30 @@ mod tests { } } + /// With Different First Page on, the first page's header comes before the later pages' + /// header, in the list and for Next. + #[test] + fn first_page_header_comes_before_the_default_header() { + let mut s = Session::new(Document::new()); + s.view.proofing = false; + s.run("document.setText", &json!({"text": "Page one"})).unwrap(); + s.run("caret.docEnd", &json!({})).unwrap(); + s.run("insert.pageBreak", &json!({})).unwrap(); + s.run("layout.differentFirstPage", &json!({"value": true})).unwrap(); + let mut header = |page: usize| { + s.run("insert.editHeader", &json!({"page": page})).unwrap(); + s.run("text.insert", &json!({"text": "header"})).unwrap(); + let c = comment_here(&mut s, "header"); + s.run("insert.closeHeader", &json!({})).unwrap(); + c + }; + let (first, default) = (header(0), header(1)); + assert_ne!(first.1, default.1); + s.run("caret.docStart", &json!({})).unwrap(); + assert_eq!(listed(&mut s), vec![(first.0, Some(first.1)), (default.0, Some(default.1))]); + assert_eq!(next_stories(&mut s, 2), vec![first.1, default.1]); + } + /// A linked text box shows the first box's text: comments in its own text have no anchor and /// Next skips them. #[test]