From 46ac4fc8f3af8579ee7dbfe6d323bacbcbbfd650 Mon Sep 17 00:00:00 2001 From: David Mann Date: Sun, 25 Oct 2026 02:28:59 +0100 Subject: [PATCH 1/3] Crop: refuse an invalid explicit rectangle instead of cropping to the selection (#9279) image.crop built an explicit rectangle only when x, y, width and height were all integers with width and height above 0; anything else fell back to the selection's bounds, so width 0, a negative or string size, or only some of the four keys cropped to the selection and reported success. A rectangle that is passed (any of the four keys non-null) but incomplete or empty is now a bad-params error and the document is unchanged; {} still crops to the selection. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/image_cmds.rs | 53 ++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/image_cmds.rs b/crates/engine/src/image_cmds.rs index 6353f1f2cc..24f39f2184 100644 --- a/crates/engine/src/image_cmds.rs +++ b/crates/engine/src/image_cmds.rs @@ -567,6 +567,7 @@ fn crop(s: &mut Session, p: &Value) -> Result { }; // Values beyond i32 would wrap through the narrowing casts into a rectangle unrelated to // the numbers passed; reject them (see `commands::int_i32`). + let given = ["x", "y", "width", "height"].into_iter().any(|k| p.get(k).is_some_and(|v| !v.is_null())); let explicit = match ( crate::commands::int_i32("image.crop", p, "x")?, crate::commands::int_i32("image.crop", p, "y")?, @@ -587,6 +588,14 @@ fn crop(s: &mut Session, p: &Value) -> Result { }; Some(Rect::new(x, y, end(x, w, "x", "width")?, end(y, h, "y", "height")?)) } + // A rectangle that is passed but incomplete or empty is refused: falling back to the + // selection would crop somewhere the caller didn't ask for (#9279). + _ if given => { + return Err(bad( + "image.crop", + "`x`, `y`, `width` and `height` go together: pass all four as numbers of pixels, with `width` and `height` rounding to at least 1", + )); + } _ => None, }; let target = crop_target(p)?; @@ -975,7 +984,7 @@ pub fn specs() -> Vec { "image.crop", "Crop", ["Image"], - r##"{"x":px,"y":px,"width":px,"height":px,"angle":-3600..3600=0,"deleteCroppedPixels":bool=true,"contentAware":bool=false,"targetWidth":px,"targetHeight":px,"resolution":ppi,"resample":"bicubic|bilinear|nearest|lanczos|preserveDetails"="bicubic"} → {x,y,width,height,documentWidth,documentHeight,resolution} (targetWidth/targetHeight, both or neither: the cropped area is resampled to exactly that size, as Image Size, in the same undo step; resolution: the document's new ppi; contentAware: the Crop tool's Fill › Content-Aware Fill, the area a rotated or extending crop uncovers filled from its surroundings on the Background, else the active pixel layer)"##, + r##"{"x":px,"y":px,"width":px,"height":px,"angle":-3600..3600=0,"deleteCroppedPixels":bool=true,"contentAware":bool=false,"targetWidth":px,"targetHeight":px,"resolution":ppi,"resample":"bicubic|bilinear|nearest|lanczos|preserveDetails"="bicubic"} → {x,y,width,height,documentWidth,documentHeight,resolution} (x/y/width/height, all four or none: without them the selection's bounds are cropped to; targetWidth/targetHeight, both or neither: the cropped area is resampled to exactly that size, as Image Size, in the same undo step; resolution: the document's new ppi; contentAware: the Crop tool's Fill › Content-Aware Fill, the area a rotated or extending crop uncovers filled from its surroundings on the Background, else the active pixel layer)"##, has_doc, crop ), @@ -1423,6 +1432,48 @@ mod tests { assert_eq!(doc(&s).layers[1].surface().unwrap().pixel(20, 5), vec![1.0, 0.0, 0.0, 1.0]); } + /// A rectangle that is passed but invalid is refused, not replaced by the selection (#9279). + #[test] + fn crop_refuses_a_bad_explicit_rect_instead_of_cropping_to_the_selection() { + let bad = [ + json!({"x": 10, "y": 10, "width": 0, "height": 50}), + json!({"x": 10, "y": 10, "width": 20, "height": 0}), + json!({"x": 10, "y": 10, "width": -50, "height": 50}), + json!({"x": 10, "y": 10, "width": "100", "height": 50}), + json!({"x": 10, "y": 10, "width": true, "height": 50}), + json!({"width": 20, "height": 10}), + json!({"x": 1, "y": 1}), + json!({"x": 1, "y": null, "width": 20, "height": 10}), + json!({"height": 10}), + ]; + for p in bad { + for selection in [true, false] { + let mut s = session(); + if selection { + s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap(); + } + let (size, past) = (doc(&s).size, s.active().unwrap().history.past_len()); + let err = s.execute("image.crop", p.clone()).unwrap_err(); + assert!(matches!(err, EngineError::BadParams { .. }), "{p} (selection: {selection}): {err:?}"); + assert_eq!(doc(&s).size, size, "{p} (selection: {selection})"); + assert_eq!(doc(&s).selection.is_some(), selection, "{p}: the selection stays"); + assert_eq!(s.active().unwrap().history.past_len(), past, "{p} (selection: {selection})"); + } + } + // No rectangle at all (or every side null) still crops to the selection. + for p in [json!({}), json!({"x": null, "y": null, "width": null, "height": null})] { + let mut s = session(); + s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap(); + s.execute("image.crop", p.clone()).unwrap(); + assert_eq!(doc(&s).size, Size::new(20, 10), "{p}"); + } + // Whole-number floats, as the Crop tool sends them, are a valid rectangle. + let mut s = session(); + s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap(); + s.execute("image.crop", json!({"x": 1.0, "y": 1.0, "width": 30.0, "height": 15.0})).unwrap(); + assert_eq!(doc(&s).size, Size::new(30, 15)); + } + #[test] fn crop_extension_fills_the_background_with_the_background_colour() { let mut shrink = Session::new(); From 7a1d2dfd4c64bc19fca46cf8d5cefe1c88b9684a Mon Sep 17 00:00:00 2001 From: David Mann Date: Thu, 29 Oct 2026 17:27:14 +0100 Subject: [PATCH 2/3] Tests: image.crop rounds fractional rectangle sizes like its other pixel params Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/image_cmds/tests.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/engine/src/cmd/image_cmds/tests.rs b/crates/engine/src/cmd/image_cmds/tests.rs index fb6f1ec883..d877e0270e 100644 --- a/crates/engine/src/cmd/image_cmds/tests.rs +++ b/crates/engine/src/cmd/image_cmds/tests.rs @@ -427,6 +427,14 @@ fn crop_refuses_a_bad_explicit_rect_instead_of_cropping_to_the_selection() { s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap(); s.execute("image.crop", json!({"x": 1.0, "y": 1.0, "width": 30.0, "height": 15.0})).unwrap(); assert_eq!(doc(&s).size, Size::new(30, 15)); + // Fractional pixels round, like the rectangle's `x`/`y` and every other pixel param + // (`commands::int`); a side that rounds to 0 is refused. + let mut s = session(); + s.execute("image.crop", json!({"x": 0.4, "y": 0.6, "width": 20.6, "height": 9.5})).unwrap(); + assert_eq!(doc(&s).size, Size::new(21, 10)); + let err = s.execute("image.crop", json!({"x": 0, "y": 0, "width": 0.4, "height": 5})).unwrap_err(); + assert!(matches!(err, EngineError::BadParams { .. }), "{err:?}"); + assert_eq!(doc(&s).size, Size::new(21, 10)); } #[test] From 2037705698ed9e0001f2f19530cfeb58b7131285 Mon Sep 17 00:00:00 2001 From: David Mann Date: Fri, 6 Nov 2026 01:17:36 +0100 Subject: [PATCH 3/3] Address review: pin crop's 0 origin, numeric strings, partly-outside rect and Reveal All x/y of 0 count as given, numeric strings are refused on every side, a rectangle partly past the canvas grows it as passed, and Reveal All (crop_keeping_background) still crops to all four sides with a selection active. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/image_cmds/tests.rs | 45 +++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/crates/engine/src/cmd/image_cmds/tests.rs b/crates/engine/src/cmd/image_cmds/tests.rs index bf414dc189..b35040cb5d 100644 --- a/crates/engine/src/cmd/image_cmds/tests.rs +++ b/crates/engine/src/cmd/image_cmds/tests.rs @@ -437,6 +437,51 @@ fn crop_refuses_a_bad_explicit_rect_instead_of_cropping_to_the_selection() { assert_eq!(doc(&s).size, Size::new(21, 10)); } +/// The explicit rectangle of #9279's edge cases, each with a selection elsewhere that a fallback +/// would crop to: `0` is a given origin, numeric strings are not numbers (refused, as `x`/`y` +/// already were), and a rectangle partly past the canvas is used as passed, never clamped. +#[test] +fn crop_rect_edge_cases_never_fall_back_to_the_selection() { + let with_selection = || { + let mut s = session(); + s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap(); + s + }; + // `x`/`y` of 0 count as given: the red square (10, 5)..(20, 15) stays where it was, where a + // crop to the selection would have moved it to (5, 3). + let mut s = with_selection(); + s.execute("image.crop", json!({"x": 0, "y": 0, "width": 15, "height": 10})).unwrap(); + assert_eq!(doc(&s).size, Size::new(15, 10)); + assert_eq!(doc(&s).layers[1].surface().unwrap().content_bounds(), Rect::new(10, 5, 15, 10)); + // Numeric strings are refused on every side, with or without a selection. + for p in [ + json!({"x": "0", "y": "0", "width": "15", "height": "10"}), + json!({"x": "0", "y": 0, "width": 15, "height": 10}), + json!({"x": 0, "y": 0, "width": 15, "height": "10"}), + ] { + for mut s in [with_selection(), session()] { + let err = s.execute("image.crop", p.clone()).unwrap_err(); + assert!(matches!(err, EngineError::BadParams { .. }), "{p}: {err:?}"); + assert_eq!(doc(&s).size, Size::new(40, 20), "{p}"); + } + } + // Partly outside the canvas: the canvas grows to the rectangle, not to its overlap with the + // canvas or the selection. + let mut s = with_selection(); + s.execute("image.crop", json!({"x": -10, "y": -5, "width": 30, "height": 40})).unwrap(); + assert_eq!(doc(&s).size, Size::new(30, 40)); + assert_eq!(doc(&s).layers[1].surface().unwrap().pixel(20, 10), vec![1.0, 0.0, 0.0, 1.0]); + // Reveal All passes all four sides through the same check (here x = y = 0: the hidden pixels + // are past the right and bottom edges), so a selection doesn't change what it reveals. + let mut s = with_selection(); + s.execute("image.canvasSize", json!({"width": 12, "height": 8, "anchor": "topLeft"})).unwrap(); + s.execute("select.rect", json!({"x": 2, "y": 2, "width": 4, "height": 4})).unwrap(); + assert_eq!(s.execute("image.revealAll", json!({})).unwrap()["changed"], true); + let size = doc(&s).size; + assert!(size.width >= 20 && size.height >= 15, "{size:?}: the red square is revealed"); + assert_eq!(doc(&s).layers[1].surface().unwrap().pixel(19, 14), vec![1.0, 0.0, 0.0, 1.0]); +} + #[test] fn crop_extension_fills_the_background_with_the_background_colour() { let mut shrink = Session::new();