#2817: Closed falls back to the layer's paths when the selection names none

Selecting a non-path property (Position) or a stale selection disabled Closed, unlike
RotoBezier and the other Mask and Shape Path commands, which fall back to every mask of
the selected layers. A selected mask property (Feather, Opacity, Expansion) now selects
its mask. A selected locked mask still counts as a match, so it never falls back to its
unlocked siblings.

crates/engine/src/cmd/paths/closed.rs

@@ -1,7 +1,7 @@
 //! The selection-aware Closed menu action. Explicit edits retain `mask.setClosed`'s API.
 
 use effectcraft_keyframe::Value as KV;
-use effectcraft_project::{GroupKind, Layer, LayerId, PropGroup, Uid};
+use effectcraft_project::{GroupKind, Layer, LayerId, Node, PropGroup, Uid};
 use effectcraft_time::Tick;
 use serde_json::{Value, json};
 
@@ -14,6 +14,7 @@ struct Target {
     layer: LayerId,
     route: Vec<usize>,
     closed: bool,
+    editable: bool,
 }
 
 struct Selection {
@@ -30,58 +31,66 @@ fn group_at_mut<'a>(mut group: &'a mut PropGroup, route: &[usize]) -> Option<&'a
     Some(group)
 }
 
+/// The selected paths on the selected layers, else every path of those layers (as the other
+/// Mask and Shape Path commands do). Locked layers and masks count as matches, so selecting one
+/// never falls back to its siblings, and are then left out.
 fn targets(s: &Session) -> Result<Vec<Target>> {
     let comp = s.active_comp().ok_or(EngineError::NoComp)?;
+    let on_selected = |lid: &LayerId| s.state.selected_layers.contains(lid);
     // Layer navigation and insertion can retain point/property selections on other layers.
     let selection = Selection {
-        vertices: s.state.selected_vertices.iter().filter(|v| s.state.selected_layers.contains(&v.layer)).copied().collect(),
-        props: s.state.selected_props.iter().filter(|(lid, _)| s.state.selected_layers.contains(lid)).copied().collect(),
+        vertices: s.state.selected_vertices.iter().filter(|v| on_selected(&v.layer)).copied().collect(),
+        props: s.state.selected_props.iter().filter(|(lid, _)| on_selected(lid)).copied().collect(),
         time: s.time(),
     };
-    let whole_layers = selection.vertices.is_empty() && selection.props.is_empty();
     let mut targets = Vec::new();
-    for layer in comp.layers.iter() {
-        let selected_layer = if whole_layers {
-            s.state.selected_layers.contains(&layer.id)
-        } else {
-            selection.vertices.iter().any(|v| v.layer == layer.id) || selection.props.iter().any(|(lid, _)| *lid == layer.id)
-        };
-        if !selected_layer || layer.switches.locked {
-            continue;
+    for whole_layers in [false, true] {
+        for layer in comp.layers.iter().filter(|l| on_selected(&l.id)) {
+            collect(&selection, layer, &layer.props, &mut Vec::new(), whole_layers, layer.switches.locked, &mut targets)?;
+        }
+        if !targets.is_empty() {
+            break;
         }
-        collect(&selection, layer, &layer.props, &mut Vec::new(), whole_layers, &mut targets)?;
     }
+    targets.retain(|target| target.editable);
     if targets.is_empty() {
         return Err(bad("path.toggleClosed", "select an unlocked mask or Bezier shape path"));
     }
     Ok(targets)
 }
 
-fn collect(selection: &Selection, layer: &Layer, group: &PropGroup, route: &mut Vec<usize>, inherited: bool, targets: &mut Vec<Target>) -> Result<()> {
+fn collect(
+    selection: &Selection,
+    layer: &Layer,
+    group: &PropGroup,
+    route: &mut Vec<usize>,
+    inherited: bool,
+    locked: bool,
+    targets: &mut Vec<Target>,
+) -> Result<()> {
     if route.len() > MAX_DEPTH {
         return Err(bad("path.toggleClosed", "path group nesting exceeds 128 levels"));
     }
-    if matches!(group.kind, GroupKind::Mask { locked: true, .. }) {
-        return Ok(());
-    }
+    let locked = locked || matches!(group.kind, GroupKind::Mask { locked: true, .. });
     let selected = inherited || selection.props.contains(&(layer.id, group.uid));
     if super::super::mask::is_path_group(group)
         && let Some(prop) = group.get("path")
         && let KV::Path(path) = prop.value_at(layer.layer_time(selection.time))
     {
-        // Property selection also selects its points; both describe the selected paths.
+        // Selecting any of a path group's properties (Mask Path, Feather, Opacity…) or its points
+        // selects the path.
         let selected = selected
-            || selection.props.contains(&(layer.id, prop.uid))
+            || group.children.iter().any(|c| matches!(c, Node::Prop(p) if selection.props.contains(&(layer.id, p.uid))))
             || selection.vertices.iter().any(|v| v.layer == layer.id && v.mask == group.uid && v.index < path.vertices.len());
         if selected {
-            targets.push(Target { layer: layer.id, route: route.clone(), closed: path.closed });
+            targets.push(Target { layer: layer.id, route: route.clone(), closed: path.closed, editable: !locked });
         }
         return Ok(());
     }
     for (index, child) in group.children.iter().enumerate() {
         let Some(child) = child.as_group() else { continue };
         route.push(index);
-        collect(selection, layer, child, route, selected, targets)?;
+        collect(selection, layer, child, route, selected, locked, targets)?;
         route.pop();
     }
     Ok(())

crates/engine/src/tests/viewer.rs

@@ -434,7 +434,10 @@ fn toggle_closed_ignores_selections_retained_on_other_layers() {
     s.execute("layer.selectPrevious", json!({})).unwrap();
     assert_eq!(s.state.selected_layers, vec![LayerId(b_layer)]);
     s.execute("prop.select", json!({"layer": b_layer, "path": "transform/position"})).unwrap();
-    assert!(!s.is_enabled("path.toggleClosed"), "a non-path property does not select sibling paths");
+    assert_eq!(crate::menus::checked(&s, "path.toggleClosed", &json!({})), Some(true), "a non-path property falls back to the layer's paths");
+    s.execute("path.toggleClosed", json!({})).unwrap();
+    assert!(!shape_path(&s, b_layer, b_path).closed);
+    s.execute("edit.undo", json!({})).unwrap();
     s.execute("layer.selectNext", json!({})).unwrap();
     s.execute("path.toggleClosed", json!({})).unwrap();
     assert!(shape_path(&s, a_layer, a_path).closed, "a property retained on another layer cannot block this layer");
@@ -442,20 +445,22 @@ fn toggle_closed_ignores_selections_retained_on_other_layers() {
 }
 
 #[test]
-fn toggle_closed_rejects_locked_stale_and_nonpath_selection() {
+fn toggle_closed_falls_back_from_stale_selection_and_rejects_locked_or_pathless_layers() {
     let mut s = comp();
     let r = s.execute("shape.newPath", json!({"vertices": [[0, 0], [100, 0], [100, 100]], "closed": true})).unwrap();
     let (lid, uid) = (r["layer"].as_u64().unwrap(), r["path"].as_u64().unwrap());
     let before = s.project.clone();
     s.state.selected_vertices.clear();
     s.state.selected_props = vec![(LayerId(lid), u64::MAX)];
-    assert!(!s.is_enabled("path.toggleClosed"));
-    assert!(s.execute("path.toggleClosed", json!({})).is_err());
+    s.execute("path.toggleClosed", json!({})).unwrap();
+    assert!(!shape_path(&s, lid, uid).closed, "a stale property selection falls back to the layer's paths");
+    s.execute("edit.undo", json!({})).unwrap();
     assert!(s.project.same_content(&before));
     s.state.selected_props.clear();
     s.state.selected_vertices = vec![crate::VertexRef { layer: LayerId(lid), mask: uid, index: usize::MAX }];
-    assert!(!s.is_enabled("path.toggleClosed"));
-    assert!(s.execute("path.toggleClosed", json!({})).is_err());
+    s.execute("path.toggleClosed", json!({})).unwrap();
+    assert!(!shape_path(&s, lid, uid).closed, "a stale vertex selection falls back to the layer's paths");
+    s.execute("edit.undo", json!({})).unwrap();
     s.state.selected_vertices.clear();
     let cid = s.active_comp_id().unwrap();
     std::sync::Arc::make_mut(&mut s.project).comp_mut(cid).unwrap().layer_mut(LayerId(lid)).unwrap().switches.locked = true;
@@ -475,10 +480,16 @@ fn toggle_closed_handles_masks_and_preserves_explicit_set_closed() {
     let uid = s.execute("mask.new", json!({"layer": lid, "vertices": [[0, 0], [100, 0], [100, 100]], "closed": true})).unwrap()["mask"].as_u64().unwrap();
     let path = |s: &Session| layer(s, lid).props.find_group(uid).unwrap().get("path").unwrap().value.as_path().unwrap().clone();
     let original = path(&s);
-    s.state.selected_props = vec![(LayerId(lid), uid)];
+    let other = s.execute("mask.new", json!({"layer": lid, "vertices": [[10, 10], [90, 10], [90, 90]], "closed": true})).unwrap()["mask"].as_u64().unwrap();
+    let other_closed = |s: &Session| layer(s, lid).props.find_group(other).unwrap().get("path").unwrap().value.as_path().unwrap().closed;
+    let feather = layer(&s, lid).props.find_group(uid).unwrap().get("feather").unwrap().uid;
+    s.state.selected_vertices.clear();
+    s.state.selected_props = vec![(LayerId(lid), feather)];
     s.execute("path.toggleClosed", json!({})).unwrap();
-    assert!(!path(&s).closed);
+    assert!(!path(&s).closed, "selecting Mask Feather selects its mask");
+    assert!(other_closed(&s), "an unselected mask stays closed");
     assert!(layer(&s, lid).props.find_group(uid).unwrap().get("path").unwrap().keys.is_empty(), "a static path gains no keyframe");
+    s.state.selected_props = vec![(LayerId(lid), uid)];
     s.execute("mask.setClosed", json!({"layer": lid, "mask": uid})).unwrap();
     assert_eq!(path(&s), original, "explicit API still defaults to closed");
     s.execute("mask.setClosed", json!({"layer": lid, "mask": uid, "closed": false})).unwrap();
@@ -493,6 +504,7 @@ fn toggle_closed_handles_masks_and_preserves_explicit_set_closed() {
     assert!(!s.is_enabled("path.toggleClosed"));
     assert!(s.execute("path.toggleClosed", json!({})).is_err());
     assert_eq!(path(&s), original);
+    assert!(other_closed(&s), "a selected locked mask does not fall back to its unlocked siblings");
 }
 
 #[test]