Viewer: a click selects the topmost layer over a selected one (#3389) (#3405)

* #3389: a viewer click selects the topmost layer over a selected one

Clicks in the viewer went through pick(), which prefers a selected layer
under the pointer so a covered layer can be dragged (#230). A large
selected layer lower in the stack therefore captured every click inside
its bounds, and the layers in front of it could not be selected there
(#1535, #3389).

Add click_pick(), the same hit test without that preference: the
topmost layer, keeping the selection when it is already selected. The
left click (Selection and fallback tools, Roto Brush), the double-click
and the right-click menu use it; drag starts keep pick(), so dragging a
covered selected layer works as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* #3389: rustfmt the viewer's shapes imports

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* #3389: test the right-click where both layers cover the pointer

The right-click check ran after the drag had moved the layer behind out
from under the pointer, so the old pick() passed it too. It now runs
before the drag with the layer behind selected, then closes the menu.
pick()'s doc names the Puppet tool's pin placement as a click that uses it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* #3389: a click on a covered path of the selected layer keeps that layer

With clicks going to the topmost layer, a click on a mask or shape path
of the selected layer where a layer in front covers it selected the
layer in front. Its paths went away, so a double-click there could no
longer free-transform the path. A plain click on a shown path now stays
on its layer, as pick() did; Shift-click still toggles the topmost one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* #3389: a right-click on a covered path of the selected layer keeps it

The context menu picked as a left click did before the path fix, so a
right-click on the selected layer's path, under a layer in front,
selected the layer in front. It now keeps the path's layer like a left
click. pick() and click_pick() docs describe the Puppet and path
exceptions exactly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

crates/ui-egui/src/panels/viewer.rs

@@ -838,9 +838,12 @@ fn phase_menu(app: &mut EffectcraftApp, ui: &mut egui::Ui, ectx: &EvalCtx, v: &m
     // A right-click an overlay took (the Cryptomatte eyedropper's) opens the menu too.
     let forwarded = ui.data_mut(|d| d.remove_temp::<egui::Pos2>(egui::Id::unique(FORWARDED_SECONDARY)));
     if let Some(pos) = resp.interact_pointer_pos().filter(|_| resp.secondary_clicked()).or(forwarded) {
-        // Picking an already selected layer preserves the group; an unselected hit
-        // becomes the target, using the same visible/unlocked hit test as left-click.
-        pick(app, ectx, map.to_comp(pos), false);
+        // The layer a left click there would select: the topmost one, keeping the group when it
+        // is already selected (#1535), or the selected layer whose shown path is under the
+        // pointer. The menu's list reaches the layers under it.
+        if ov::segment_at(&v.paths, &map, pos, 6.0).is_none() {
+            click_pick(app, ectx, map.to_comp(pos), false);
+        }
         let point = map.to_comp(pos);
         let hits: Vec<(u64, String)> = layers_at(ectx, point).map(|l| (l.id.0, l.name.clone())).collect();
         ui.data_mut(|d| d.insert_temp(menu_hits_id, hits));
@@ -1845,15 +1848,19 @@ fn phase_clicked(app: &mut EffectcraftApp, ectx: &EvalCtx, resp: &egui::Response
             }
             // The Roto Brush paints in the Layer panel only: select as usual and say so.
             rt if rt.roto_kind(false).is_some() => {
-                if pick(app, ectx, cpt, mods.shift).is_none() {
+                if click_pick(app, ectx, cpt, mods.shift).is_none() {
                     let _ = app.session.execute("edit.deselectAll", json!({}));
                 }
                 app.ui.status = crate::i18n::tr(ROTO_HINT).to_string();
             }
             _ => {
-                if pick(app, ectx, cpt, mods.shift).is_none() {
+                let on_path = ov::segment_at(&paths, &map, pos, 6.0).is_some();
+                if on_path && !mods.shift {
+                    // A click on a path of a selected layer stays on that layer, even where a
+                    // layer in front covers it, so a double-click there free-transforms the path.
+                } else if click_pick(app, ectx, cpt, mods.shift).is_none() {
                     let _ = app.session.execute("edit.deselectAll", json!({}));
-                } else if !mods.shift && !app.session.state.selected_vertices.is_empty() && ov::segment_at(&paths, &map, pos, 6.0).is_none() {
+                } else if !mods.shift && !app.session.state.selected_vertices.is_empty() && !on_path {
                     // A click away from the paths, even on their layer, deselects the mask
                     // points (the layer stays selected), as in After Effects (#513).
                     let _ = app.session.execute("mask.selectVertices", json!({"vertices": []}));
@@ -1904,7 +1911,8 @@ fn phase_dblclick(app: &mut EffectcraftApp, ectx: &EvalCtx, resp: &egui::Respons
     {
         let cpt = map.to_comp(pos);
         let roto = app.ui.tool.roto_kind(false).is_some();
-        if let Some(l) = pick(app, ectx, cpt, false).and_then(|l| comp.layer(l)) {
+        // The topmost layer, as the double-click's first click selected it (#1535).
+        if let Some(l) = click_pick(app, ectx, cpt, false).and_then(|l| comp.layer(l)) {
             match &l.source {
                 effectcraft_engine::project::LayerSource::Comp { item } => app.session.open_comp(*item),
                 // With a Roto Brush tool, double-clicking a layer opens it in the Layer panel,
@@ -2002,8 +2010,8 @@ use geometry::{
 use geometry::{box_corners, project_clipped};
 pub(crate) use geometry::{from_effect_space, fx2c, l2c, layer_outline, layer_quad};
 use gesture::{Gesture, HANDLE, ROTO_HINT, pen_id, roi_drag, roi_handles, vertex_grab_radius};
+pub(crate) use shapes::{click_pick, layer_at, layer_at_including_locked, layers_at, pick, selectable_layers};
 use shapes::{create_shape, draw_shape_ghost, shape_drag};
-pub(crate) use shapes::{layer_at, layer_at_including_locked, layers_at, pick, selectable_layers};
 #[cfg(test)]
 use tools::constrain_45;
 use tools::{add_motion_key, axis_at, axis_drag, drag_axes, draw_rigs, parent_world_inverse, pen_cursor, pen_cursor_icon, pen_press, select_motion_key};

crates/ui-egui/src/panels/viewer/shapes.rs

@@ -39,10 +39,24 @@ fn quad_area(q: &[[f64; 2]; 4]) -> f64 {
     (0..4).map(|i| q[i][0] * q[(i + 1) % 4][1] - q[(i + 1) % 4][0] * q[i][1]).sum::<f64>().abs() / 2.0
 }
 
-/// The layer a press at a comp point acts on, selected (Shift toggles the topmost layer). As in
+/// The layer a drag from a comp point acts on, selected (Shift toggles the topmost layer). As in
 /// After Effects, a selected layer under the point comes before the layers in front of it, so it
-/// can be dragged where they cover it; otherwise the topmost layer (#230).
+/// can be dragged where they cover it; otherwise the topmost layer (#230). Clicks use
+/// [`click_pick`], except a Puppet tool click while no layer has pins, which uses this so a pin
+/// can go on a selected layer behind others.
 pub(crate) fn pick(app: &mut EffectcraftApp, ectx: &EvalCtx, cpt: [f64; 2], toggle: bool) -> Option<LayerId> {
+    pick_layer(app, ectx, cpt, toggle, true)
+}
+
+/// The layer a click (press and release without a drag) at a comp point acts on: always the
+/// topmost layer, selected unless it already is (Shift toggles it). Only a drag goes to a selected
+/// layer behind others ([`pick`]), so a click can still select a layer in front of it (#1535).
+/// The viewer doesn't call it for a click on a shown path, which stays on the path's layer.
+pub(crate) fn click_pick(app: &mut EffectcraftApp, ectx: &EvalCtx, cpt: [f64; 2], toggle: bool) -> Option<LayerId> {
+    pick_layer(app, ectx, cpt, toggle, false)
+}
+
+fn pick_layer(app: &mut EffectcraftApp, ectx: &EvalCtx, cpt: [f64; 2], toggle: bool, prefer_selected: bool) -> Option<LayerId> {
     if toggle {
         let hit = layer_at(ectx, cpt)?.id;
         let _ = app.session.execute("layer.select", json!({"layers": [hit.0], "toggle": true}));
@@ -50,7 +64,8 @@ pub(crate) fn pick(app: &mut EffectcraftApp, ectx: &EvalCtx, cpt: [f64; 2], togg
     }
     let selected = &app.session.state.selected_layers;
     let hits: Vec<LayerId> = layers_at(ectx, cpt).map(|l| l.id).collect();
-    let hit = *hits.iter().find(|id| selected.contains(id)).or(hits.first())?;
+    let preferred = if prefer_selected { hits.iter().find(|id| selected.contains(id)) } else { None };
+    let hit = *preferred.or(hits.first())?;
     if !selected.contains(&hit) {
         let _ = app.session.execute("layer.select", json!({"layers": [hit.0]}));
     }

crates/ui-egui/tests/it/ui_viewer.rs

@@ -198,11 +198,10 @@ fn viewer_click_picks_nothing_while_the_solo_layer_is_inactive() {
     assert!(h.state().session.state.selected_layers.is_empty(), "{back:?} stays excluded by solo");
 }
 
-/// A selected layer under the pointer takes the press before the layers in front of it, as in
-/// After Effects: it stays selected and a drag moves it. With none selected there, the topmost
-/// layer does (#230).
+/// A click, left or right, selects the topmost layer under the pointer, even over a selected
+/// layer behind it (#1535); a drag still moves a selected layer behind others (#230).
 #[test]
-fn viewer_press_prefers_a_selected_layer_under_the_pointer() {
+fn viewer_click_selects_the_topmost_layer_and_a_drag_moves_the_selected_one() {
     let mut h = selection_harness();
     let comp = h.state().session.active_comp().unwrap();
     let (front, back) = (comp.layers[0].id, comp.layers[1].id);
@@ -210,7 +209,20 @@ fn viewer_press_prefers_a_selected_layer_under_the_pointer() {
     h.run_steps(3);
     let at = screen(&h, [320.0, 180.0]);
     click(&mut h, at);
-    assert_eq!(h.state().session.state.selected_layers, vec![back], "a click keeps the selected layer behind");
+    assert_eq!(h.state().session.state.selected_layers, vec![front], "a click selects the layer in front of the selected one (#1535)");
+    // A right-click selects what a left click would, so the menu acts on the layer in front,
+    // here where both layers cover the pointer and the layer behind is selected.
+    h.state_mut().session.execute("layer.select", json!({"layers": [back.0]})).unwrap();
+    h.run_steps(3);
+    for pressed in [true, false] {
+        h.input_mut().events.push(Event::PointerButton { pos: at, button: egui::PointerButton::Secondary, pressed, modifiers: Default::default() });
+        h.step();
+    }
+    h.run_steps(2);
+    assert_eq!(h.state().session.state.selected_layers, vec![front], "a right-click selects the layer in front");
+    h.key_press(egui::Key::Escape);
+    h.run_steps(3);
+    h.state_mut().session.execute("layer.select", json!({"layers": [back.0]})).unwrap();
     h.run_steps(40);
     let (front0, back0) = (position_and_anchor(&h, front).0, position_and_anchor(&h, back).0);
     drag(&mut h, at, at + vec2(60.0, 0.0));
@@ -224,6 +236,71 @@ fn viewer_press_prefers_a_selected_layer_under_the_pointer() {
     assert_eq!(h.state().session.state.selected_layers, vec![front], "nothing selected: the topmost layer");
 }
 
+/// A click, left or right, on a path of the selected layer, where a layer in front covers it, stays
+/// on that path's layer rather than selecting the layer in front, so a double-click there
+/// free-transforms the path (#1535 keeps this from `pick`).
+#[test]
+fn viewer_double_click_on_a_covered_path_of_the_selected_layer_free_transforms_it() {
+    let mut h = selection_harness();
+    let comp = h.state().session.active_comp().unwrap();
+    let back = comp.layers[1].id;
+    let s = &mut h.state_mut().session;
+    s.execute("layer.select", json!({"layers": [back.0]})).unwrap();
+    // In Back's own space (80×80): its top edge runs through comp (320, 150), under Front.
+    s.execute("mask.new", json!({"layer": back.0, "vertices": [[10, 10], [70, 10], [70, 70], [10, 70]], "closed": true})).unwrap();
+    h.run_steps(40);
+    let at = screen(&h, [320.0, 150.0]);
+    h.input_mut().events.push(Event::PointerMoved(at));
+    h.step();
+    // A right-click there acts on the path's layer too, as a left click does.
+    for pressed in [true, false] {
+        h.input_mut().events.push(Event::PointerButton { pos: at, button: egui::PointerButton::Secondary, pressed, modifiers: Default::default() });
+        h.step();
+    }
+    h.run_steps(2);
+    assert_eq!(h.state().session.state.selected_layers, vec![back], "a right-click on the path keeps its layer");
+    h.key_press(egui::Key::Escape);
+    h.run_steps(40);
+    // Two clicks a frame apart: the first selects as a click does, the second double-clicks.
+    for _ in 0..2 {
+        for pressed in [true, false] {
+            h.input_mut().events.push(Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed, modifiers: Default::default() });
+        }
+        h.step();
+    }
+    h.run_steps(3);
+    assert_eq!(h.state().session.state.selected_layers, vec![back], "the path's layer stays selected");
+    assert_eq!(h.state().session.state.selected_vertices.len(), 4, "all the path's points are selected");
+    assert!(h.state().auto.find("viewer.freeTransform.handle.0").is_some(), "the free-transform box is up");
+}
+
+/// A double-click where a precomp layer covers the selected layer opens the precomp: like a single
+/// click, it acts on the topmost layer, which its first click selects (#1535).
+#[test]
+fn viewer_double_click_opens_the_topmost_layer_over_a_selected_one() {
+    let mut s = Session::default();
+    let inner = s.execute("comp.new", json!({"name": "Inner", "width": 80, "height": 80, "duration": 4})).unwrap()["comp"].as_u64().unwrap();
+    s.execute("comp.new", json!({"name": "Main", "width": 640, "height": 360, "duration": 4})).unwrap();
+    s.execute("layer.newSolid", json!({"name": "Back", "color": "#406080", "width": 200, "height": 200})).unwrap();
+    s.execute("layer.addItem", json!({"item": "Inner"})).unwrap();
+    let back = s.active_comp().unwrap().layers[1].id;
+    s.execute("layer.select", json!({"layers": [back.0]})).unwrap();
+    let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(move |_| EffectcraftApp::new(s));
+    h.run_steps(40);
+    let at = screen(&h, [320.0, 180.0]);
+    h.input_mut().events.push(Event::PointerMoved(at));
+    h.step();
+    // Two clicks a frame apart: the first selects as a click does, the second double-clicks.
+    for _ in 0..2 {
+        for pressed in [true, false] {
+            h.input_mut().events.push(Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed, modifiers: Default::default() });
+        }
+        h.step();
+    }
+    h.run_steps(3);
+    assert_eq!(h.state().session.active_comp_id().map(|c| c.0), Some(inner), "the precomp in front opens");
+}
+
 /// Each composition keeps its Resolution, saved with the project as in After Effects: chosen for
 /// comp A in the viewer's popup, it is still A's after showing comp B, costs no undo step, keeps
 /// A's cached frames and is read back from the saved file (#417).