diff --git a/crates/ui-egui/src/comments.rs b/crates/ui-egui/src/comments.rs index 7fd9b7a62..13281f263 100644 --- a/crates/ui-egui/src/comments.rs +++ b/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, +} + +/// 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). diff --git a/crates/ui-egui/src/comments_panel.rs b/crates/ui-egui/src/comments_panel.rs index 2ac1853f4..5f37789a7 100644 --- a/crates/ui-egui/src/comments_panel.rs +++ b/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| { diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index 4aed1c086..977c88ade 100644 --- a/crates/ui-egui/src/editing.rs +++ b/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, diff --git a/crates/ui-egui/tests/comments.rs b/crates/ui-egui/tests/comments.rs index 01166a41d..601d4f8b2 100644 --- a/crates/ui-egui/tests/comments.rs +++ b/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 { + 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| {