From bf409db55c22f125284ea376c9bbf9cb2e47ad69 Mon Sep 17 00:00:00 2001
From: "Hasan N." <42965049+hasannadeem@users.noreply.github.com>
Date: Sun, 8 Nov 2026 19:38:40 -0500
Subject: [PATCH] M3: pages shorter than the window sit in its middle (#5708)
 (#5759)

---
 ROADMAP.md                               |  1 +
 crates/ui-egui/src/canvas.rs             | 35 ++++++++-----
 crates/ui-egui/src/canvas/tests.rs       | 20 ++++++++
 crates/ui-egui/tests/it/dialog_chrome.rs | 16 +++---
 crates/ui-egui/tests/it/view.rs          | 62 ++++++++++++++++++++++++
 docs/roadmap.md                          |  1 +
 parity/acrobat-features.toml             |  4 +-
 7 files changed, 118 insertions(+), 21 deletions(-)

diff --git a/ROADMAP.md b/ROADMAP.md
index 834e61cfd7..2df1bd26ca 100644
--- a/ROADMAP.md
+++ b/ROADMAP.md
@@ -256,6 +256,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md
 
 - **2026-11-02 (M10, #5743):** macOS print jobs name their title and owner (`com.apple.print.JobInfo.PMJobName`, `PMJobOwner`), as the system print dialog does, because a Kyocera filter crashes on a job without them. Checked by a test and through `doc_print` against a stand-in `lp`; not yet on a physical printer.
 
+- **2026-11-02 (M3, #5708):** Pages shorter than the window sit in its middle, one page or row at a time and when scrolling continuously with every page in view.
 - **2026-11-02 (M14, #5737, #5460):** Preferences can turn off the update check at start. It stays on by default, and Help ▸ Check for updates still asks.
 
 - **2026-11-01 (M7, #5657):** Header/footer, watermark and background additions and replacements stage the complete call before committing. Synthetic later-page failures preserve exact incremental bytes, object count, dirty state and allocation cursor; success controls retain other marks and shared/inherited resources. Estimates unchanged.
diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs
index bae59fc980..3e51dac3bf 100644
--- a/crates/ui-egui/src/canvas.rs
+++ b/crates/ui-egui/src/canvas.rs
@@ -2136,8 +2136,7 @@ fn single_page_paging(ui: &egui::Ui, view: &mut DocView, cx: &PagingCx<'_>) -> (
     if !view.continuous {
         // The row's bounding box on screen (one page, or a two-page spread).
         let row_rect = row.iter().filter_map(|&i| rects.get(i)).copied().reduce(Rect::union);
-        // Within a point, so layout rounding can't stop a fitting page from turning.
-        let fits = row_rect.is_some_and(|r| r.height() + 2.0 * MARGIN <= avail.height() + 1.0);
+        let fits = row_rect.is_some_and(|r| fits_height(r, avail.height(), ui.ctx().pixels_per_point()));
         let can_turn = !middle_gesture && unobstructed && ui.rect_contains_pointer(avail);
         single_paging = can_turn && fits;
         // Every wheel event goes to the pager, so it follows each trackpad touch to its end
@@ -2936,6 +2935,28 @@ fn refine_goto_point(mut scroll: egui::ScrollArea, view: &mut DocView, geometry:
     scroll
 }
 
+/// Where the `shown` pages sit in the scroll content, as how far the content starts into the
+/// layout and how tall it is. Scrolling continuously every page scrolls; otherwise only the
+/// current row does, the content being its bounding box plus margins. Pages that fit a view
+/// `view_height` tall sit in its middle, the spare height shared above and below (#5708): the
+/// content fills the view, so there is nothing to scroll, and starts on a whole physical pixel
+/// as the layout's pages do, or their edges would blur and the seams between them open.
+fn vertical_placement(continuous: bool, rects: &[Rect], shown: &[usize], view_height: f32, pixels_per_point: f32) -> (f32, f32) {
+    let Some(r) = shown.iter().filter_map(|&i| rects.get(i)).copied().reduce(Rect::union) else { return (0.0, 0.0) };
+    if fits_height(r, view_height, pixels_per_point) {
+        return (((r.center().y - view_height / 2.0) * pixels_per_point).round() / pixels_per_point, view_height);
+    }
+    if continuous { (0.0, r.bottom() + MARGIN) } else { (r.top() - MARGIN, r.height() + 2.0 * MARGIN) }
+}
+
+/// Whether the pages in `r` show whole in a view `view_height` tall, without their margins.
+/// Within half a physical pixel, which is all that snapping the pages to pixels can add: a
+/// fitting page is centred and has nothing to scroll, so any more would be cut off unseen.
+fn fits_height(r: Rect, view_height: f32, pixels_per_point: f32) -> bool {
+    let slack = if pixels_per_point.is_finite() && pixels_per_point > 0.0 { 0.5 / pixels_per_point } else { 0.0 };
+    r.height() <= view_height + slack
+}
+
 /// Draws the document tab at `index`: its notice bars, then its pages (or the Organize grid) in a
 /// scroll area with this frame's zoom, scrolling, tool input, overlays and render requests.
 pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) {
@@ -3057,15 +3078,7 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) {
     // row, the jump to the new page scrolled both out of view, a blank frame on every turn forward.
     let row = shown_row(view, &rects);
     let visible_pages: Vec<usize> = if view.continuous { (0..rects.len()).collect() } else { row.clone() };
-    let (y_shift, content_h) = if view.continuous {
-        (0.0, rects.iter().map(|r| r.bottom()).reduce(f32::max).map(|bottom| bottom + MARGIN).unwrap_or(0.0))
-    } else {
-        // Only the current row scrolls into view; the content is its bounding box plus margins.
-        match row.iter().filter_map(|&i| rects.get(i)).copied().reduce(Rect::union) {
-            Some(r) => (r.top() - MARGIN, r.height() + 2.0 * MARGIN),
-            None => (0.0, 0.0),
-        }
-    };
+    let (y_shift, content_h) = vertical_placement(view.continuous, &rects, &visible_pages, avail.height(), ui.ctx().pixels_per_point());
 
     let mut scroll = page_scroll_area(
         ui,
diff --git a/crates/ui-egui/src/canvas/tests.rs b/crates/ui-egui/src/canvas/tests.rs
index 013d643990..62c00d6b7a 100644
--- a/crates/ui-egui/src/canvas/tests.rs
+++ b/crates/ui-egui/src/canvas/tests.rs
@@ -2560,3 +2560,23 @@ fn enhancing_a_scan_re_renders_the_page_shown() {
     assert!(app.scan_clean_run.is_none());
     assert_eq!(app.views[i].pages.get(&0).unwrap().tag, STALE_TAG, "the old raster is re-rendered");
 }
+
+/// #5708 review: a page that fits is centred with nothing to scroll, so only pixel snapping may
+/// pass for fitting. A page half a pixel or more too tall scrolls, or its ends would be cut off.
+#[test]
+fn only_pixel_snapping_passes_for_fitting_the_view() {
+    let page = |height: f32| [Rect::from_min_size(pos2(0.0, MARGIN), vec2(300.0, height))];
+    for ppp in [1.0, 2.0] {
+        let slack = 0.5 / ppp;
+        let (_, content) = vertical_placement(false, &page(600.0 + slack), &[0], 600.0, ppp);
+        assert_eq!(content, 600.0, "at {ppp}x, snapping's half pixel still fits");
+        for over in [slack + 0.1, 1.0] {
+            let (shift, content) = vertical_placement(false, &page(600.0 + over), &[0], 600.0, ppp);
+            assert_eq!((shift, content), (0.0, 600.0 + over + 2.0 * MARGIN), "at {ppp}x, {over} pt too tall scrolls");
+        }
+    }
+    // A scale that isn't one has no slack and never panics.
+    for ppp in [0.0, f32::NAN, f32::INFINITY] {
+        assert!(!fits_height(Rect::from_min_size(pos2(0.0, 0.0), vec2(1.0, 600.5)), 600.0, ppp));
+    }
+}
diff --git a/crates/ui-egui/tests/it/dialog_chrome.rs b/crates/ui-egui/tests/it/dialog_chrome.rs
index 47462fcf2b..c495e608ff 100644
--- a/crates/ui-egui/tests/it/dialog_chrome.rs
+++ b/crates/ui-egui/tests/it/dialog_chrome.rs
@@ -236,25 +236,25 @@ fn page_hover_cursors_stay_off_while_a_dialog_is_open() {
         let mut app = PdfCraftApp::new();
         app.set_option("language", "en").unwrap();
         app.open_bytes("form.pdf", None, include_bytes!("../data/form.pdf").to_vec()).unwrap();
-        // Small enough that the check box sits above the dialog card.
-        app.set_option("zoom", "100").unwrap();
+        // Large enough that the drop-down sits below the dialog card.
+        app.set_option("zoom", "300").unwrap();
         app
     });
     for _ in 0..30 {
         h.run_steps(2);
     }
-    // The middle of the "agree" check box (/Rect [20 310 36 326] on a 300 × 400 page).
+    // The middle of the "country" drop-down (/Rect [20 220 200 240] on a 300 × 400 page).
     let page = h.state().views[0].page_screen_rect(0).expect("on screen");
     let k = page.width() / 300.0;
-    let check = page.min + egui::vec2(28.0 * k, (400.0 - 318.0) * k);
-    h.hover_at(check);
+    let field = page.min + egui::vec2(110.0 * k, (400.0 - 230.0) * k);
+    h.hover_at(field);
     h.run_steps(2);
-    assert_eq!(h.output().platform_output.cursor_icon, egui::CursorIcon::PointingHand, "the check box takes clicks");
+    assert_eq!(h.output().platform_output.cursor_icon, egui::CursorIcon::PointingHand, "the drop-down takes clicks");
     h.state_mut().set_option("dialog", "security").unwrap();
     h.run_steps(4);
     let card = card(&h, "dialog");
-    assert!(!card.contains(check), "the check box {check:?} is beside the dialog {card:?}, under its backdrop");
-    h.hover_at(check + egui::vec2(1.0, 0.0));
+    assert!(!card.contains(field), "the drop-down {field:?} is beside the dialog {card:?}, under its backdrop");
+    h.hover_at(field + egui::vec2(1.0, 0.0));
     h.run_steps(2);
     assert_eq!(h.output().platform_output.cursor_icon, egui::CursorIcon::Default, "nothing under the dialog's backdrop can be clicked");
 }
diff --git a/crates/ui-egui/tests/it/view.rs b/crates/ui-egui/tests/it/view.rs
index 59503bba30..f6384127e8 100644
--- a/crates/ui-egui/tests/it/view.rs
+++ b/crates/ui-egui/tests/it/view.rs
@@ -1345,6 +1345,68 @@ fn single_page_fit_page_fits_the_shown_page() {
     assert_ne!(h.state().views[0].zoom, z1, "the wider page refits instead of keeping page 1's zoom");
 }
 
+#[test]
+fn pages_shorter_than_the_window_sit_in_its_middle() {
+    let centred = |h: &Harness<'static, PdfCraftApp>, r: egui::Rect| {
+        let vp = h.state().views[0].viewport_rect();
+        assert!(r.height() < vp.height(), "{r:?} is shorter than {vp:?}");
+        assert!((r.center().y - vp.center().y).abs() < 1.0, "{r:?} is in the middle of {vp:?}");
+    };
+    let mut h = harness(&[("layout", "single"), ("continuous", "off"), ("zoom", "50")]);
+    h.run_steps(4);
+    centred(&h, rect(&h, 0).expect("page 1"));
+    assert_eq!(h.state().views[0].current, 0);
+
+    // The page drawn there is the page a click finds: its link still goes to page 3.
+    let r = rect(&h, 0).expect("page 1");
+    let at = egui::pos2(r.min.x + r.width() * 0.5, r.min.y + r.height() * (1.0 - 325.0 / 400.0));
+    h.hover_at(at);
+    h.run_steps(2);
+    h.drag_at(at); // press
+    h.drop_at(at); // release
+    h.run_steps(8);
+    assert_eq!(h.state().views[0].current, 2, "the link is where the page is drawn");
+
+    // Scrolling continuously, the column of pages as a whole.
+    let mut h = harness(&[("layout", "single"), ("continuous", "on"), ("zoom", "25")]);
+    h.run_steps(4);
+    let column = rect(&h, 0).expect("page 1").union(rect(&h, 2).expect("page 3"));
+    centred(&h, column);
+
+    // A page taller than the view starts at its top, as before.
+    let mut h = harness(&[("layout", "single"), ("continuous", "off"), ("zoom", "250")]);
+    h.run_steps(4);
+    let (r, vp) = (rect(&h, 0).expect("page 1"), h.state().views[0].viewport_rect());
+    assert!(r.height() > vp.height() && (r.min.y - vp.min.y).abs() < 40.0, "{r:?} in {vp:?}");
+}
+
+/// A page too tall for its margins still fits the window: it shows whole, in the middle.
+#[test]
+fn a_page_that_fits_without_its_margins_shows_whole() {
+    let mut h = harness(&[("layout", "single"), ("continuous", "off"), ("zoom", "50")]);
+    h.run_steps(4);
+    let (half, vp) = (rect(&h, 0).expect("page 1"), h.state().views[0].viewport_rect());
+    let spare = 20.0;
+    let zoom = (50.0 * (vp.height() - spare) / half.height()).floor();
+    h.state_mut().set_option("zoom", &zoom.to_string()).unwrap();
+    h.run_steps(4);
+    let r = rect(&h, 0).expect("page 1");
+    assert!(r.height() < vp.height() && r.height() + 2.0 * 28.0 > vp.height(), "{r:?} fits {vp:?} only without its margins");
+    assert!(r.min.y >= vp.min.y && r.max.y <= vp.max.y, "{r:?} shows whole in {vp:?}");
+    assert!((r.center().y - vp.center().y).abs() < 1.0, "{r:?} is in the middle of {vp:?}");
+}
+
+/// Two pages side by side are placed as the row they make: the taller decides, not the first.
+#[test]
+fn a_row_too_tall_for_the_window_starts_at_its_top_whichever_page_is_shorter() {
+    let h = mixed_harness(&[("layout", "two-page"), ("zoom", "180")]);
+    let vp = h.state().views[0].viewport_rect();
+    let (first, second) = (rect(&h, 0).expect("page 1"), rect(&h, 1).expect("page 2"));
+    assert!(first.height() > vp.height() && second.height() < vp.height(), "{first:?}, {second:?} in {vp:?}");
+    let row = first.union(second);
+    assert!((row.min.y - vp.min.y).abs() < 40.0, "{row:?} starts in {vp:?}");
+}
+
 #[test]
 fn clicking_a_link_goes_to_its_destination() {
     // Default fit-width zoom: pages are taller than the window, so the target can scroll to the top.
diff --git a/docs/roadmap.md b/docs/roadmap.md
index 7ce026b3a2..ac9bcbc9e1 100644
--- a/docs/roadmap.md
+++ b/docs/roadmap.md
@@ -324,6 +324,7 @@ Estimates unchanged.
 | 2026-11-03 | patch | M5 #5850 review: indirect typewriter intent and composer edits preserve outer whitespace; movement regression commits by outside click; estimates unchanged. |
 | 2026-11-03 | patch | M5 #5850: Enter adds Fill & Sign line breaks; outside clicks commit, Escape cancels, and saved multiline appearances retain their text and height; estimates unchanged. |
 | 2026-11-02 | patch | M10 #5743: macOS print jobs carry the job name and owner some vendors' filters need; not yet checked on a physical printer |
+| 2026-11-02 | patch | M3 #5708: pages shorter than the window sit in its middle, on a whole physical pixel |
 | 2026-11-02 | patch | M14 #5737, #5460: Preferences turns off the update check at start, on by default |
 | 2026-11-01 | patch | M7 #5657: header/footer, watermark and background operations commit all selected pages together; later-page errors preserve marks, content, shared/inherited resources and pending edits; estimates unchanged. |
 | 2026-10-31 | patch | #1577: reconcile optional page shadows with current viewer/organizer and settings modules; CI validation pending. |
diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml
index 6405daf739..e0c7adf36a 100644
--- a/parity/acrobat-features.toml
+++ b/parity/acrobat-features.toml
@@ -878,8 +878,8 @@ milestone = "M3"
 status = "shipped"
 commands = ["view.layout.single"]
 tools = []
-evidence = ["crates/ui-egui/tests/it/view.rs::single_page_layout_shows_one_page_at_a_time", "crates/ui-egui/tests/it/view.rs::single_page_wheel_pans_instead_when_zoomed_in", "crates/ui-egui/tests/it/view.rs::wheel_turns_one_page_per_notch_and_per_trackpad_swipe", "crates/ui-egui/tests/it/view.rs::single_page_wheel_at_the_bottom_edge_turns_to_the_next_page_top_and_back", "crates/ui-egui/tests/it/view.rs::single_page_swipe_that_reaches_the_edge_does_not_turn_but_a_fresh_one_does", "crates/ui-egui/tests/it/view.rs::rail_page_display_button_switches_layouts", "crates/ui-egui/tests/it/view.rs::view_menu_page_display_uses_the_same_commands"]
-notes = "when the whole page fits, each wheel notch turns a page and each trackpad swipe turns one, momentum included; zoomed in so the page is taller than the window, the wheel pans, and at the bottom (top) edge a wheel notch, or a fresh trackpad swipe, shows the next page at its top (the previous page at its bottom); a swipe that panned to the edge, momentum included, does not turn until the fingers lift and swipe again (#1924). Not verified against Acrobat's own thresholds. Switched from the View menu, the rail's Page display button or the palette"
+evidence = ["crates/ui-egui/tests/it/view.rs::pages_shorter_than_the_window_sit_in_its_middle", "crates/ui-egui/tests/it/view.rs::a_page_that_fits_without_its_margins_shows_whole", "crates/ui-egui/tests/it/view.rs::a_row_too_tall_for_the_window_starts_at_its_top_whichever_page_is_shorter", "crates/ui-egui/tests/it/view.rs::single_page_layout_shows_one_page_at_a_time", "crates/ui-egui/tests/it/view.rs::single_page_wheel_pans_instead_when_zoomed_in", "crates/ui-egui/tests/it/view.rs::wheel_turns_one_page_per_notch_and_per_trackpad_swipe", "crates/ui-egui/tests/it/view.rs::single_page_wheel_at_the_bottom_edge_turns_to_the_next_page_top_and_back", "crates/ui-egui/tests/it/view.rs::single_page_swipe_that_reaches_the_edge_does_not_turn_but_a_fresh_one_does", "crates/ui-egui/tests/it/view.rs::rail_page_display_button_switches_layouts", "crates/ui-egui/tests/it/view.rs::view_menu_page_display_uses_the_same_commands"]
+notes = "a page or row shorter than the window sits in its middle, the spare height shared above and below, continuous scrolling included when every page fits (#5708); when the whole page fits, each wheel notch turns a page and each trackpad swipe turns one, momentum included; zoomed in so the page is taller than the window, the wheel pans, and at the bottom (top) edge a wheel notch, or a fresh trackpad swipe, shows the next page at its top (the previous page at its bottom); a swipe that panned to the edge, momentum included, does not turn until the fingers lift and swipe again (#1924). Not verified against Acrobat's own thresholds. Switched from the View menu, the rail's Page display button or the palette"
 
 [[feature]]
 id = "view.layout-two-up"