From f45fd7507efadd04627a5a977070d1dc63c8a99d Mon Sep 17 00:00:00 2001 From: Joel Tony Date: Sun, 11 Oct 2026 02:34:14 +0530 Subject: [PATCH] Issue #870: keys leave the document under a dialog alone (#898) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Issue #870: keys leave the document under a dialog alone With Preferences (or any other dialog) open, the registered shortcuts still ran on the document underneath it: ⌘W / Ctrl+W closed the file, ⌘Z undid an edit, ⌘F opened Find. In Organize Pages the grid's own keys did too, so Delete or Backspace deleted the selected pages behind the dialog, and the grid took the Escape meant to close it. Every modal now owns the keyboard, as "Save changes?" already did: PdfCraftApp::modal_open covers the dialog, the save, password and link prompts and the update check. Shortcuts stop there, and the organize grid's keys and autoscroll wait until the page is unobstructed. * Contributors: credit jay-tau by their GitHub profile name --- contributors/people.toml | 3 +++ crates/ui-egui/src/canvas.rs | 16 +++++++------ crates/ui-egui/src/lib.rs | 13 +++++++++- crates/ui-egui/tests/editing.rs | 42 ++++++++++++++++++++++++++++++++- 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/contributors/people.toml b/contributors/people.toml index 8ba666cc..c685b584 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -71,3 +71,6 @@ sync_github_name = true [people.Hathor875] real_name = "Krzysztof Cieślik" sync_github_name = true + +[people.jay-tau] +sync_github_name = true diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 6438c903..15560090 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1772,7 +1772,8 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { None => {} } // No dialog, close prompt or palette over the page: only then does page input count. - let unobstructed = app.dialog.is_none() && app.close_request.is_none() && !app.palette_open; + let unobstructed = !app.modal_open() && !app.palette_open; + let view = &mut app.views[index]; if view.organize { organize_grid(view, info, doc.allows_assembly(), doc.dirty, unobstructed, ui, &t); return; @@ -3273,7 +3274,7 @@ fn quick_bar(app: &mut PdfCraftApp, area: Rect, ui: &mut egui::Ui) { } /// The organize toolbar: page operations on the selection (Acrobat's Organize Pages bar). -fn organize_toolbar(view: &mut DocView, info: &DocInfo, editable: bool, dirty: bool, ui: &mut egui::Ui, t: &Tokens) { +fn organize_toolbar(view: &mut DocView, info: &DocInfo, editable: bool, dirty: bool, unobstructed: bool, ui: &mut egui::Ui, t: &Tokens) { let targets = view.target_pages(); let n = info.pages.len(); let (first, last) = (targets.first().copied().unwrap_or(0), targets.last().copied().unwrap_or(0)); @@ -3379,8 +3380,9 @@ fn organize_toolbar(view: &mut DocView, info: &DocInfo, editable: bool, dirty: b }); }); }); - // Keys act on the selection unless a text field has focus. - if editable && !ui.ctx().egui_wants_keyboard_input() { + // Keys act on the selection unless a text field or a dialog has them (Delete under + // Preferences deleted the selected pages). + if editable && unobstructed && !ui.ctx().egui_wants_keyboard_input() { use egui::{Key, Modifiers}; let (del, esc) = ui.input_mut(|i| { ( @@ -3431,16 +3433,16 @@ fn gap_button(ui: &mut egui::Ui, gap: usize, at: Pos2, height: f32, label: Strin resp.on_hover_text(label).clicked() } -fn organize_grid(view: &mut DocView, info: &DocInfo, editable: bool, dirty: bool, auto_scroll_enabled: bool, ui: &mut egui::Ui, t: &Tokens) { +fn organize_grid(view: &mut DocView, info: &DocInfo, editable: bool, dirty: bool, unobstructed: bool, ui: &mut egui::Ui, t: &Tokens) { let ppp = ui.ctx().pixels_per_point(); // The page image scales with the zoom; the padding and the page number don't. let cell = vec2(146.0 * view.grid_zoom + 44.0, 194.0 * view.grid_zoom + 56.0); let anchor = view.grid_anchor.take(); let mut open_page = None; - organize_toolbar(view, info, editable, dirty, ui, t); + organize_toolbar(view, info, editable, dirty, unobstructed, ui, t); let viewport = ui.available_rect_before_wrap(); view.viewport_screen = viewport; - let auto_delta = if auto_scroll_enabled { + let auto_delta = if unobstructed { view.auto_scroll.update(ui, viewport, true) } else { view.auto_scroll.cancel(); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 039103ec..70e14a8d 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1194,6 +1194,12 @@ impl PdfCraftApp { self.active.and_then(|i| self.views.get(i).map(|v| (i, v.id))) } + /// A dialog or prompt is open over the window: the keyboard is its own, and nothing may act + /// on the document underneath it. + pub(crate) fn modal_open(&self) -> bool { + self.dialog.is_some() || self.close_request.is_some() || self.password_prompt.is_some() || self.pending_link.is_some() || self.updates.open + } + /// Enable the UI control channel on `ctx` (opt-in; see [`control`]). Returns a client that /// sends requests to this app; [`control::serve`] exposes it on loopback. pub fn attach_control(&mut self, ctx: &egui::Context) -> control::ControlClient { @@ -1769,6 +1775,11 @@ impl PdfCraftApp { } return; } + // The other dialogs and prompts are modal too (⌘W closed the file behind Preferences, + // #870). + if self.modal_open() { + return; + } if let Some(view) = self.active.and_then(|i| self.views.get_mut(i)) && view.auto_scroll.escape(ctx) { @@ -1958,7 +1969,7 @@ impl eframe::App for PdfCraftApp { self.shortcuts(ctx); // Scrolling is transient: never resume after changing tabs, opening a modal/palette, // or returning to a window that lost focus. - let blocked = self.dialog.is_some() || self.close_request.is_some() || self.palette_open || !ctx.input(|i| i.focused); + let blocked = self.modal_open() || self.palette_open || !ctx.input(|i| i.focused); for (index, view) in self.views.iter_mut().enumerate() { if blocked || self.active != Some(index) { view.auto_scroll.cancel(); diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index 93c244af..518399f9 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -6,7 +6,7 @@ use egui::{Key, Modifiers}; use egui_kittest::Harness; use egui_kittest::kittest::{NodeT, Queryable}; use pdfcraft_render::{PageRenderer, RenderRequest, RequestKind}; -use pdfcraft_ui_egui::{CloseRequest, PdfCraftApp}; +use pdfcraft_ui_egui::{CloseRequest, Dialog, PdfCraftApp}; /// An `n`-page document with a proper xref table; page `i` shows "Page i+1". fn fixture(n: usize) -> Vec { @@ -356,6 +356,46 @@ fn the_save_prompt_answers_to_the_keyboard() { let _ = std::fs::remove_file(out); } +#[test] +fn keys_leave_the_document_under_a_dialog_alone() { + // Issue #870: ⌘W with Preferences open closed the file underneath it, and in the page grid + // Delete deleted the selected page. While a dialog is open its keys are its own. + let mut h = organize(3); + h.get_by_label("Page 2").click(); + h.run_steps(2); + h.get_by_label("Rotate clockwise").click(); + h.run_steps(3); + assert!(h.state_mut().execute("app.preferences")); + h.run_steps(3); + let shift = Modifiers::COMMAND | Modifiers::SHIFT; + for (m, key) in [ + (Modifiers::COMMAND, Key::W), + (shift, Key::W), + (Modifiers::COMMAND, Key::Z), + (Modifiers::COMMAND, Key::D), + (Modifiers::COMMAND, Key::F), + (Modifiers::NONE, Key::Delete), + (Modifiers::NONE, Key::Backspace), + ] { + h.key_press_modifiers(m, key); + h.run_steps(3); + let app = h.state(); + assert_eq!(app.views.len(), 1, "{m:?}+{key:?} closed the file"); + assert!(app.close_request.is_none(), "{m:?}+{key:?} asked to close the file"); + assert!(app.session.get(app.views[0].id).unwrap().can_undo().is_some(), "{m:?}+{key:?} undid the rotation"); + assert!(app.views[0].find.is_none(), "{m:?}+{key:?} opened Find"); + assert_eq!(app.dialog, Some(Dialog::Preferences), "{m:?}+{key:?} replaced Preferences"); + } + assert_eq!(page_texts(h.state()), ["Page 1", "Page 2", "Page 3"], "a page was deleted under the dialog"); + h.key_press(Key::Escape); + h.run_steps(3); + assert!(h.state().dialog.is_none(), "Escape closes Preferences"); + assert_eq!(h.state().views[0].target_pages(), [1], "Escape went to the dialog, not the page selection"); + h.key_press_modifiers(Modifiers::COMMAND, Key::W); + h.run_steps(3); + assert!(h.state().close_request.is_some(), "with the dialog closed, ⌘W closes the file again"); +} + #[test] fn clean_tabs_close_without_asking() { let mut h = harness(1, |_| {});