Merge PR #3015: fix(comments): the note editor takes the keyboard once, so V and H pick tools again

crates/ui-egui/src/comments.rs

@@ -521,6 +521,19 @@ pub struct CommentView {
     pub context_at: Option<(usize, [f64; 2])>,
     /// A one-shot tool just finished; the app returns to the Select tool unless pinned.
     pub tool_done: bool,
+    /// What the inline editor (`editing`) does with the keyboard the next time it is drawn.
+    pub(crate) editor_focus: Option<EditorFocus>,
+}
+
+/// A one-off change to the keyboard focus of the Comments panel's inline editor. The editor
+/// never asks for the keyboard on its own: when it did so on every frame, it took the keyboard
+/// back after every click elsewhere, and the tool keys (V, H) were typed into it (#2669).
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub(crate) enum EditorFocus {
+    /// The editor just opened: it takes the keyboard, once.
+    Take,
+    /// Another comment was added: the editor keeps its text but gives the keyboard back.
+    Release,
 }
 
 impl CommentView {
@@ -537,6 +550,25 @@ impl CommentView {
         }
         true
     }
+
+    /// Open the inline editor on comment (page, index) with `text`. It takes the keyboard once,
+    /// as it opens; after that the keyboard goes wherever the user clicks.
+    pub(crate) fn open_editor(&mut self, page: usize, index: usize, text: String) {
+        self.editing = Some((page, index, text));
+        self.editor_focus = Some(EditorFocus::Take);
+    }
+
+    /// A comment was just added. The editor open on another one must not keep the keyboard, or
+    /// the tool keys pressed next are typed into it (#2669): an empty editor closes, and one
+    /// holding text stays open with it, without the keyboard.
+    pub(crate) fn comment_added(&mut self) {
+        if self.editing.as_ref().is_some_and(|(_, _, text)| !text.is_empty()) {
+            self.editor_focus = Some(EditorFocus::Release);
+        } else {
+            self.editing = None;
+            self.editor_focus = None;
+        }
+    }
 }
 
 /// Comments panel order (Acrobat: Sort by page, author, date, type, colour).

crates/ui-egui/src/comments_panel.rs

@@ -324,8 +324,10 @@ fn card(
                 ui.set_width(ui.available_width());
                 if editing && let Some((_, _, text)) = view.comments.editing.as_mut() {
                     let r = ui.add(crate::widgets::block(text).desired_rows(2).desired_width(f32::INFINITY).id_salt(("comment-edit", key)));
-                    if !r.has_focus() && !ui.memory(|m| m.focused().is_some()) {
-                        r.request_focus();
+                    match view.comments.editor_focus.take() {
+                        Some(crate::comments::EditorFocus::Take) => r.request_focus(),
+                        Some(crate::comments::EditorFocus::Release) => r.surrender_focus(),
+                        None => {}
                     }
                     let submit = crate::comments::submit_key(ui, &r, false);
                     ui.horizontal(|ui| {
@@ -434,7 +436,7 @@ fn card(
         *nav = Some(Nav::Flash(a.page, a.rect));
     }
     if click.double_clicked() && allowed {
-        view.comments.editing = Some((a.page, a.index, a.contents.clone().unwrap_or_default()));
+        view.comments.open_editor(a.page, a.index, a.contents.clone().unwrap_or_default());
     }
     let menu_id = ui.id().with(("card-menu", key));
     if open_menu {
@@ -450,7 +452,7 @@ fn card(
             ui.set_min_width(160.0);
             crate::comments::attachment_items(ui, view, info, a);
             if ui.button(tl_ctx!("comment menu", "Edit")).clicked() {
-                view.comments.editing = Some((a.page, a.index, a.contents.clone().unwrap_or_default()));
+                view.comments.open_editor(a.page, a.index, a.contents.clone().unwrap_or_default());
                 ui.close();
             }
             ui.menu_button(tl!("Set status"), |ui| {

crates/ui-egui/src/editing.rs

@@ -126,6 +126,9 @@ impl PdfCraftApp {
                 } else {
                     view.document_changed(info);
                 }
+                if matches!(edit, Edit::AddAnnotation(_)) {
+                    view.comments.comment_added();
+                }
                 // Keep the pages the user acted on selected, where they now are.
                 match edit {
                     Edit::MovePages { pages, to } => {
@@ -157,9 +160,9 @@ impl PdfCraftApp {
                         // unless a note typed into another card is still unsaved.
                         if let (pdfcraft_engine::Shape::TextMarkup { kind: pdfcraft_engine::Markup::Highlight, .. }, Some(n)) = (&a.shape, newest)
                             && a.contents.is_empty()
-                            && view.comments.editing.as_ref().is_none_or(|(_, _, text)| text.is_empty())
+                            && view.comments.editing.is_none()
                         {
-                            view.comments.editing = Some((a.page, n, String::new()));
+                            view.comments.open_editor(a.page, n, String::new());
                         }
                     }
                     Edit::DeleteAnnotation { .. } => view.comments.selected = None,

crates/ui-egui/tests/comments.rs

@@ -233,6 +233,99 @@ fn a_new_highlight_opens_its_note_for_typing() {
     assert_eq!(h.state().views[0].comments.editing, Some((0, 1, "typed".into())));
 }
 
+/// The Comments panel's editor for a comment's note (the reply field is the other multiline one).
+fn note_editor<'a>(h: &'a Harness<'static, PdfCraftApp>) -> egui_kittest::Node<'a> {
+    h.query_all_by(|n| n.role() == egui::accesskit::Role::MultilineTextInput && n.placeholder() != Some("Add a reply")).next().expect("note editor")
+}
+
+/// The text in the open note editor.
+fn note(h: &Harness<'static, PdfCraftApp>) -> Option<String> {
+    h.state().views[0].comments.editing.as_ref().map(|(_, _, text)| text.clone())
+}
+
+/// A letter key as a keyboard sends it: the key goes down, types its letter and comes up.
+fn type_key(h: &mut Harness<'static, PdfCraftApp>, key: egui::Key, letter: &str) {
+    h.key_down(key);
+    h.event(egui::Event::Text(letter.into()));
+    h.key_up(key);
+    h.run_steps(2);
+}
+
+/// Highlight glyphs `from..to` of page 1's text with the highlighter.
+fn highlight_text(h: &mut Harness<'static, PdfCraftApp>, from: usize, to: usize) {
+    let (a, b) = {
+        let v = &h.state().views[0];
+        (v.glyph_screen_pos(0, from).expect("text layer"), v.glyph_screen_pos(0, to).expect("glyph"))
+    };
+    drag(h, a, b);
+    h.run_steps(2);
+}
+
+#[test]
+fn clicking_the_page_gives_the_keyboard_back_from_a_highlight_note() {
+    // #2669: the note editor took the keyboard back on every frame, so after a click on the
+    // page V and H were typed into the note instead of picking a tool.
+    let mut h = harness(|app| {
+        app.set_option("quick", "highlight").unwrap();
+        app.set_option("panel", "comments").unwrap();
+    });
+    highlight_text(&mut h, 4, 14);
+    assert!(note_editor(&h).is_focused(), "a new highlight's note takes the keyboard straight away");
+    h.event(egui::Event::Text("Note".into()));
+    h.run_steps(2);
+    assert_eq!(note(&h).as_deref(), Some("Note"), "typing goes into the note");
+    // A click on an empty part of the page gives the keyboard back to the page.
+    click_pt(&mut h, (250.0, 20.0));
+    type_key(&mut h, egui::Key::V, "v");
+    assert_eq!(h.state().quick_tool, QuickTool::Select, "V picks the Select tool");
+    assert_eq!(note(&h).as_deref(), Some("Note"), "V is not typed into the note");
+    type_key(&mut h, egui::Key::H, "h");
+    assert_eq!(h.state().quick_tool, QuickTool::Hand, "H picks the Hand tool");
+    assert_eq!(note(&h).as_deref(), Some("Note"), "nor is H");
+    // Clicking back into the note types into it again, and the letters are text there.
+    note_editor(&h).click();
+    h.run_steps(2);
+    type_key(&mut h, egui::Key::V, "v");
+    assert_eq!(note(&h).as_deref(), Some("Notev"), "the note takes keys while the user types in it");
+    assert_eq!(h.state().quick_tool, QuickTool::Hand, "typed letters don't pick tools");
+}
+
+#[test]
+fn drawing_another_comment_takes_the_keyboard_from_a_highlight_note() {
+    // #2669: a highlight's note kept the keyboard while rectangles and ovals were drawn (a drag
+    // is not a click, so the editor never let go), and V and H were typed into it.
+    use pdfcraft_ui_egui::comments::CommentTool;
+    let mut h = harness(|app| {
+        app.set_option("quick", "highlight").unwrap();
+        app.set_option("panel", "comments").unwrap();
+    });
+    highlight_text(&mut h, 4, 14);
+    assert!(note_editor(&h).is_focused());
+    // The tools are picked without a click, as a shortcut would: the drawing alone must take
+    // the keyboard back. An empty note closes.
+    h.state_mut().quick_tool = QuickTool::Comment(CommentTool::Rectangle);
+    drag_pt(&mut h, (40.0, 100.0), (140.0, 40.0));
+    assert_eq!(comments(&h).len(), 2);
+    assert_eq!(h.state().views[0].comments.editing, None, "the empty note closed");
+    type_key(&mut h, egui::Key::H, "h");
+    assert_eq!(h.state().quick_tool, QuickTool::Hand, "H picks the Hand tool");
+    // A note with text in it stays open with its text, but lets the keyboard go.
+    h.state_mut().quick_tool = QuickTool::Comment(CommentTool::Highlight);
+    drag_pt(&mut h, (160.0, 100.0), (260.0, 40.0));
+    h.run_steps(2);
+    assert_eq!(h.state().views[0].comments.editing, Some((0, 2, String::new())), "the new area highlight's note opens");
+    assert!(note_editor(&h).is_focused());
+    h.event(egui::Event::Text("Note".into()));
+    h.run_steps(2);
+    h.state_mut().quick_tool = QuickTool::Comment(CommentTool::Rectangle);
+    drag_pt(&mut h, (40.0, 30.0), (260.0, 8.0));
+    assert_eq!(comments(&h).len(), 4);
+    assert_eq!(h.state().views[0].comments.editing, Some((0, 2, "Note".into())), "a note with text stays open");
+    type_key(&mut h, egui::Key::H, "h");
+    assert_eq!(h.state().quick_tool, QuickTool::Hand, "H picks the Hand tool");
+    assert_eq!(note(&h).as_deref(), Some("Note"), "H is not typed into the note");
+}
+
 #[test]
 fn ctrl_enter_posts_a_highlight_comment_and_shift_enter_adds_a_line() {
     let mut h = harness(|app| {