M8: Zoom to selection centres a point above the toolbar and ignores non-finite rectangles; tests pin the margins (#2139 review)

fit_selection checks every coordinate is finite (a NaN y or x1 left a NaN centre) and leaves the view as it is for such a rectangle. A point selection is centred in the area above the toolbar at its zoom, not in the whole viewport. Tests pin the exact short-canvas and no-inset positions, the narrow-canvas margins, lines, tiny and huge rectangles, the bottomInset clamp and that it is never saved, the unchanged Zoom to fit centre, the pill strip the app hands the engine (shown and hidden), and the Shift+2 centre without reading the inset back.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Model-Effort: Opus 5.5 triaged and briefed; Sonnet 5.5 verified and wrote at medium

crates/engine/src/view_behaviour_tests.rs

@@ -806,6 +806,9 @@ fn zoom_to_selection_on_a_wide_layer_is_limited_by_the_width() {
 fn zoom_to_selection_without_a_toolbar_inset_uses_the_whole_height() {
     let (v, _) = zoom_on("view.zoomSelection", 0.0, 100.0, 100.0);
     assert!((v.zoom - (903.0 - 80.0) / 100.0).abs() < 1e-6, "{}", v.zoom);
+    let (_, top) = v.page_to_screen(0.0, 0.0);
+    let (_, bottom) = v.page_to_screen(100.0, 100.0);
+    assert!((top - 40.0).abs() < 1e-6 && (bottom - 863.0).abs() < 1e-6, "top {top}, bottom {bottom}");
 }
 
 #[test]
@@ -822,6 +825,86 @@ fn zoom_to_selection_in_a_short_canvas_still_ends_above_the_toolbar() {
     let (_, bottom) = v.page_to_screen(100.0, 100.0);
     assert!(top >= 0.0 && bottom <= 70.0 + 1e-6, "top {top}, bottom {bottom}");
     assert!((top - (70.0 - bottom)).abs() < 1e-6, "centred above the toolbar: top {top}, bottom {bottom}");
+    // The margin shrinks to a quarter of the 70 points (17.5) and the zoom is the exact fit, not the floor.
+    assert!((v.zoom - 0.35).abs() < 1e-9, "{}", v.zoom);
+    assert!((top - 17.5).abs() < 1e-6 && (bottom - 52.5).abs() < 1e-6, "top {top}, bottom {bottom}");
+}
+
+/// A canvas narrower than 160 points keeps a quarter of its width on each side.
+#[test]
+fn zoom_to_selection_in_a_narrow_canvas_shrinks_the_side_margins() {
+    let mut s = Session::new();
+    let r = rect(&mut s, 0.0, 0.0, 100.0, 100.0, None);
+    ex(&mut s, "view.set", json!({"vw": 100, "vh": 903, "bottomInset": 0}));
+    ex(&mut s, "select.set", json!({"ids": [r]}));
+    ex(&mut s, "view.zoomSelection", json!({}));
+    let v = view(&s);
+    let (left, _) = v.page_to_screen(0.0, 0.0);
+    let (right, _) = v.page_to_screen(100.0, 100.0);
+    assert!((v.zoom - 0.5).abs() < 1e-9, "{}", v.zoom);
+    assert!((left - 25.0).abs() < 1e-6 && (right - 75.0).abs() < 1e-6, "left {left}, right {right}");
+}
+
+/// A view 1000 by 800 with a 100-point toolbar strip, for the edge cases of the fit.
+fn strip_view() -> ViewState {
+    ViewState { vw: 1000.0, vh: 800.0, bottom_inset: 100.0, ..ViewState::default() }
+}
+
+#[test]
+fn fitting_a_selection_survives_points_lines_extremes_and_junk() {
+    // A point: the zoom stays, and it lands in the middle of the area above the toolbar (y = 350).
+    let mut v = strip_view();
+    v.zoom = 2.0;
+    v.fit_selection(Rect::new(10.0, 20.0, 10.0, 20.0), 32.0);
+    assert_eq!(v.zoom, 2.0);
+    let (x, y) = v.page_to_screen(10.0, 20.0);
+    assert!((x - 500.0).abs() < 1e-9 && (y - 350.0).abs() < 1e-9, "{x}, {y}");
+    // A zero-width line is limited by its height, a zero-height one by its width.
+    let mut v = strip_view();
+    v.fit_selection(Rect::new(0.0, 0.0, 0.0, 100.0), 32.0);
+    assert!((v.zoom - (700.0 - 80.0) / 100.0).abs() < 1e-9, "{}", v.zoom);
+    let mut v = strip_view();
+    v.fit_selection(Rect::new(0.0, 0.0, 100.0, 0.0), 32.0);
+    assert!((v.zoom - (1000.0 - 80.0) / 100.0).abs() < 1e-9, "{}", v.zoom);
+    // A tiny one stops at the maximum zoom, a huge one at the minimum.
+    let mut v = strip_view();
+    v.fit_selection(Rect::new(0.0, 0.0, 0.001, 0.001), 32.0);
+    assert_eq!(v.zoom, 32.0);
+    v.fit_selection(Rect::new(0.0, 0.0, 1e12, 1e12), 32.0);
+    assert_eq!(v.zoom, ViewState::MIN_ZOOM);
+    // A rectangle with any coordinate that is not a finite number changes nothing.
+    for bad in [
+        Rect::new(0.0, f64::NAN, 10.0, 10.0),
+        Rect::new(0.0, 0.0, f64::INFINITY, 10.0),
+        Rect::new(0.0, 0.0, 10.0, f64::NAN),
+        Rect::new(f64::NAN, 0.0, 10.0, 10.0),
+    ] {
+        let mut v = strip_view();
+        let before = (v.zoom, v.cx, v.cy);
+        v.fit_selection(bad, 32.0);
+        assert_eq!((v.zoom, v.cx, v.cy), before, "{bad:?}");
+    }
+}
+
+#[test]
+fn the_bottom_inset_is_clamped_and_never_saved() {
+    let mut s = Session::new();
+    ex(&mut s, "view.set", json!({"bottomInset": -5}));
+    assert_eq!(view(&s).bottom_inset, 0.0);
+    ex(&mut s, "view.set", json!({"bottomInset": 1e12}));
+    assert_eq!(view(&s).bottom_inset, 20000.0);
+    // Larger than the viewport: the fit still leaves a sliver of canvas and a finite centre.
+    ex(&mut s, "view.set", json!({"vw": 400, "vh": 300, "bottomInset": 5000}));
+    let r = rect(&mut s, 0.0, 0.0, 10.0, 10.0, None);
+    ex(&mut s, "select.set", json!({"ids": [r]}));
+    ex(&mut s, "view.zoomSelection", json!({}));
+    let v = view(&s);
+    assert!(v.zoom.is_finite() && v.cx.is_finite() && v.cy.is_finite(), "{v:?}");
+    // It is not part of what the view saves.
+    let saved = serde_json::to_string(&v).unwrap();
+    assert!(!saved.to_lowercase().contains("inset"), "{saved}");
+    let back: ViewState = serde_json::from_str(&saved).unwrap();
+    assert_eq!(back.bottom_inset, 0.0);
 }
 
 #[test]
@@ -831,5 +914,8 @@ fn zoom_to_fit_is_unchanged() {
     rect(&mut s, 0.0, 0.0, 100.0, 100.0, None);
     ex(&mut s, "view.set", json!({"vw": 1389, "vh": 903, "bottomInset": 58}));
     ex(&mut s, "view.zoomFit", json!({}));
-    assert!((view(&s).zoom - (903.0 - 128.0) / 100.0).abs() < 1e-6);
+    let v = view(&s);
+    assert!((v.zoom - (903.0 - 128.0) / 100.0).abs() < 1e-6);
+    // No toolbar offset: the rectangle's own centre stays in the middle of the whole viewport.
+    assert_eq!((v.cx, v.cy), (50.0, 50.0));
 }

crates/engine/src/view_state.rs

@@ -113,13 +113,18 @@ impl ViewState {
     /// Fit a page rectangle for Zoom to selection: a 40 point margin on all four sides of the area
     /// above the bottom toolbar (`vw` by `vh - bottom_inset`), centred in that area.
     pub fn fit_selection(&mut self, r: figcraft_geom::Rect, max_zoom: f64) {
-        if !(r.width() > 0.0 || r.height() > 0.0) || !r.x0.is_finite() {
-            self.cx = r.center().x;
-            self.cy = r.center().y;
+        // A rectangle that isn't all finite numbers leaves the view as it is.
+        if ![r.x0, r.y0, r.x1, r.y1].iter().all(|v| v.is_finite()) {
             return;
         }
         let inset = if self.bottom_inset.is_finite() { self.bottom_inset.clamp(0.0, (self.vh - 1.0).max(0.0)) } else { 0.0 };
         let area_h = (self.vh - inset).max(1.0);
+        // A point (nothing to fit) is centred in the area above the toolbar at the zoom it has.
+        if !(r.width() > 0.0 || r.height() > 0.0) {
+            self.cx = r.center().x;
+            self.cy = r.center().y + (self.vh - area_h) / 2.0 / self.zoom.max(1e-6);
+            return;
+        }
         // 40 points as measured; a canvas too small for that keeps a quarter of its size on each
         // side instead, so the selection still ends above the toolbar.
         let mx = (self.vw / 4.0).clamp(0.0, 40.0);