Merge pull request #3907 from el-j/fix/direct-selection-fill-occlusion

M2.1: prevent click-through on open path fills and support segment toggle (#3405)

crates/engine/src/cmd/select.rs

@@ -76,7 +76,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Select Segments",
             [],
             None,
-            "{id, segments: [[subpath, segment]…]} select path segments; their endpoints move with them, Delete removes only their connections",
+            "{id, segments: [[subpath, segment]…], mode?: \"set\"|\"add\"|\"toggle\"} select path segments; their endpoints move with them, Delete removes only their connections",
             has_doc,
             segments
         ),
@@ -436,22 +436,85 @@ pub(crate) fn checked_refs(v: Option<&Value>, cmd: &str) -> Result<Vec<(usize, u
         .collect()
 }
 
+fn segment_endpoints(doc: &Document, id: NodeId, refs: &BTreeSet<(usize, usize)>, cmd: &str) -> Result<BTreeSet<(usize, usize)>> {
+    let path = doc.node(id).and_then(|n| n.path_data()).ok_or_else(|| bad(cmd, "object is not a path"))?;
+    let mut endpoints = BTreeSet::new();
+    for &(si, seg) in refs {
+        let sp = path.subpaths.get(si).filter(|sp| seg < sp.segment_count()).ok_or_else(|| bad(cmd, "segment does not exist"))?;
+        let n = sp.anchors.len().max(1);
+        endpoints.extend([(si, seg), (si, (seg + 1) % n)]);
+    }
+    Ok(endpoints)
+}
+
 fn segments(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "select.segments";
     let id = checked_id(s, p.get("id").ok_or_else(|| bad(C, "missing id"))?, C)?;
     let refs: BTreeSet<_> = checked_refs(p.get("segments"), C)?.into_iter().collect();
-    let path = s.doc()?.doc.node(id).and_then(|n| n.path_data()).ok_or_else(|| bad(C, "object is not a path"))?;
-    let mut endpoints = BTreeSet::new();
-    for &(si, seg) in &refs {
-        let sp = path.subpaths.get(si).filter(|sp| seg < sp.segment_count()).ok_or_else(|| bad(C, "segment does not exist"))?;
-        endpoints.extend([(si, seg), (si, (seg + 1) % sp.anchors.len())]);
-    }
-    s.select(|_, sel| {
-        sel.clear();
-        if !refs.is_empty() {
-            sel.add(id);
-            sel.anchors.insert(id, endpoints);
-            sel.segments.insert(id, refs);
+    let mode = match str_param(p, "mode") {
+        None | Some("set") => "set",
+        Some("add") => "add",
+        Some("toggle") => "toggle",
+        Some(m) => return Err(bad(C, format!("unknown mode {m:?}"))),
+    };
+    let endpoints = segment_endpoints(&s.doc()?.doc, id, &refs, C)?;
+    s.select(|d, sel| {
+        if mode == "set" {
+            sel.clear();
+            if !refs.is_empty() {
+                sel.add(id);
+                sel.anchors.insert(id, endpoints);
+                sel.segments.insert(id, refs);
+            }
+        } else if mode == "add" {
+            if !refs.is_empty() {
+                sel.add(id);
+                sel.anchors.entry(id).or_default().extend(endpoints);
+                sel.segments.entry(id).or_default().extend(refs);
+            }
+        } else if mode == "toggle" {
+            let seg_entry = sel.segments.entry(id).or_default();
+            let mut removed_endpoints = BTreeSet::new();
+            let mut added_endpoints = BTreeSet::new();
+            let path = d.node(id).and_then(|n| n.path_data());
+            for &(si, seg) in &refs {
+                let Some(sp) = path.as_ref().and_then(|p| p.subpaths.get(si)) else { continue };
+                let n = sp.anchors.len().max(1);
+                let end1 = (si, seg);
+                let end2 = (si, (seg + 1) % n);
+                if seg_entry.contains(&(si, seg)) {
+                    seg_entry.remove(&(si, seg));
+                    removed_endpoints.insert(end1);
+                    removed_endpoints.insert(end2);
+                } else {
+                    seg_entry.insert((si, seg));
+                    added_endpoints.insert(end1);
+                    added_endpoints.insert(end2);
+                }
+            }
+            let mut surviving_seg_endpoints = BTreeSet::new();
+            for &(si, seg) in seg_entry.iter() {
+                if let Some(sp) = path.as_ref().and_then(|p| p.subpaths.get(si)) {
+                    let n = sp.anchors.len().max(1);
+                    surviving_seg_endpoints.insert((si, seg));
+                    surviving_seg_endpoints.insert((si, (seg + 1) % n));
+                }
+            }
+            let anchor_entry = sel.anchors.entry(id).or_default();
+            for pt in removed_endpoints {
+                if !surviving_seg_endpoints.contains(&pt) {
+                    anchor_entry.remove(&pt);
+                }
+            }
+            anchor_entry.extend(added_endpoints);
+            if seg_entry.is_empty() {
+                sel.segments.remove(&id);
+            }
+            if anchor_entry.is_empty() {
+                sel.remove(id);
+            } else {
+                sel.add(id);
+            }
         }
     })?;
     ok()
@@ -471,8 +534,9 @@ fn anchors(s: &mut Session, p: &Value) -> Result<Value> {
         if mode == "set" {
             sel.clear();
         }
-        // A path selected as a whole has every anchor selected: toggle and add start from them all.
-        let whole = sel.contains(id) && sel.partial(id).is_none();
+        // A path selected as a whole has every anchor selected: toggle and add start from them all,
+        // but only when no partial anchor selection is active and only on a single isolated object.
+        let whole = sel.contains(id) && sel.partial(id).is_none() && sel.anchors.is_empty() && sel.objects.len() <= 1;
         sel.add(id);
         // A segment pick's endpoints aren't selected anchors: start from none of them.
         if sel.segments.remove(&id).is_some() {
@@ -489,6 +553,9 @@ fn anchors(s: &mut Session, p: &Value) -> Result<Value> {
                 e.insert(r);
             }
         }
+        if e.is_empty() {
+            sel.remove(id);
+        }
     })?;
     ok()
 }

crates/engine/tests/selection_commands.rs

@@ -133,3 +133,32 @@ fn select_key_and_document_inspect_report_the_key_object() {
     assert_eq!(s.execute("select.key", &json!({"id": b.0})).unwrap(), json!({"key": null}));
     assert_eq!(inspected(&mut s), Some(Value::Null));
 }
+
+#[test]
+fn select_segments_mode_toggle_and_add() {
+    let (mut s, a, _) = session();
+    // Select segment 0 with mode set
+    s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 0]], "mode": "set"})).unwrap();
+    let sel = &s.active().unwrap().selection;
+    assert_eq!(sel.segments.get(&a), Some(&[(0, 0)].into()));
+    assert_eq!(sel.anchors.get(&a), Some(&[(0, 0), (0, 1)].into()));
+
+    // Toggle segment 1 on (add)
+    s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 1]], "mode": "toggle"})).unwrap();
+    let sel = &s.active().unwrap().selection;
+    assert_eq!(sel.segments.get(&a), Some(&[(0, 0), (0, 1)].into()));
+    assert_eq!(sel.anchors.get(&a), Some(&[(0, 0), (0, 1), (0, 2)].into()));
+
+    // Toggle segment 0 off
+    s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 0]], "mode": "toggle"})).unwrap();
+    let sel = &s.active().unwrap().selection;
+    assert_eq!(sel.segments.get(&a), Some(&[(0, 1)].into()));
+    assert_eq!(sel.anchors.get(&a), Some(&[(0, 1), (0, 2)].into()));
+
+    // Toggle segment 1 off -> clears selection for this path
+    s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 1]], "mode": "toggle"})).unwrap();
+    let sel = &s.active().unwrap().selection;
+    assert!(!sel.contains(a));
+    assert!(sel.segments.is_empty());
+    assert!(sel.anchors.is_empty());
+}

crates/tools/src/direct.rs

@@ -300,11 +300,51 @@ fn anchor_owners(cx: &ToolContext, f: fn(&Node) -> bool) -> Vec<NodeId> {
 /// The anchor within `tol` of `p` nearest to it, of the topmost selected path or area type frame
 /// that has one, else of the topmost visible one: (id, si, ai, where it is). Selected objects come
 /// first so an object above can't take the press from the one being edited (#1508), as
-/// [`hovered_anchor`] marks it.
+/// [`hovered_anchor`] marks it. In normal viewmode, an opaque fill in front occludes unselected
+/// objects behind it (#3405).
 fn hit_anchor(cx: &ToolContext, p: Point, tol: f64) -> Option<(NodeId, usize, usize, Point)> {
     let mut owners = anchor_owners(cx, |n| matches!(n.kind, NodeKind::Path { .. }) || is_area_type(n));
     owners.sort_by_key(|id| !cx.selection.contains(*id));
-    owners.into_iter().find_map(|id| {
+
+    // 1. Check selected objects first. Selected objects are NEVER occluded (#1508).
+    let mut unselected_idx = owners.len();
+    for (i, &id) in owners.iter().enumerate() {
+        if !cx.selection.contains(id) {
+            unselected_idx = i;
+            break;
+        }
+        if let Some(pd) = cx.doc.node(id).and_then(editable_path)
+            && let Some((si, ai, a)) = nearest_anchor(&pd, p, tol)
+        {
+            return Some((id, si, ai, a));
+        }
+    }
+
+    // 2. For unselected objects: in normal viewmode, an opaque fill in front occludes
+    // unselected objects behind it (#3405).
+    let unselected = &owners[unselected_idx..];
+    if unselected.is_empty() {
+        return None;
+    }
+
+    let filter: Option<Box<dyn Fn(NodeId) -> bool>> = if !cx.outline
+        && let Some(h) = hit_test(cx.doc, p, cx.selection_hit_options())
+        && matches!(h.kind, HitKind::Fill)
+    {
+        let leaf = h.leaf;
+        let ancestry = h.ancestry.clone();
+        let part = cx.doc.node(leaf).and_then(|n| compound_part_at(n, p, tol));
+        Some(Box::new(move |id| id == leaf || ancestry.contains(&id) || part == Some(id)))
+    } else {
+        None
+    };
+
+    unselected.iter().copied().find_map(|id| {
+        if let Some(f) = &filter
+            && !f(id)
+        {
+            return None;
+        }
         let pd = cx.doc.node(id).and_then(editable_path)?;
         nearest_anchor(&pd, p, tol).map(|(si, ai, a)| (id, si, ai, a))
     })
@@ -579,27 +619,36 @@ impl Tool for DirectSelectionTool {
                     self.state = State::Idle;
                     return out;
                 }
-                if let Some(h) = hit_test(cx.doc, p, cx.selection_hit_options()) {
+                if let Some(mut h) = hit_test(cx.doc, p, cx.selection_hit_options()) {
+                    if let Some(part) = cx.doc.node(h.leaf).and_then(|n| compound_part_at(n, p, tol)) {
+                        h.ancestry.push(part);
+                        h.leaf = part;
+                    }
                     // A segment pick keeps its endpoints available for movement without selecting
                     // the anchors themselves. Dragging reshapes a curve, else moves it. Alt picks
                     // the whole path (and Alt-drag copies it), as with Group Selection.
                     let whole = cx.selection.contains(h.leaf) && cx.selection.partial(h.leaf).is_none();
-                    if !ev.mods.shift
-                        && !ev.mods.alt
+                    if !ev.mods.alt
                         && matches!(h.kind, HitKind::Stroke | HitKind::Outline)
                         && let Some(s) = segment_at(cx, h.leaf, p)
                     {
-                        if whole {
+                        if whole && !ev.mods.shift {
                             self.click_segment = Some((h.leaf, s.si, s.seg));
                         } else {
                             let selected = cx.selection.segments.get(&h.leaf).is_some_and(|sel| sel.contains(&(s.si, s.seg)));
-                            self.state = if s.curved {
+                            self.state = if s.curved && !ev.mods.shift {
                                 State::Segment { id: h.leaf, si: s.si, seg: s.seg, t: s.t, start: p, began: false }
                             } else {
                                 // Held by the segment itself, so the segment is what lands on a
                                 // ruler guide or lines up, not the pressed spot beside it (#3701).
                                 State::MoveAnchors { start: p, grab: hold_point(cx, &h, p), began: false }
                             };
+                            if ev.mods.shift {
+                                return vec![Action::Exec(
+                                    "select.segments".into(),
+                                    json!({"id": h.leaf.0, "segments": [[s.si, s.seg]], "mode": "toggle"}),
+                                )];
+                            }
                             if selected {
                                 return vec![];
                             }
@@ -962,8 +1011,9 @@ impl DirectSelectionTool {
 /// The anchor within a press's reach of `p` ([`ToolContext::point_tol`]), of a selected path or the
 /// path under `p`: what Highlight anchors on mouse over marks (a press there picks it).
 fn hovered_anchor(cx: &ToolContext, p: Point) -> Option<(NodeId, Point)> {
-    let under = hit_test(cx.doc, p, cx.selection_hit_options()).map(|h| h.leaf);
     let tol = cx.point_tol();
+    let under =
+        hit_test(cx.doc, p, cx.selection_hit_options()).map(|h| cx.doc.node(h.leaf).and_then(|n| compound_part_at(n, p, tol)).unwrap_or(h.leaf));
     cx.selection.objects.iter().copied().filter(|id| in_scope(cx.doc, cx.isolation, *id)).chain(under).find_map(|id| {
         let path = cx.doc.node(id).and_then(editable_path)?;
         nearest_anchor(&path, p, tol).map(|(.., a)| (id, a))
@@ -2113,4 +2163,138 @@ mod tests {
         let acts_group_text = t.pointer(&c2, &PointerEvent::new(PointerKind::DoubleClick, in_group_b.center().x, in_group_b.center().y));
         assert_eq!(acts_group_text, vec![Action::SwitchTool("type".into())]);
     }
+
+    #[test]
+    fn direct_selection_click_and_drag_member_of_compound_path() {
+        let mut d = vectorcraft_doc::Document::new(500.0, 500.0);
+        let l = d.layers[0].id;
+        let (outer, hole, compound) = (d.alloc_id(), d.alloc_id(), d.alloc_id());
+        let rect = |id, a: f64, b: f64| {
+            std::sync::Arc::new(vectorcraft_doc::Node::path(id, vectorcraft_geom::shapes::rectangle(Rect::new(a, a, b, b)), Default::default()))
+        };
+        let mut c = vectorcraft_doc::Node::new(
+            compound,
+            vectorcraft_doc::NodeKind::Compound {
+                children: vec![rect(outer, 0.0, 200.0), rect(hole, 50.0, 150.0)],
+                rule: vectorcraft_geom::FillRule::EvenOdd,
+            },
+        );
+        c.appearance = vectorcraft_doc::Appearance::default_art().into();
+        d.insert(Some(l), 0, c).unwrap();
+        let p = paint();
+        let none = Selection::default();
+        let mut t = DirectSelectionTool::new(false);
+        let ctx = cx(&d, &none, &p);
+        let down = t.pointer(&ctx, &PointerEvent::new(PointerKind::Down, 25.0, 100.0));
+        assert_eq!(
+            down,
+            vec![Action::Exec("select.set".into(), json!({"ids": [outer.0]}))],
+            "Direct Selection selects child member, not compound parent"
+        );
+        let mut s = Selection::default();
+        s.set([outer]);
+        let ctx_sel = cx(&d, &s, &p);
+        let drag = t.pointer(&ctx_sel, &PointerEvent::new(PointerKind::Drag, 35.0, 110.0));
+        assert!(drag.iter().any(|a| matches!(a, Action::Preview(cmd, _) if cmd == "object.transform")), "drags the selected child member: {drag:?}");
+        let up = t.pointer(&ctx_sel, &PointerEvent::new(PointerKind::Up, 35.0, 110.0));
+        assert_eq!(up, vec![Action::Commit]);
+    }
+
+    #[test]
+    fn shift_clicking_anchor_after_segment_pick_toggles_or_adds() {
+        let (d, id) = doc_with_rect();
+        let p = paint();
+        let mut s = Selection::default();
+        s.set([id]);
+        s.segments.insert(id, [(0, 0)].into());
+        s.anchors.insert(id, [(0, 0), (0, 1)].into());
+        let cx = cx(&d, &s, &p);
+        let mut t = DirectSelectionTool::new(false);
+        let shift = Mods { shift: true, ..Default::default() };
+        // Shift-click on anchor (0, 0) (top-left at 100, 100) toggles it off
+        let a = t.pointer(&cx, &PointerEvent::new(PointerKind::Down, 100.0, 100.0).with_mods(shift));
+        assert_eq!(a, vec![Action::Exec("select.anchors".into(), json!({"id": id.0, "anchors": [[0, 0]], "mode": "toggle"}))]);
+        // Shift-click on anchor (0, 2) (bottom-right at 200, 200) toggles it on (adds it)
+        let a2 = t.pointer(&cx, &PointerEvent::new(PointerKind::Down, 200.0, 200.0).with_mods(shift));
+        assert_eq!(a2, vec![Action::Exec("select.anchors".into(), json!({"id": id.0, "anchors": [[0, 2]], "mode": "toggle"}))]);
+    }
+
+    #[test]
+    fn shift_clicking_segment_toggles_segment() {
+        let (d, id) = doc_with_rect();
+        let p = paint();
+        let s = Selection::default();
+        let cx = cx(&d, &s, &p);
+        let mut t = DirectSelectionTool::new(false);
+        let shift = Mods { shift: true, ..Default::default() };
+        // The rectangle is at 100..200. Segment 0 is the top edge (y=100, x between 100 and 200).
+        // Shift-click at (150, 100) selects segment 0 with mode "toggle".
+        let a = t.pointer(&cx, &PointerEvent::new(PointerKind::Down, 150.0, 100.0).with_mods(shift));
+        assert_eq!(a, vec![Action::Exec("select.segments".into(), json!({"id": id.0, "segments": [[0, 0]], "mode": "toggle"}))]);
+    }
+
+    /// In normal viewmode, clicking the fill of an open path in front selects the open path's shape
+    /// and anchors, and NEVER clicks through to select or grab a path behind it (even if the path
+    /// behind has an anchor directly under the click).
+    #[test]
+    fn clicking_fill_of_open_path_in_front_never_selects_path_behind() {
+        use vectorcraft_geom::{Anchor, SubPath};
+        let (mut d, _behind) = doc_with_rect();
+        // `behind` has an anchor at (100.0, 100.0).
+        // Create an open path in front (id `front`), with a fill, covering (100.0, 100.0).
+        let front = d.alloc_id();
+        let l = d.layers[0].id;
+        // Triangle from (50, 50) to (150, 50) to (150, 150), open (implicit closing edge from (150, 150) to (50, 50)).
+        // (100.0, 100.0) is inside this filled triangle, but `front` has NO anchor at (100.0, 100.0).
+        let anchors = vec![Anchor::corner(Point::new(50.0, 50.0)), Anchor::corner(Point::new(150.0, 50.0)), Anchor::corner(Point::new(150.0, 150.0))];
+        let open_path = PathData::single(SubPath::new(anchors, false));
+        let ap = vectorcraft_doc::Appearance::default_art(); // has solid white fill
+        d.insert(Some(l), 1, vectorcraft_doc::Node::path(front, open_path, ap)).unwrap();
+
+        let (s, p) = (Selection::default(), paint());
+        let c = cx(&d, &s, &p);
+
+        // Hovering over (100.0, 100.0) does NOT hover the hidden anchor of `behind`.
+        assert_eq!(hovered_anchor(&c, Point::new(100.0, 100.0)), None);
+
+        // Clicking at (100.0, 100.0) selects `front` (the open path in front), NOT `behind`.
+        let mut t = DirectSelectionTool::new(false);
+        let acts = t.pointer(&c, &PointerEvent::new(PointerKind::Down, 100.0, 100.0));
+        assert_eq!(acts, vec![Action::Exec("select.set".into(), json!({"ids": [front.0]}))]);
+    }
+
+    /// A selected path's Bézier direction handle positioned over a background filled rectangle
+    /// can be hit and dragged without being occluded by the background fill.
+    #[test]
+    fn selected_path_handle_over_background_rect_is_draggable() {
+        use vectorcraft_geom::{Anchor, SubPath};
+        let mut d = vectorcraft_doc::Document::new(500.0, 500.0);
+        let l = d.layers[0].id;
+        // Background rectangle covering 0..300 with default white fill
+        let bg = d.alloc_id();
+        let bg_rect = vectorcraft_geom::shapes::rectangle(Rect::new(0.0, 0.0, 300.0, 300.0));
+        d.insert(Some(l), 0, vectorcraft_doc::Node::path(bg, bg_rect, vectorcraft_doc::Appearance::default_art())).unwrap();
+
+        // Selected path with a Bézier curve whose out-handle is at (150, 150)
+        let fg = d.alloc_id();
+        let mut a0 = Anchor::corner(Point::new(100.0, 150.0));
+        a0.h_out = Point::new(150.0, 150.0); // handle over the background rect
+        let a1 = Anchor::corner(Point::new(200.0, 150.0));
+        let curve = PathData::single(SubPath::new(vec![a0, a1], false));
+        d.insert(Some(l), 1, vectorcraft_doc::Node::path(fg, curve, vectorcraft_doc::Appearance::default_art())).unwrap();
+
+        let mut s = Selection::default();
+        s.set([fg]);
+        s.anchors.insert(fg, [(0, 0)].into());
+        let p = paint();
+        let c = cx(&d, &s, &p);
+
+        // hit_handle on the handle at (150, 150) must find the handle of `fg`
+        assert_eq!(hit_handle(&c, Point::new(150.0, 150.0), c.point_tol()), Some((fg, 0, 0, true)));
+
+        // Direct Selection tool clicking on the handle initiates Reshape
+        let mut t = DirectSelectionTool::new(false);
+        let acts = t.pointer(&c, &PointerEvent::new(PointerKind::Down, 150.0, 150.0));
+        assert_eq!(acts, vec![Action::Begin("Reshape".into())]);
+    }
 }