From f866d995c80f4644d96974d0b5e2bf01515e99c7 Mon Sep 17 00:00:00 2001 From: wstock Date: Thu, 8 Oct 2026 13:11:30 +0100 Subject: [PATCH] Issue #185: arrow keys and Page Down / Up move through a scrolling document (#261) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the continuous (default) and two-page views, → / ← and Page Down / Page Up only did something with ⌘ held, and ↓ / ↑ never did: egui's ScrollArea has no keyboard scrolling of its own, so without a modifier the keyboard could not move through the document at all. As in Acrobat: → / ← now go to the next / previous page (a spread in two-page view) in every layout; ↓ / ↑ scroll a line (40 pt, a desktop mouse-wheel line) and Page Down / Page Up scroll a screen (less one line) where the pages scroll. ⌘ + those keys and single-page view keep their page steps. The scroll is queued on the DocView and applied by the page view's ScrollArea in the same frame, next to the middle-button auto-scroll. The shortcuts list shows ← / → beside ⌘← / ⌘→; the view.page-navigation parity entry cites the new test. Test: viewer.rs::arrow_and_page_keys_move_through_a_scrolling_document (continuous at Fit Width: → / ← turn pages; ↓ and Page Down take several presses to reach page 2, ↓ more than Page Down, and ↑ / Page Up come back; → in two-page view and Page Down in single-page view). It failed at the first → before the fix. Co-authored-by: wstock <1394687+wstock@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 40 ++++++++++++++++++++++------ crates/ui-egui/src/dialogs.rs | 2 +- crates/ui-egui/tests/viewer.rs | 48 ++++++++++++++++++++++++++++++++++ parity/acrobat-features.toml | 4 +-- 4 files changed, 83 insertions(+), 11 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 604c6e7e52..bd175e6405 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -197,6 +197,9 @@ pub struct DocView { pub forward: Vec, /// Pending navigation: page and fraction down the page to align with the viewport top. pub goto: Option<(usize, f32)>, + /// Pending keyboard scrolling, in points down (negative: up): ↓ / ↑, and Page Down / + /// Page Up where the pages scroll. + pub key_scroll: f32, /// Briefly outline an annotation after navigating to it from a panel. pub flash: Option<(usize, [f32; 4], f64)>, /// Compare files: differences shaded on this document's pages (page, user-space box, colour). @@ -331,6 +334,7 @@ impl DocView { back: Vec::new(), forward: Vec::new(), goto: None, + key_scroll: 0.0, flash: None, compare_marks: Vec::new(), pages: HashMap::new(), @@ -1048,18 +1052,36 @@ pub fn shortcuts(view: &mut DocView, ctx: &egui::Context) { if key(Key::End) { view.go_to_page(usize::MAX); } - if view.layout == PageLayout::Single || ctx.input(|i| i.modifiers.command) { - if key(Key::ArrowRight) || key(Key::PageDown) { - view.step_page(true); - } - if key(Key::ArrowLeft) || key(Key::PageUp) { - view.step_page(false); + // As in Acrobat: → / ← go to the next / previous page in every layout (#185), and so do + // ⌘Page Down / ⌘Page Up, or plain Page Down / Page Up in single-page view. + let command = ctx.input(|i| i.modifiers.command); + let scrolls = view.layout != PageLayout::Single; + if key(Key::ArrowRight) || (key(Key::PageDown) && (command || !scrolls)) { + view.step_page(true); + } + if key(Key::ArrowLeft) || (key(Key::PageUp) && (command || !scrolls)) { + view.step_page(false); + } + // ↓ / ↑ scroll a line, and Page Down / Page Up a screen where the pages scroll. + if !command { + let screen = (view.viewport_h - KEY_SCROLL_LINE).max(KEY_SCROLL_LINE); + let steps = [(Key::ArrowDown, KEY_SCROLL_LINE), (Key::ArrowUp, -KEY_SCROLL_LINE), (Key::PageDown, screen), (Key::PageUp, -screen)]; + for (k, by) in steps { + if key(k) && (scrolls || matches!(k, Key::ArrowDown | Key::ArrowUp)) { + view.key_scroll += by; + } } } } +/// How far ↓ / ↑ scroll: a mouse-wheel line on the desktop. Page Down / Page Up keep this much +/// of the previous screen in view. +const KEY_SCROLL_LINE: f32 = 40.0; + pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { let t = Tokens::get(ui.ctx()); + // Keyboard scrolling applies to this frame's page view only, never later. + let key_scroll = std::mem::take(&mut app.views[index].key_scroll); // The Search panel closed: its search moves to the find bar. let search_open = app.right == Some(crate::RightPanel::Search); if let Some(f) = app.views[index].find.as_mut() @@ -1259,8 +1281,10 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { // Only interactions pause; the document must keep its original colours. ui.set_opacity(opacity); } - if auto_delta != Vec2::ZERO { - ui.scroll_with_delta_animation(auto_delta, egui::style::ScrollAnimation::none()); + // Middle-button auto-scroll and the keyboard (positive y moves the content down). + let delta = auto_delta - vec2(0.0, key_scroll); + if delta != Vec2::ZERO { + ui.scroll_with_delta_animation(delta, egui::style::ScrollAnimation::none()); } let (resp_rect, resp) = ui.allocate_exact_size(vec2(content_w, content_h), Sense::click_and_drag()); // The Hand tool pans: the content widget takes every drag, so scroll by its delta. diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 533dcf8fdd..5abbbd865a 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1030,7 +1030,7 @@ pub fn show(app: &mut PdfCraftApp, ctx: &egui::Context) { ("⌘+ / ⌘−", tl!("Zoom in / out (also pinch or ⌘-scroll)")), ("⇧⌘+ / ⇧⌘−", tl!("Rotate view")), ("Home / End", tl!("First / last page")), - ("⌘← / ⌘→", tl!("Previous / next page")), + ("← / →, ⌘← / ⌘→", tl!("Previous / next page")), ("Delete", tl!("Delete selected pages (Organize)")), ("⌘A", tl!("Select all pages (Organize)")), ] { diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index 13a8f12332..ca80901024 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -423,3 +423,51 @@ fn each_document_keeps_its_own_scroll_position() { assert_eq!(show(&mut h, 0), 3); assert_eq!(show(&mut h, 1), 1); } + +#[test] +fn arrow_and_page_keys_move_through_a_scrolling_document() { + // #185: in the continuous (default) and two-page views the arrow keys and Page Down / Up did + // nothing without ⌘. + use egui::Key; + use pdfcraft_ui_egui::canvas::{Fit, PageLayout}; + let mut h = harness(); + h.state_mut().active = Some(0); + h.state_mut().views[0].fit = Fit::Width; + h.run_steps(4); + assert_eq!(h.state().views[0].layout, PageLayout::Continuous); + let press = |h: &mut Harness<'static, PdfCraftApp>, key, times: usize| { + for _ in 0..times { + h.key_press(key); + h.run_steps(2); + } + h.run_steps(2); + h.state().views[0].current + }; + // → / ← turn pages, as in Acrobat + assert_eq!(press(&mut h, Key::ArrowRight, 2), 2); + assert_eq!(press(&mut h, Key::ArrowLeft, 1), 1); + assert_eq!(press(&mut h, Key::ArrowLeft, 1), 0); + // ↓ scrolls a line at a time, Page Down a screen: count the presses that reach page 2 + let presses = |h: &mut Harness<'static, PdfCraftApp>, key| { + let mut n = 0; + while h.state().views[0].current == 0 { + press(h, key, 1); + n += 1; + assert!(n < 500, "{key:?} never reached page 2"); + } + n + }; + let lines = presses(&mut h, Key::ArrowDown); + assert_eq!(press(&mut h, Key::ArrowUp, lines + 1), 0); + let screens = presses(&mut h, Key::PageDown); + assert_eq!(press(&mut h, Key::PageUp, screens + 1), 0); + assert!(screens >= 2 && lines > 2 * screens, "a fit-width page: {lines} lines, {screens} screens"); + // two-page view: → moves a spread + h.state_mut().views[0].layout = PageLayout::TwoUp; + h.run_steps(4); + assert_eq!(press(&mut h, Key::ArrowRight, 1), 2); + // single-page view keeps Page Down for the next page + h.state_mut().views[0].layout = PageLayout::Single; + h.run_steps(4); + assert_eq!(press(&mut h, Key::PageDown, 1), 3); +} diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 0ea77eba61..c0eb3259c0 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -1106,8 +1106,8 @@ milestone = "M3" status = "shipped" commands = [] tools = [] -evidence = ["crates/ui-egui/tests/control.rs::state_and_view_options", "crates/ui-egui/tests/ui.rs::theme_and_view_options_apply", "crates/ui-egui/tests/viewer.rs::two_page_view_steps_a_spread_at_a_time"] -notes = "in two-page view, next and previous page move a whole spread (with or without a cover page)" +evidence = ["crates/ui-egui/tests/control.rs::state_and_view_options", "crates/ui-egui/tests/ui.rs::theme_and_view_options_apply", "crates/ui-egui/tests/viewer.rs::two_page_view_steps_a_spread_at_a_time", "crates/ui-egui/tests/viewer.rs::arrow_and_page_keys_move_through_a_scrolling_document"] +notes = "in two-page view, next and previous page move a whole spread (with or without a cover page); → / ← turn pages in every layout, ↓ / ↑ scroll a line and Page Down / Page Up a screen (the page in single-page view), as in Acrobat" [[feature]] id = "view.goto-page-label"