Merge PR #2298: Comments: comments in headers, footers, notes and text boxes are listed, get balloons and are reached by Next/Previous (#2289)

ROADMAP.md

@@ -251,6 +251,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 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). |
@@ -615,6 +616,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 |

crates/doc/src/lib.rs

@@ -1080,10 +1080,48 @@ 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<StoryRef> {
-        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], &notes_and_boxes)
+    }
+
+    /// 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<StoryRef> {
+        let followers: std::collections::BTreeSet<u32> = 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<StoryRef> = 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() {
+            // 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);
+                // 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));
+                    }
+                }
+            }
+        }
+        for st in self.reachable(roots, &follow) {
+            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
@@ -1453,6 +1491,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<u32>, out: &mut Vec<Path>, depth: usize) {
     if depth > PARA_WALK_DEPTH {
         return;

crates/engine/src/cmd/review.rs

@@ -301,14 +301,22 @@ fn delete_comment(s: &mut Session, v: &Value) -> CmdResult {
     Ok(json!({"deleted": ids}))
 }
 
-/// Comments in document order with their anchors.
+/// 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<Pos>)> {
     let mut found: Vec<(u32, Option<Pos>)> = 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 })));
+    // 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() {
+                if let Some(InlineObject::CommentStart { id }) = p.object_at(o) {
+                    found.push((*id, Some(Pos { story, path: path.clone(), off: o })));
+                }
             }
         }
     }
@@ -338,8 +346,16 @@ fn list_comments(s: &mut Session, _: &Value) -> CmdResult {
 
 fn nav_comment(s: &mut Session, dir: i32) -> CmdResult {
     let list: Vec<Pos> = 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<StoryRef, usize> = 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());
     }
@@ -1763,6 +1779,231 @@ mod tests {
         assert!(started.elapsed() < std::time::Duration::from_secs(5), "took {:?}", started.elapsed());
     }
 
+    /// 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)> {
+        s.view.proofing = false;
+        s.run("document.setText", &json!({"text": "Body text with a comment on it."})).unwrap();
+        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 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();
+        vec![body, text_box, note, header, footer]
+    }
+
+    /// `review.comments` as (id, the anchor's story).
+    fn listed(s: &mut Session) -> Vec<(u64, Option<wordcraft_doc::StoryRef>)> {
+        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<wordcraft_doc::StoryRef> {
+        (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: 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 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 wrapped = want.clone();
+        wrapped.push(want[0]);
+        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(&note.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:?}");
+        }
+    }
+
+    /// 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");
+        }
+    }
+
+    /// 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]
+    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]
+    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