From 8303cfa56f19fd11c13bb4f6933edd5717c7de03 Mon Sep 17 00:00:00 2001 From: RealtaNua Date: Fri, 16 Oct 2026 22:52:41 +0700 Subject: [PATCH] fix(engine): Shift-drag on a selected layer moves the selection (#498) Shift on a selected layer took it out of the selection on press, so the drag that followed had nothing to move, and with several layers selected the grabbed one stayed behind. It now leaves the selection on release, and only when the press didn't become a drag. Where no move can start (a version being viewed), it still leaves on press. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../engine/src/selection_behaviour_tests.rs | 89 +++++++++++++++++++ crates/engine/src/tools.rs | 38 +++++--- docs/ui-parity.md | 2 +- 3 files changed, 115 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/selection_behaviour_tests.rs b/crates/engine/src/selection_behaviour_tests.rs index b65b7936..bc961a4a 100644 --- a/crates/engine/src/selection_behaviour_tests.rs +++ b/crates/engine/src/selection_behaviour_tests.rs @@ -275,3 +275,92 @@ fn locked_layers_selected_in_the_layers_panel_stay_put() { ex(&mut s, "object.toggleLock", json!({"ids": [k.g], "locked": true})); assert_eq!(handle_at(s.doc().unwrap(), &[k.a], Point::new(60.0, 60.0), 1.0), None); } + +#[test] +fn shift_drag_moves_the_selection() { + let mut s = Session::new(); + let k = scene(&mut s); + let at = |s: &Session, i: NodeId| s.doc().unwrap().world_bounds(i).unwrap().origin(); + let home = (Point::new(500.0, 0.0), Point::new(600.0, 0.0)); + // (selection before, Shift-drag to (from r at 520,20), selection after, where r and empty end up, derivation) + let rows: Vec<(Vec, (f64, f64), Vec, (Point, Point), &str)> = vec![ + ( + vec![k.r], + (560.0, 22.0), + vec![k.r], + (Point::new(540.0, 0.0), Point::new(600.0, 0.0)), + "Shift-drag on the selected layer moves it along one axis and keeps it selected", + ), + ( + vec![k.r, k.empty], + (560.0, 22.0), + vec![k.r, k.empty], + (Point::new(540.0, 0.0), Point::new(640.0, 0.0)), + "Shift-drag on one of several selected layers moves them all", + ), + ( + vec![k.empty], + (560.0, 22.0), + vec![k.r, k.empty], + (Point::new(540.0, 0.0), Point::new(640.0, 0.0)), + "Shift-drag on an unselected layer adds it and moves the selection", + ), + (vec![k.r, k.empty], (521.0, 21.0), vec![k.empty], home, "a Shift-press that wobbles under the drag threshold is a click: the layer leaves"), + ]; + for (before, to, after, end, why) in rows { + ex(&mut s, "select.set", json!({"ids": before})); + drag(&mut s, (520.0, 20.0), to, SHIFT); + assert_eq!(sorted(s.selection()), sorted(after), "{why}"); + assert_eq!((at(&s, k.r), at(&s, k.empty)), end, "{why}"); + if end != home { + assert!(s.undo().unwrap(), "{why}: one undo step"); + assert_eq!((at(&s, k.r), at(&s, k.empty)), home, "{why}: undone"); + } + } + // Shift let go before the button: still a Shift-click. + ex(&mut s, "select.set", json!({"ids": [k.r, k.empty]})); + s.pointer(&ev(PointerKind::Down, 520.0, 20.0, SHIFT)).unwrap(); + s.pointer(&ev(PointerKind::Up, 520.0, 20.0, NONE)).unwrap(); + assert_eq!(s.selection(), vec![k.empty], "Shift let go before the button still takes the layer out"); +} + +#[test] +fn alt_shift_drag_on_a_selected_layer_duplicates_it() { + let mut s = Session::new(); + let k = scene(&mut s); + let layers = |s: &Session| s.doc().unwrap().node(s.page().unwrap()).unwrap().children.len(); + let n = layers(&s); + let alt_shift = |kind, x, y| { + let mut e = ev(kind, x, y, SHIFT); + e.alt = true; + e + }; + ex(&mut s, "select.set", json!({"ids": [k.r]})); + s.pointer(&alt_shift(PointerKind::Down, 520.0, 20.0)).unwrap(); + s.pointer(&alt_shift(PointerKind::Up, 520.0, 20.0)).unwrap(); + assert_eq!(s.selection(), Vec::::new(), "an Alt+Shift-click is a Shift-click: the layer leaves the selection"); + assert_eq!(layers(&s), n, "and nothing is copied"); + ex(&mut s, "select.set", json!({"ids": [k.r]})); + s.pointer(&alt_shift(PointerKind::Down, 520.0, 20.0)).unwrap(); + s.pointer(&alt_shift(PointerKind::Move, 560.0, 22.0)).unwrap(); + s.pointer(&alt_shift(PointerKind::Up, 560.0, 22.0)).unwrap(); + let sel = s.selection(); + assert_eq!(layers(&s), n + 1, "an Alt+Shift-drag copies the layer"); + assert!(sel.len() == 1 && sel[0] != k.r, "the copy is selected"); + let doc = s.doc().unwrap(); + assert_eq!(doc.world_bounds(sel[0]).unwrap().origin(), Point::new(540.0, 0.0), "the copy moves along one axis"); + assert_eq!(doc.world_bounds(k.r).unwrap().origin(), Point::new(500.0, 0.0), "the original stays"); +} + +#[test] +fn shift_click_takes_a_layer_out_where_nothing_can_move() { + let mut s = Session::new(); + let k = scene(&mut s); + // A version being viewed is read-only: the press can't start a move. + let v = ex(&mut s, "file.saveVersion", json!({"name": "v"})); + ex(&mut s, "version.preview", json!({"id": v["id"]})); + ex(&mut s, "select.set", json!({"ids": [k.r, k.empty]})); + assert!(s.pointer(&ev(PointerKind::Down, 520.0, 20.0, SHIFT)).is_err(), "a version being viewed can't be edited"); + s.pointer(&ev(PointerKind::Up, 520.0, 20.0, SHIFT)).unwrap(); + assert_eq!(s.selection(), vec![k.empty], "Shift-click still takes the layer out"); +} diff --git a/crates/engine/src/tools.rs b/crates/engine/src/tools.rs index 3d3b9acb..d074052f 100644 --- a/crates/engine/src/tools.rs +++ b/crates/engine/src/tools.rs @@ -344,6 +344,9 @@ pub(crate) enum Gesture { targets: Targets, duplicate: bool, moved: bool, + /// A Shift-press on this selected layer: it leaves the selection on release, unless the + /// press became a drag. + deselect: Option, }, Resize { start: Point, @@ -1194,15 +1197,15 @@ impl Session { Some(target) => { // Clicking inside the current selection's box keeps it (drag them all). let in_sel_box = sel.len() > 1 && doc.bounds_of(&sel).is_some_and(|b| b.contains(ev.p())) && !ev.shift; + // Shift on a selected layer takes it out on release if the press doesn't become a + // drag; a drag moves the selection with it. Where no drag can start, it goes now. + let deselect = (ev.shift && sel.contains(&target)).then_some(target); + let without_target = || sel.iter().copied().filter(|x| *x != target).collect::>(); let new_sel = if in_sel_box && (sel.contains(&target) || sel.iter().any(|s| doc.is_ancestor(*s, target))) { sel.clone() - } else if ev.shift { + } else if ev.shift && deselect.is_none() { let mut s2 = sel.clone(); - if let Some(pos) = s2.iter().position(|x| *x == target) { - s2.remove(pos); - } else { - s2.push(target); - } + s2.push(target); s2 } else if sel.contains(&target) { sel.clone() @@ -1210,12 +1213,12 @@ impl Session { vec![target] }; self.set_selection(new_sel.clone()); - if new_sel.is_empty() || doc.node(target).is_some_and(|n| n.locked) { - return Ok(()); - } // Locked layers selected in the layers panel stay put. let ids: Vec = roots(&doc, &new_sel).into_iter().filter(|i| !locked_in(&doc, *i)).collect(); - if ids.is_empty() { + if new_sel.is_empty() || doc.node(target).is_some_and(|n| n.locked) || ids.is_empty() { + if deselect.is_some() { + self.set_selection(without_target()); + } return Ok(()); } let mut targets = Targets::gather(&doc, page, &ids); @@ -1225,8 +1228,13 @@ impl Session { { targets.add_box(b); } - self.begin_interaction(if ev.alt { "Duplicate" } else { "Move" })?; - self.tool.gesture = Gesture::Move { start: ev.p(), ids, targets, duplicate: ev.alt, moved: false }; + if let Err(e) = self.begin_interaction(if ev.alt { "Duplicate" } else { "Move" }) { + if deselect.is_some() { + self.set_selection(without_target()); + } + return Err(e); + } + self.tool.gesture = Gesture::Move { start: ev.p(), ids, targets, duplicate: ev.alt, moved: false, deselect }; self.tool.overlay.cursor = if ev.alt { Cursor::Copy } else { Cursor::Default }; } None => { @@ -1735,7 +1743,7 @@ impl Session { self.tool.overlay.drop_cell = None; let drop_target = self.tool.overlay.drop_target.take(); match g { - Gesture::Move { ids, moved, duplicate, start, .. } => { + Gesture::Move { ids, moved, duplicate, start, deselect, .. } => { if moved { // A free move lands in the frame under the pointer (or the page) where it was // dropped; layers in an auto layout frame already sit in their slot. @@ -1777,6 +1785,10 @@ impl Session { } } else { self.cancel_interaction()?; + if let Some(d) = deselect { + let sel = self.selection().into_iter().filter(|x| *x != d).collect(); + self.set_selection(sel); + } if !(ev.shift || ev.cmd || duplicate) { self.widget_click(start, ev.tol() * 2.0)?; } diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 62001715..a8dfab69 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -48,7 +48,7 @@ interaction feel not yet compared. | Item | Status | Notes | |---|---|---| -| Click, Shift-click, marquee, Shift-marquee toggle | done | a marquee selects what it touches; from a top-level frame's background, the layers inside it; locked and hidden layers are skipped (`selection_behaviour_tests`) | +| Click, Shift-click, marquee, Shift-marquee toggle | done | a marquee selects what it touches; from a top-level frame's background, the layers inside it; locked and hidden layers are skipped; Shift-click takes a selected layer out on release, so a Shift-drag on it moves the selection along one axis instead (`selection_behaviour_tests`) | | Deep select (Cmd/Ctrl-click, Cmd/Ctrl-marquee) | done | | | Select layer menu (every layer under the click) | done | | | Enter / Shift+Enter (children / parent), Tab / Shift+Tab (siblings) | done | Tab renames the next layer while renaming |