Issue #188: page steps in two-page view take every spread when several fit on screen (#265)

The current page is the page with the largest visible height. When more
than one row of pages is wholly on screen (a book with a portrait cover
and landscape spreads, at Fit Width or Fit Page), those heights differ
only by sub-point rounding after the scroll, so the "current" page was
whichever wholly visible page rounding favoured. ⌘Page Down then stepped
from the wrong spread: from the cover it went to 2–3 and then to 6–7,
skipping 4–5 (or, the other way round, stayed put after the cover).

Visible heights within half a point are now equal (the topmost wins),
and the page navigated to stays current while no page shows more of
itself, so the next step starts from it.

Test: viewer.rs::page_down_steps_every_spread_when_several_fit_on_screen
(a 12-page book with a cover page at Fit Width, Fit Page and Fit Height:
⌘Page Down visits 1, 3, 5, 7, 9, 11 and ⌘Page Up 9, 7, 5, 3, 1, 0).
Before the fix it went 1, 5, 7, 11, 11, 11 at Fit Width. Cited on the
view.cover-page parity entry.

Co-authored-by: wstock <1394687+wstock@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Brandon Thomas <echelon@gmail.com>

crates/ui-egui/src/canvas.rs

@@ -1303,8 +1303,12 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) {
         let mut visible_now = Vec::new();
         view.screen_rects.clear();
         view.screen_xforms.clear();
+        // Visible heights (points) closer than this are equal: one page size can meet the
+        // viewport a fraction of a point differently once the scroll offset is applied.
+        const TIE: f32 = 0.5;
         let mut current = view.current;
         let mut best_overlap = -1.0f32;
+        let mut current_overlap = -1.0f32;
         let pointer = ui.input(|i| i.pointer.hover_pos());
         for &i in &visible_pages {
             let r = rects[i].translate(origin.to_vec2());
@@ -1318,7 +1322,12 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) {
                     .push((i, PageXform { rect: r, rot: view.rotation, pw: info.pages[i].width.max(1.0), ph: info.pages[i].height.max(1.0) }));
             }
             let overlap = r.intersect(visible).height();
-            if overlap > best_overlap {
+            if i == view.current {
+                current_overlap = overlap;
+            }
+            // Pages shown equally (two rows wholly on screen) differ only by rounding: the
+            // topmost one counts.
+            if overlap > best_overlap + TIE {
                 best_overlap = overlap;
                 current = i;
             }
@@ -1678,6 +1687,11 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) {
                 }
             }
         }
+        // The page navigated to stays current while no page shows more of itself, so the next
+        // page step starts from it rather than from a page further down the screen (#188).
+        if current_overlap >= best_overlap - TIE {
+            current = view.current;
+        }
         if view.layout != PageLayout::Single {
             view.current = current;
             if !ui.memory(|m| m.has_focus(egui::Id::new("page-input"))) {

crates/ui-egui/tests/viewer.rs

@@ -471,3 +471,68 @@ fn arrow_and_page_keys_move_through_a_scrolling_document() {
     h.run_steps(4);
     assert_eq!(press(&mut h, Key::PageDown, 1), 3);
 }
+
+/// Like [`fixture`], with each page's size given.
+fn sized_fixture(sizes: &[(u32, u32)]) -> Vec<u8> {
+    let n = sizes.len();
+    let mut objs: Vec<String> = vec!["<< /Type /Catalog /Pages 2 0 R >>".into()];
+    let kids: Vec<String> = (0..n).map(|i| format!("{} 0 R", 4 + 2 * i)).collect();
+    objs.push(format!("<< /Type /Pages /Kids [{}] /Count {n} >>", kids.join(" ")));
+    objs.push("<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into());
+    for (i, (w, h)) in sizes.iter().enumerate() {
+        objs.push(format!(
+            "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 {w} {h}] /Contents {} 0 R /Resources << /Font << /F1 3 0 R >> >> >>",
+            5 + 2 * i
+        ));
+        let body = format!("BT /F1 18 Tf 20 150 Td (Page {}) Tj ET", i + 1);
+        objs.push(format!("<< /Length {} >>\nstream\n{body}\nendstream", body.len()));
+    }
+    let mut out = b"%PDF-1.7\n".to_vec();
+    let mut offs = Vec::new();
+    for (i, o) in objs.iter().enumerate() {
+        offs.push(out.len());
+        out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes());
+    }
+    let x = out.len();
+    out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes());
+    for o in offs {
+        out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+    }
+    out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{x}\n%%EOF\n", objs.len() + 1).as_bytes());
+    out
+}
+
+#[test]
+fn page_down_steps_every_spread_when_several_fit_on_screen() {
+    // #188: in two-page view with a cover page, ⌘Page Down skipped spreads (or stuck) once more
+    // than one spread fit on screen: the current page was whichever wholly visible page rounding
+    // made "most visible", so the next step started from the wrong spread.
+    use egui::{Key, Modifiers};
+    use pdfcraft_ui_egui::canvas::{Fit, PageLayout};
+    // a printed book: portrait cover and back, landscape two-page spreads between
+    let mut sizes = vec![(200, 300)];
+    sizes.extend([(400, 300); 10]);
+    sizes.push((200, 300));
+    for fit in [Fit::Width, Fit::Page, Fit::Height] {
+        let bytes = sized_fixture(&sizes);
+        let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(move |_cc| {
+            let mut app = PdfCraftApp::new();
+            app.open_bytes("book.pdf", None, bytes).unwrap();
+            app
+        });
+        h.run_steps(6);
+        h.state_mut().active = Some(0);
+        let v = &mut h.state_mut().views[0];
+        (v.fit, v.layout, v.cover) = (fit, PageLayout::TwoUp, true);
+        h.run_steps(6);
+        let mut step = |key| {
+            h.key_press_modifiers(Modifiers::COMMAND, key);
+            h.run_steps(4);
+            h.state().views[0].current
+        };
+        let down: Vec<usize> = (0..6).map(|_| step(Key::PageDown)).collect();
+        assert_eq!(down, [1, 3, 5, 7, 9, 11], "{fit:?}: cover, five spreads, back cover");
+        let up: Vec<usize> = (0..6).map(|_| step(Key::PageUp)).collect();
+        assert_eq!(up, [9, 7, 5, 3, 1, 0], "{fit:?}");
+    }
+}

parity/acrobat-features.toml

@@ -866,8 +866,8 @@ milestone = "M3"
 status = "shipped"
 commands = ["view.layout.cover"]
 tools = []
-evidence = ["crates/ui-egui/tests/viewer.rs::layouts_fit_height_labels_and_system_theme", "crates/ui-egui/tests/view.rs::page_display_commands_switch_layouts"]
-notes = "View ▸ Page display, the rail's Page display button or the palette; disabled outside two-page view"
+evidence = ["crates/ui-egui/tests/viewer.rs::layouts_fit_height_labels_and_system_theme", "crates/ui-egui/tests/view.rs::page_display_commands_switch_layouts", "crates/ui-egui/tests/viewer.rs::page_down_steps_every_spread_when_several_fit_on_screen"]
+notes = "View ▸ Page display, the rail's Page display button or the palette; disabled outside two-page view; next / previous page step one spread at a time even when several spreads fit on screen"
 
 [[feature]]
 id = "view.page-gaps"