From f4ee74db05328b3659766482593bc71afadc74a9 Mon Sep 17 00:00:00 2001 From: Guillem Hermida Date: Fri, 16 Oct 2026 18:59:04 +0200 Subject: [PATCH] fix(comments): the note editor takes the keyboard once, so V and H pick tools again After a highlight, its note editor in the Comments panel kept the keyboard, so V and H were typed into the note (#2669, item 2). - The editor asked for the keyboard on every frame when no other widget had it, so it took it back after every click elsewhere. It now takes it once, when it opens (CommentView::open_editor, EditorFocus::Take). - Adding another comment left the editor open, and drawing is a drag, which egui doesn't treat as a click elsewhere. An empty editor on another comment now closes when a comment is added; one with text stays open with it but gives the keyboard back (EditorFocus::Release). Part of #2669. --- crates/ui-egui/src/comments.rs | 32 ++++++++++ crates/ui-egui/src/comments_panel.rs | 10 +-- crates/ui-egui/src/editing.rs | 7 ++- crates/ui-egui/tests/comments.rs | 93 ++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/comments.rs b/crates/ui-egui/src/comments.rs index 5a708c14f..6d27b3ff4 100644 --- a/crates/ui-egui/src/comments.rs +++ b/crates/ui-egui/src/comments.rs @@ -496,6 +496,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, +} + +/// 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 { @@ -512,6 +525,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). diff --git a/crates/ui-egui/src/comments_panel.rs b/crates/ui-egui/src/comments_panel.rs index c5fbbd0ce..c62709c16 100644 --- a/crates/ui-egui/src/comments_panel.rs +++ b/crates/ui-egui/src/comments_panel.rs @@ -323,8 +323,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| { @@ -433,7 +435,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 { @@ -448,7 +450,7 @@ fn card( .show(|ui| { ui.set_min_width(160.0); 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| { diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index be1347d68..80feef11f 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -116,6 +116,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 } => { @@ -142,9 +145,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, diff --git a/crates/ui-egui/tests/comments.rs b/crates/ui-egui/tests/comments.rs index 914bc281b..d1cdbe5c6 100644 --- a/crates/ui-egui/tests/comments.rs +++ b/crates/ui-egui/tests/comments.rs @@ -231,6 +231,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 { + 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| {