From 6bcb952830f6623417bf7caa762c0a74413543e9 Mon Sep 17 00:00:00 2001 From: brentyi Date: Fri, 26 Jun 2026 04:03:56 +0000 Subject: [PATCH 1/8] Support live modifier switching mid-drag Previously a scene-node drag froze its (button, modifier) combo at drag-start: changing the held modifier mid-gesture had no effect. This partitions a physical drag into one logical segment per combo. When the held modifier changes, the client ends the current segment and -- if the new combo is bound -- starts a fresh one under it, preserving the grab geometry (plane, grab point, instance) so the drag continues without a visual jump; only the addressed callback set changes. Modifier changes are picked up from both pointermove and (for stationary switches) window keydown/keyup during the drag. A switch to an unbound combo drops the gesture into a dormant gap (no messages) so callbacks see properly paired start/end per bound combo; re-entering a bound combo starts a new segment. The wire message already carries `modifier`, so this is client-side segmentation plus docs -- no message-schema change. `SceneNodeDragEvent` keeps a single `modifier` field that now identifies the active segment. Tests: planModifierTransition unit cases (vitest), a server-side multi-segment dispatch/bookkeeping test, and an e2e test that adds Shift mid-drag and asserts the cmd/ctrl segment ends before release. --- src/viser/_scene_handles.py | 52 +++++--- src/viser/client/src/DragLayer.tsx | 115 ++++++++++++++--- src/viser/client/src/dragUtils.test.ts | 53 ++++++++ src/viser/client/src/dragUtils.ts | 55 ++++++++ tests/e2e/test_scene_node_drag.py | 70 ++++++++++ tests/test_drag_modifier_segments.py | 172 +++++++++++++++++++++++++ 6 files changed, 485 insertions(+), 32 deletions(-) create mode 100644 tests/test_drag_modifier_segments.py diff --git a/src/viser/_scene_handles.py b/src/viser/_scene_handles.py index 792a1b845..e7c561cd5 100644 --- a/src/viser/_scene_handles.py +++ b/src/viser/_scene_handles.py @@ -475,9 +475,16 @@ class SceneNodeDragEvent(Generic[TSceneNodeHandle]): """Scene node that is being dragged.""" phase: DragPhase """Drag lifecycle phase: ``"start"`` at press, ``"update"`` on - every throttled pointermove (~20Hz), ``"end"`` at release. A - single drag fires exactly one ``"start"``, zero or more - ``"update"``s, and exactly one ``"end"``.""" + every throttled pointermove (~20Hz), ``"end"`` at release. + + A gesture is partitioned into one *segment* per held modifier-combo. + Each segment fires exactly one ``"start"``, zero or more + ``"update"``s, and exactly one ``"end"``. If the user changes the + held modifier mid-drag, the current segment ends and a new one + starts under the new modifier (see :attr:`modifier`) -- so a single + physical drag can produce more than one ``start``/``end`` pair. When + the modifier doesn't change, this collapses to the common case of a + single ``start`` ... ``end`` per gesture.""" instance_index: int | None """Instance index within a batched scene node (e.g. batched meshes, batched GLBs, batched axes); ``None`` for non-batched nodes. Frozen @@ -498,9 +505,12 @@ class SceneNodeDragEvent(Generic[TSceneNodeHandle]): button: Literal["left", "middle", "right"] """Mouse button that initiated the drag.""" modifier: _messages.KeyModifier | None - """Modifier-combo held at drag-start (frozen for the drag's - lifetime). ``None`` if no modifiers were held; otherwise a - canonical :data:`KeyModifier` string.""" + """Modifier-combo that owns the current drag segment. Constant within + a segment and matches the binding this callback was registered for; + if the user changes the held modifier mid-drag the segment ends and a + new one begins under the new combo (see :attr:`phase`). ``None`` if no + modifiers are held; otherwise a canonical :data:`KeyModifier` + string.""" _VALID_DRAG_BUTTONS: Tuple[_messages.DragButton, ...] = get_args(_messages.DragButton) @@ -611,12 +621,21 @@ def on_drag( ) -> Any: """Attach a callback for the full drag lifecycle. - Fires three times per gesture: once with - ``event.phase == "start"`` at press, zero or more times with - ``"update"`` (throttled pointermove), once with ``"end"`` at - release. ``end`` fires even on cancellation paths (window - blur, pointer cancel, node removed mid-drag) so per-drag - state can be released. + Fires once with ``event.phase == "start"`` at press, zero or + more times with ``"update"`` (throttled pointermove), and once + with ``"end"`` at release. ``end`` fires even on cancellation + paths (window blur, pointer cancel, node removed mid-drag) so + per-drag state can be released. + + Modifiers are live: if the user changes the held modifier + mid-drag, the current segment ends and a new one begins under + the new combo, routing to whichever callback that combo is bound + to. A callback therefore sees a clean ``start`` ... ``end`` pair + for *its* modifier each time that modifier is engaged, and a + single physical drag may fire more than one such pair. To switch + behavior mid-drag (e.g. changing the drag plane), register a + separate ``on_drag`` for each modifier combo. ``event.modifier`` + identifies the active segment. Usable as a bare decorator (``@handle.on_drag``, defaults to ``button="left"`` and no modifiers) or with arguments @@ -629,9 +648,12 @@ def on_drag( ordered ``"+"``-separated string like ``"cmd/ctrl"``, ``"shift"``, or ``"cmd/ctrl+shift"``. ``None`` matches "no modifiers held". Matching is exact: listed modifiers - must be held and others must not be. Left-drag on this - node intercepts the gesture -- the camera only orbits on - empty-space drags. + must be held and others must not be. The match is + re-evaluated whenever the held modifier changes mid-drag, + so this callback is entered and exited as its combo is + engaged and released. Left-drag on this node intercepts + the gesture -- the camera only orbits on empty-space + drags. Note on ordering: synchronous (``def``) callbacks are submitted to a thread pool fire-and-forget and can run out of order -- an diff --git a/src/viser/client/src/DragLayer.tsx b/src/viser/client/src/DragLayer.tsx index 747749948..c778b00c5 100644 --- a/src/viser/client/src/DragLayer.tsx +++ b/src/viser/client/src/DragLayer.tsx @@ -31,9 +31,12 @@ import { useThrottledMessageSender } from "./WebsocketUtils"; import { ActiveDragState, DragScratches, + KeyModifier, anyBindingMatches, computeInstanceWorldMatrix, isInstancedMesh2VendoredMessage, + keyModifierFromEvent, + planModifierTransition, } from "./dragUtils"; import { DragLayerApi, DragLayerContext } from "./dragLayerContext"; import { useDragArrow } from "./useDragArrow"; @@ -231,21 +234,58 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { // Build + send a drag message in one call, no-op if ``buildDragMessage`` // returns null (live grab point unavailable). Wraps the duplicated - // null-check pattern at the three send sites. */ + // null-check pattern at the send sites. Returns whether a message was + // actually sent -- callers use this to keep ``segmentActive`` honest + // (a ``start`` that couldn't build must not be paired with a later + // ``end``). */ const sendDragMessage = React.useCallback( ( activeDrag: ActiveDragState, phase: "start" | "update" | "end", throttle: boolean, - ) => { + ): boolean => { const message = buildDragMessage(activeDrag, phase); - if (message === null) return; + if (message === null) return false; if (throttle) sendDragsThrottled(message); else viewerMutable.sendMessage(message); + return true; }, [buildDragMessage, sendDragsThrottled, viewerMutable], ); + // Apply a mid-drag modifier change. Ends the current segment (if any), + // switches ownership to ``nextModifier``, and starts a fresh segment + // when the new combo is bound. Geometry is untouched, so the drag + // continues without a visual jump -- only the addressed callback set + // changes. Dormant (unbound) modifiers send nothing; see + // ``planModifierTransition``. */ + const transitionDragModifier = React.useCallback( + (activeDrag: ActiveDragState, nextModifier: KeyModifier | null) => { + const plan = planModifierTransition( + activeDrag.input.modifier, + nextModifier, + activeDrag.bindings, + activeDrag.input.button, + activeDrag.segmentActive, + ); + if (plan === null) return; + if (plan.emitEnd) { + // Flush queued throttled updates so the old segment's pending + // update lands *before* its synthetic end -- preserves wire + // ordering across the segment boundary. + flushDragsThrottled(); + sendDragMessage(activeDrag, "end", false); + } + // Switch ownership before emitting the new ``start`` so the start + // message carries the new modifier. ``button`` is unchanged. + activeDrag.input = { ...activeDrag.input, modifier: nextModifier }; + activeDrag.segmentActive = plan.emitStart + ? sendDragMessage(activeDrag, "start", false) + : false; + }, + [flushDragsThrottled, sendDragMessage], + ); + type EndInfo = { clientX: number; clientY: number; @@ -264,15 +304,14 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { } flushDragsThrottled(); - if (sendEndMessage) { - // Modifier state is frozen at drag_start: a drag is "owned" by - // whichever (button, modifiers) combo was held when the user - // pressed the mouse, and stays owned by that combo until release - // regardless of modifier changes mid-drag. This guarantees the - // drag_start / drag_end callbacks see the same dispatch and - // avoids a class of footguns where a key-up arrives a beat - // before mouse-up (downgrading the gesture at the last moment) - // or a modifier is accidentally pressed mid-drag. + if (sendEndMessage && activeDrag.segmentActive) { + // End the currently-active segment. A drag is partitioned into + // one segment per (button, modifier) combo; the modifier can + // switch mid-drag (see ``transitionDragModifier``), and each + // switch already emitted the prior segment's ``end``. So here we + // only emit when a segment is still active -- a release while + // dormant (current modifier matches no binding) has nothing left + // to end. sendDragMessage(activeDrag, "end", false); } @@ -362,16 +401,42 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { if (event.pointerId !== activeDrag.pointerId) return; if (!updateActiveDragEnd(event.clientX, event.clientY)) return; - // Modifier/button state is frozen at drag_start and reused on - // every update/end -- see the note in `stopActiveDrag` above. + // A modifier change carried on this move ends the current + // segment and starts a new one under the new combo. Run it + // *after* refreshing the end position so the synthetic end + // reports the latest pointer location, and *before* the update + // so the update is attributed to the new segment. + const liveModifier = keyModifierFromEvent(event); + if (liveModifier !== activeDrag.input.modifier) { + transitionDragModifier(activeDrag, liveModifier); + } + // ``start_position`` is recomputed live inside buildDragMessage, // so the wire payload always reflects the click point's - // current world position (tracking the moving object). - sendDragMessage(activeDrag, "update", true); + // current world position (tracking the moving object). Skip + // while dormant -- the current modifier matches no binding. + if (activeDrag.segmentActive) { + sendDragMessage(activeDrag, "update", true); + } // The per-frame useFrame updates the arrow tail from target // transforms; no manual update needed here. }; + // Modifier changes can also arrive with the pointer stationary + // (the user taps a modifier key without moving the mouse). Listen + // for key transitions during the drag and re-evaluate ownership + // using the last-known pointer position. ``keyModifierFromEvent`` + // reads ``ctrl/meta/shift/alt`` off the KeyboardEvent, so both + // keydown and keyup resolve the current combo. + const handleWindowKeyChange = (event: KeyboardEvent) => { + const activeDrag = activeDragRef.current; + if (activeDrag === null) return; + const liveModifier = keyModifierFromEvent(event); + if (liveModifier !== activeDrag.input.modifier) { + transitionDragModifier(activeDrag, liveModifier); + } + }; + const handleWindowPointerUp = (event: PointerEvent) => { // Ignore mismatched pointers -- we only end the drag when the // *same* pointer that started it lifts up (or cancels). @@ -393,6 +458,8 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { window.removeEventListener("pointerup", handleWindowPointerUp); window.removeEventListener("pointercancel", handleWindowPointerUp); window.removeEventListener("blur", handleWindowBlur); + window.removeEventListener("keydown", handleWindowKeyChange); + window.removeEventListener("keyup", handleWindowKeyChange); }; // Plane parallel to the camera image plane, through the start @@ -424,6 +491,13 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { endPointWorld: startWorld.clone(), endPointerXy: [pointerXy[0], pointerXy[1]], input, + bindings, + // Set from the initial ``start`` send below. The pointerdown + // path only calls ``beginDrag`` when ``input`` matches a + // binding, so the opening segment is bound -- but the send can + // still fail if the live grab point is unavailable, so we trust + // its return value rather than assuming ``true``. + segmentActive: false, releaseCameraLock: null, cleanup, }; @@ -434,7 +508,13 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { window.addEventListener("pointerup", handleWindowPointerUp); window.addEventListener("pointercancel", handleWindowPointerUp); window.addEventListener("blur", handleWindowBlur); - sendDragMessage(activeDragRef.current, "start", false); + window.addEventListener("keydown", handleWindowKeyChange); + window.addEventListener("keyup", handleWindowKeyChange); + activeDragRef.current.segmentActive = sendDragMessage( + activeDragRef.current, + "start", + false, + ); return true; }, stopIfNodeIs: (nodeName) => { @@ -449,6 +529,7 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { frameScratches, sendDragMessage, stopActiveDrag, + transitionDragModifier, updateActiveDragEnd, viewer, viewerMutable, diff --git a/src/viser/client/src/dragUtils.test.ts b/src/viser/client/src/dragUtils.test.ts index 7826715e1..60a2e1fc7 100644 --- a/src/viser/client/src/dragUtils.test.ts +++ b/src/viser/client/src/dragUtils.test.ts @@ -7,6 +7,7 @@ import { anyBindingMatches, hasCmdCtrl, motionExceedsThreshold, + planModifierTransition, MOTION_THRESHOLD_PX, } from "./dragUtils"; @@ -91,6 +92,58 @@ describe("hasCmdCtrl", () => { }); }); +describe("planModifierTransition", () => { + // mjviser-style setup: two bound combos on the left button, used to + // switch the drag plane mid-gesture. + const bindings = [ + { button: "left" as const, modifier: "cmd/ctrl" as const }, + { button: "left" as const, modifier: "cmd/ctrl+shift" as const }, + ]; + + it("is a no-op when the modifier is unchanged", () => { + expect( + planModifierTransition("cmd/ctrl", "cmd/ctrl", bindings, "left", true), + ).toBeNull(); + // Even when nothing is held and nothing changes. + expect(planModifierTransition(null, null, bindings, "left", false)).toBeNull(); + }); + + it("ends the old segment and starts a new one between two bound combos", () => { + expect( + planModifierTransition("cmd/ctrl", "cmd/ctrl+shift", bindings, "left", true), + ).toEqual({ emitEnd: true, emitStart: true }); + }); + + it("ends the segment and goes dormant when the new combo is unbound", () => { + // ctrl is bound, shift-only is not -- releasing ctrl mid-drag drops + // into a dormant gap rather than starting a spurious segment. + expect( + planModifierTransition("cmd/ctrl", "shift", bindings, "left", true), + ).toEqual({ emitEnd: true, emitStart: false }); + }); + + it("starts a fresh segment when re-entering a bound combo from dormant", () => { + // Already dormant (segmentActive=false): no end to emit, just the + // new start. + expect( + planModifierTransition("shift", "cmd/ctrl", bindings, "left", false), + ).toEqual({ emitEnd: false, emitStart: true }); + }); + + it("stays dormant when moving between two unbound combos", () => { + expect( + planModifierTransition("shift", "alt", bindings, "left", false), + ).toEqual({ emitEnd: false, emitStart: false }); + }); + + it("respects the button when matching the new combo", () => { + // The same modifier on a button with no binding is unbound. + expect( + planModifierTransition(null, "cmd/ctrl", bindings, "right", false), + ).toEqual({ emitEnd: false, emitStart: false }); + }); +}); + describe("motionExceedsThreshold", () => { it("is false for movement at or under the threshold (L-infinity)", () => { expect(motionExceedsThreshold([0, 0], [0, 0])).toBe(false); diff --git a/src/viser/client/src/dragUtils.ts b/src/viser/client/src/dragUtils.ts index e00a92a1c..7b708d9b0 100644 --- a/src/viser/client/src/dragUtils.ts +++ b/src/viser/client/src/dragUtils.ts @@ -97,6 +97,48 @@ export function anyBindingMatches( return bindings.some((b) => matchesDragBinding(b, input)); } +/** Plan emitted by :func:`planModifierTransition`: which lifecycle + * messages a mid-drag modifier change should produce. */ +export type DragModifierTransition = { + /** Send a ``phase="end"`` for the *current* (pre-switch) modifier + * before switching ownership. True iff a segment is currently active. */ + emitEnd: boolean; + /** Send a ``phase="start"`` under the *new* modifier after switching. + * True iff the new combo matches a registered binding. */ + emitStart: boolean; +}; + +/** Decide how an in-progress drag reacts to a mid-gesture modifier change. + * + * A single physical drag is partitioned into one logical segment per + * (button, modifier) combo. When the held modifier changes, the current + * segment is ended and -- if the new combo matches a registered binding -- + * a fresh segment is started under it. The grab geometry (plane, grab + * point, instance) is preserved across the boundary by the caller, so the + * drag continues without a visual jump; only which callback set is + * addressed changes. + * + * If the new combo matches no binding the drag goes *dormant*: the + * physical gesture stays alive (camera locked, geometry retained) but no + * messages are sent, so the user's callbacks see properly paired + * start/end per bound combo. Re-entering a bound combo before release + * starts a fresh segment. + * + * Returns ``null`` when the modifier is unchanged (a no-op). */ +export function planModifierTransition( + current: KeyModifier | null, + next: KeyModifier | null, + bindings: DragBinding[], + button: PointerButton, + segmentActive: boolean, +): DragModifierTransition | null { + if (next === current) return null; + return { + emitEnd: segmentActive, + emitStart: anyBindingMatches(bindings, { button, modifier: next }), + }; +} + /** True when the held modifier includes cmd/ctrl. Used to gate browser * context-menu suppression: macOS raises a ``contextmenu`` event on * ctrl+click, and we only want to suppress it when the gesture is a @@ -167,7 +209,20 @@ export type ActiveDragState = { * compute ``end_screen_pos`` and re-cast the pointer ray each * pointermove. */ endPointerXy: [number, number]; + /** Current (button, modifier) the drag is owned by. ``button`` is + * frozen at drag-start; ``modifier`` is *live* -- it switches when the + * held modifier changes mid-drag, partitioning the gesture into one + * segment per combo (see :func:`planModifierTransition`). */ input: DragInput; + /** Registered drag bindings for this node, captured at drag-start. + * Used to decide whether a new modifier combo owns a segment or is + * dormant when the held modifier changes mid-drag. */ + bindings: DragBinding[]; + /** Whether an in-flight segment is currently active (a ``start`` was + * sent and its ``end`` hasn't). ``false`` while dormant -- the gesture + * is physically held but the current modifier matches no binding, so + * no messages are sent. */ + segmentActive: boolean; /** Release for the camera-control lock held for the lifetime of * this drag. Called in `stopActiveDrag` (and on every cancel * path). Routing through `cameraLock` keeps a concurrent diff --git a/tests/e2e/test_scene_node_drag.py b/tests/e2e/test_scene_node_drag.py index 2a1b68925..b00ad254a 100644 --- a/tests/e2e/test_scene_node_drag.py +++ b/tests/e2e/test_scene_node_drag.py @@ -205,6 +205,76 @@ def _(event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: assert event.modifier == "cmd/ctrl", event.modifier +def test_scene_node_drag_modifier_switch_mid_drag( + page: Page, + viser_server: viser.ViserServer, +) -> None: + """Changing the held modifier mid-drag ends the current segment and + starts a new one under the new combo, routing each segment to the + callback bound to it. Holding Ctrl, dragging, then adding Shift while + still dragging should: end the ``cmd/ctrl`` segment, then run a full + ``cmd/ctrl+shift`` segment -- all within one physical button press.""" + viser_server.initial_camera.position = (0.0, 0.0, 4.0) + viser_server.initial_camera.look_at = (0.0, 0.0, 0.0) + + ctrl_started = threading.Event() + ctrl_ended = threading.Event() + ctrl_shift_started = threading.Event() + ctrl_shift_ended = threading.Event() + + box = viser_server.scene.add_box( + "/switch_box", + dimensions=(4.0, 4.0, 0.2), + color=(200, 100, 255), + ) + + # The server routes each segment to the callback bound to its combo, + # so a fired event is itself proof the segment carried that modifier. + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl")( + lambda _: ctrl_started.set() + ) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl")(lambda _: ctrl_ended.set()) + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl+shift")( + lambda _: ctrl_shift_started.set() + ) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl+shift")( + lambda _: ctrl_shift_ended.set() + ) + + wait_for_connection(page, viser_server.get_port()) + wait_for_scene_node(page, "/switch_box") + + (start_x, start_y), (end_x, end_y) = _get_canvas_drag_points(page) + mid_x = (start_x + end_x) / 2 + mid_y = (start_y + end_y) / 2 + + # cmd/ctrl segment: press and drag halfway. + page.keyboard.down("Control") + page.mouse.move(start_x, start_y) + page.mouse.down() + page.mouse.move(mid_x, mid_y, steps=8) + assert ctrl_started.wait(timeout=5.0), "cmd/ctrl segment didn't start" + + # Add Shift mid-drag -- the keydown alone (no further motion) ends the + # cmd/ctrl segment and starts the cmd/ctrl+shift one. Asserting + # ctrl_ended *before* mouse-up is the crux: it proves the segment + # ended from the modifier switch, not from the release. + page.keyboard.down("Shift") + assert ctrl_ended.wait(timeout=5.0), "cmd/ctrl segment didn't end on modifier add" + assert ctrl_shift_started.wait(timeout=5.0), ( + "cmd/ctrl+shift segment didn't start when Shift was added mid-drag" + ) + + # Keep dragging under the new combo, then release. + page.mouse.move(end_x, end_y, steps=8) + page.mouse.up() + page.keyboard.up("Shift") + page.keyboard.up("Control") + assert ctrl_shift_ended.wait(timeout=5.0), ( + "cmd/ctrl+shift segment didn't end on release" + ) + + def test_scene_node_drag_filter_rejects_wrong_modifier( page: Page, viser_server: viser.ViserServer, diff --git a/tests/test_drag_modifier_segments.py b/tests/test_drag_modifier_segments.py new file mode 100644 index 000000000..97da7fc36 --- /dev/null +++ b/tests/test_drag_modifier_segments.py @@ -0,0 +1,172 @@ +"""Unit tests for mid-drag modifier switching (drag "segments"). + +A single physical drag is partitioned into one *segment* per held +(button, modifier) combo. When the held modifier changes mid-drag the +client ends the current segment and starts a new one under the new +combo; the server routes each segment's start/update/end to whichever +callback that combo is bound to (``_handle_node_drag`` -> +``_dispatch_drag_callbacks``). + +These tests drive ``_handle_node_drag`` directly with a crafted message +sequence -- no browser -- to lock down that the server bookkeeping +(``_active_drag_handles``) and per-modifier dispatch stay consistent +across the synthetic end/start boundary the client emits. The real +client-side segmentation (key listeners, geometry preservation) is +covered by the e2e suite and the ``planModifierTransition`` unit tests. +""" + +from __future__ import annotations + +import asyncio +from typing import Generator, cast +from unittest.mock import Mock + +import pytest + +import viser +from viser import _messages +from viser.infra import ClientId + + +@pytest.fixture +def server() -> Generator[viser.ViserServer, None, None]: + s = viser.ViserServer() + try: + yield s + finally: + s.stop() + + +def _drag_msg( + name: str, + phase: _messages._DragPhase, + modifier: _messages.KeyModifier | None, +) -> _messages.SceneNodeDragMessage: + return _messages.SceneNodeDragMessage( + phase=phase, + name=name, + instance_index=None, + start_position=(0.0, 0.0, 0.0), + start_screen_pos=(0.0, 0.0), + end_position=(1.0, 1.0, 1.0), + end_screen_pos=(1.0, 1.0), + button="left", + modifier=modifier, + ) + + +def _dispatch( + server: viser.ViserServer, + client_id: ClientId, + message: _messages.SceneNodeDragMessage, +) -> None: + """Run ``_handle_node_drag`` on the server's event loop and block.""" + asyncio.run_coroutine_threadsafe( + server.scene._handle_node_drag(client_id, message), + server._event_loop, + ).result() + + +def test_modifier_switch_routes_segments_to_each_binding( + server: viser.ViserServer, +) -> None: + """Switching the held modifier mid-drag routes each segment's + start/update/end to the callback bound to that combo, and leaves the + other combo's callback untouched.""" + box = server.scene.add_box("/seg_box", dimensions=(1.0, 1.0, 1.0)) + + ctrl_phases: list[str] = [] + ctrl_shift_phases: list[str] = [] + + # Async callbacks are awaited in dispatch order on the event loop, so + # the phase lists are complete and deterministic once ``_dispatch`` + # returns. (Sync callbacks are fire-and-forget on a thread pool and + # would race the assertions.) + @box.on_drag("left", modifier="cmd/ctrl") + async def _(event: viser.SceneNodeDragEvent) -> None: + ctrl_phases.append(event.phase) + # The active segment's modifier always matches this binding. + assert event.modifier == "cmd/ctrl" + + @box.on_drag("left", modifier="cmd/ctrl+shift") + async def _(event: viser.SceneNodeDragEvent) -> None: + ctrl_shift_phases.append(event.phase) + assert event.modifier == "cmd/ctrl+shift" + + client = cast(ClientId, 42) + server._connected_clients[client] = Mock() + + # The client emits this sequence when the user holds Ctrl, drags, + # then adds Shift mid-drag and keeps dragging before releasing: the + # Ctrl segment is ended and a Ctrl+Shift segment is started, with the + # grab geometry preserved across the boundary. + _dispatch(server, client, _drag_msg("/seg_box", "start", "cmd/ctrl")) + _dispatch(server, client, _drag_msg("/seg_box", "update", "cmd/ctrl")) + _dispatch(server, client, _drag_msg("/seg_box", "end", "cmd/ctrl")) + _dispatch(server, client, _drag_msg("/seg_box", "start", "cmd/ctrl+shift")) + _dispatch(server, client, _drag_msg("/seg_box", "update", "cmd/ctrl+shift")) + _dispatch(server, client, _drag_msg("/seg_box", "end", "cmd/ctrl+shift")) + + # Each callback sees exactly one clean start...end for its own combo, + # and nothing from the other segment. + assert ctrl_phases == ["start", "update", "end"] + assert ctrl_shift_phases == ["start", "update", "end"] + + # Bookkeeping released after the final end. + assert not server.scene._is_drag_active_for("/seg_box") + + +def test_modifier_switch_keeps_drag_active_across_boundary( + server: viser.ViserServer, +) -> None: + """The node stays "actively dragged" across the segment boundary -- + the end of one segment is immediately followed by the start of the + next, so a concurrent ``remove()`` would still preserve callbacks + until the gesture truly finishes.""" + box = server.scene.add_box("/active_box", dimensions=(1.0, 1.0, 1.0)) + box.on_drag("left", modifier="cmd/ctrl")(lambda _: None) + box.on_drag("left", modifier="cmd/ctrl+shift")(lambda _: None) + + client = cast(ClientId, 7) + server._connected_clients[client] = Mock() + + _dispatch(server, client, _drag_msg("/active_box", "start", "cmd/ctrl")) + assert server.scene._is_drag_active_for("/active_box") + + # Old segment ends... + _dispatch(server, client, _drag_msg("/active_box", "end", "cmd/ctrl")) + # ...new segment starts. The node is active again immediately. + _dispatch(server, client, _drag_msg("/active_box", "start", "cmd/ctrl+shift")) + assert server.scene._is_drag_active_for("/active_box") + + _dispatch(server, client, _drag_msg("/active_box", "end", "cmd/ctrl+shift")) + assert not server.scene._is_drag_active_for("/active_box") + + +def test_unbound_segment_does_not_dispatch( + server: viser.ViserServer, +) -> None: + """A segment whose modifier matches no binding dispatches to nobody + (server-side filter), but still cycles the active-drag bookkeeping + cleanly. In practice the client suppresses these entirely (dormant + state); this pins the server's filter as a backstop.""" + box = server.scene.add_box("/unbound_box", dimensions=(1.0, 1.0, 1.0)) + + ctrl_phases: list[str] = [] + + @box.on_drag("left", modifier="cmd/ctrl") + async def _(event: viser.SceneNodeDragEvent) -> None: + ctrl_phases.append(event.phase) + + client = cast(ClientId, 99) + server._connected_clients[client] = Mock() + + _dispatch(server, client, _drag_msg("/unbound_box", "start", "cmd/ctrl")) + _dispatch(server, client, _drag_msg("/unbound_box", "end", "cmd/ctrl")) + # Shift-only is unbound: no callback fires for this segment. + _dispatch(server, client, _drag_msg("/unbound_box", "start", "shift")) + _dispatch(server, client, _drag_msg("/unbound_box", "update", "shift")) + _dispatch(server, client, _drag_msg("/unbound_box", "end", "shift")) + + assert ctrl_phases == ["start", "end"] + assert not server.scene._is_drag_active_for("/unbound_box") From a279c1868b726e9c9e82989161fa471df0e860fa Mon Sep 17 00:00:00 2001 From: brentyi Date: Fri, 26 Jun 2026 05:10:48 +0000 Subject: [PATCH 2/8] Add e2e coverage for dormant-then-resume drag Covers the gap surfaced in review: switching to an unbound modifier combo mid-drag must suspend the gesture (dormant), not end it. Asserts the active segment ends on the switch (before release), and that re-entering a bound combo resumes with a fresh segment within the same button press -- which only holds if dormant is a suspend rather than a teardown. Both transitions are driven by key events with the pointer stationary, so this also exercises the keydown/keyup path. --- tests/e2e/test_scene_node_drag.py | 90 +++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/tests/e2e/test_scene_node_drag.py b/tests/e2e/test_scene_node_drag.py index b00ad254a..c92f08a30 100644 --- a/tests/e2e/test_scene_node_drag.py +++ b/tests/e2e/test_scene_node_drag.py @@ -275,6 +275,96 @@ def test_scene_node_drag_modifier_switch_mid_drag( ) +def test_scene_node_drag_dormant_then_resume( + page: Page, + viser_server: viser.ViserServer, +) -> None: + """Switching to an UNBOUND modifier combo mid-drag suspends the gesture + (dormant) rather than ending it: the active segment's ``end`` fires, but + the physical drag stays alive and a *new* segment starts when a bound + combo is re-entered -- all within one button press. + + Only ``cmd/ctrl`` is bound, so adding Shift lands in a dormant gap. The + crux is the resume: if pressing Shift had torn the drag down, releasing + it could NOT produce a second ``cmd/ctrl`` start without a fresh + mouse-down. Both transitions here are driven by key events with the + pointer stationary, so this also exercises the keydown/keyup path.""" + viser_server.initial_camera.position = (0.0, 0.0, 4.0) + viser_server.initial_camera.look_at = (0.0, 0.0, 0.0) + + starts = [0] + ends = [0] + first_start = threading.Event() + first_end = threading.Event() + resumed_start = threading.Event() + second_end = threading.Event() + lock = threading.Lock() + + box = viser_server.scene.add_box( + "/dormant_box", + dimensions=(4.0, 4.0, 0.2), + color=(120, 220, 160), + ) + + def _on_start(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + starts[0] += 1 + n = starts[0] + (first_start if n == 1 else resumed_start).set() + + def _on_end(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + ends[0] += 1 + n = ends[0] + (first_end if n == 1 else second_end).set() + + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl")(_on_start) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl")(_on_end) + + wait_for_connection(page, viser_server.get_port()) + wait_for_scene_node(page, "/dormant_box") + + (start_x, start_y), (end_x, end_y) = _get_canvas_drag_points(page) + mid_x = (start_x + end_x) / 2 + mid_y = (start_y + end_y) / 2 + + # cmd/ctrl segment: press and drag partway. + page.keyboard.down("Control") + page.mouse.move(start_x, start_y) + page.mouse.down() + page.mouse.move(mid_x, mid_y, steps=8) + assert first_start.wait(timeout=5.0), "cmd/ctrl segment didn't start" + + # Add Shift (cmd/ctrl+shift is unbound) -> dormant. The active segment + # ends now, BEFORE any release -- proving the switch, not the mouse-up, + # ended it. + page.keyboard.down("Shift") + assert first_end.wait(timeout=5.0), "segment didn't end when going dormant" + with lock: + assert ends[0] == 1 and starts[0] == 1, (starts[0], ends[0]) + + # Drag while dormant -- no segment is active, so nothing should fire. + page.mouse.move(end_x, end_y, steps=8) + + # Release Shift -> back to the bound cmd/ctrl combo -> RESUME with a + # fresh segment, even though the pointer is stationary and the button + # was never released. + page.keyboard.up("Shift") + assert resumed_start.wait(timeout=5.0), ( + "drag did not resume on re-entering cmd/ctrl -- dormant was treated " + "as a full teardown instead of a suspend" + ) + + page.mouse.up() + page.keyboard.up("Control") + assert second_end.wait(timeout=5.0), "resumed segment didn't end on release" + + # Exactly two clean segments, no churn from the dormant gap. + with lock: + assert starts[0] == 2, starts[0] + assert ends[0] == 2, ends[0] + + def test_scene_node_drag_filter_rejects_wrong_modifier( page: Page, viser_server: viser.ViserServer, From 78aac95e9c4f777a01600d9d7ddff8eb8d7c4e85 Mon Sep 17 00:00:00 2001 From: brentyi Date: Fri, 26 Jun 2026 06:15:31 +0000 Subject: [PATCH 3/8] Add multi-stage drag coverage (many switches + dormant cycles) Existing tests each exercised a single modifier switch. These add sequences within one drag: - server: one drag cycling cmd/ctrl -> cmd/ctrl+shift -> cmd/ctrl -> cmd/ctrl+shift (re-entering both combos), asserting each callback sees clean ordered start...end pairs and the active-drag map fully releases. - e2e: one continuous press cycling cmd/ctrl -> cmd/ctrl+shift -> dormant -> cmd/ctrl+shift -> cmd/ctrl, asserting exactly two segments per combo across repeated switching, re-entry, and resume-after-dormant. Uses async callbacks so the counters settle deterministically. --- tests/e2e/test_scene_node_drag.py | 112 +++++++++++++++++++++++++++ tests/test_drag_modifier_segments.py | 55 +++++++++++++ 2 files changed, 167 insertions(+) diff --git a/tests/e2e/test_scene_node_drag.py b/tests/e2e/test_scene_node_drag.py index c92f08a30..99c01aff9 100644 --- a/tests/e2e/test_scene_node_drag.py +++ b/tests/e2e/test_scene_node_drag.py @@ -365,6 +365,118 @@ def _on_end(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: assert ends[0] == 2, ends[0] +def test_scene_node_drag_many_stages_one_drag( + page: Page, + viser_server: viser.ViserServer, +) -> None: + """A single drag that cycles through several segments AND a dormant gap: + cmd/ctrl -> cmd/ctrl+shift -> (dormant) -> cmd/ctrl+shift -> cmd/ctrl, + all within one button press. Both combos are entered twice, with an + unbound (dormant) interval in the middle, exercising repeated switching, + re-entry, and resume-after-dormant in one gesture. Each callback must + see exactly two clean start...end pairs.""" + viser_server.initial_camera.position = (0.0, 0.0, 4.0) + viser_server.initial_camera.look_at = (0.0, 0.0, 0.0) + + ctrl_starts = [0] + ctrl_ends = [0] + cs_starts = [0] + cs_ends = [0] + lock = threading.Lock() + # One event per milestone we gate the gesture on. + ev = { + k: threading.Event() for k in ("cs_s1", "cs_e1", "cs_s2", "ctrl_s2", "ctrl_e2") + } + + box = viser_server.scene.add_box( + "/many_stage_box", + dimensions=(4.0, 4.0, 0.2), + color=(160, 140, 240), + ) + + # Async callbacks are awaited in dispatch order on the event loop, so + # the counters settle deterministically (sync callbacks fire-and-forget + # on a thread pool and would race the final count assertions -- step 4 + # below fires two callbacks at once). + async def _ctrl_start(_e: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + ctrl_starts[0] += 1 + n = ctrl_starts[0] + if n == 2: + ev["ctrl_s2"].set() + + async def _ctrl_end(_e: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + ctrl_ends[0] += 1 + n = ctrl_ends[0] + if n == 2: + ev["ctrl_e2"].set() + + async def _cs_start(_e: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + cs_starts[0] += 1 + n = cs_starts[0] + ev["cs_s1" if n == 1 else "cs_s2"].set() + + async def _cs_end(_e: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + cs_ends[0] += 1 + n = cs_ends[0] + if n == 1: + ev["cs_e1"].set() + + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl")(_ctrl_start) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl")(_ctrl_end) + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl+shift")(_cs_start) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl+shift")(_cs_end) + + wait_for_connection(page, viser_server.get_port()) + wait_for_scene_node(page, "/many_stage_box") + + (start_x, start_y), (end_x, end_y) = _get_canvas_drag_points(page) + q_x = start_x + (end_x - start_x) / 3 + q_y = start_y + (end_y - start_y) / 3 + + # Segment 1: cmd/ctrl. + page.keyboard.down("Control") + page.mouse.move(start_x, start_y) + page.mouse.down() + page.mouse.move(q_x, q_y, steps=6) + + # Segment 2: add Shift -> cmd/ctrl+shift (bound). + page.keyboard.down("Shift") + assert ev["cs_s1"].wait(timeout=5.0), "cmd/ctrl+shift didn't start on Shift add" + + # Dormant: release Ctrl -> shift-only (unbound). cmd/ctrl+shift ends. + page.keyboard.up("Control") + assert ev["cs_e1"].wait(timeout=5.0), "cmd/ctrl+shift didn't end going dormant" + + # Drag while dormant (nothing should fire). + page.mouse.move(end_x, end_y, steps=6) + + # Resume: press Ctrl -> cmd/ctrl+shift again (bound). + page.keyboard.down("Control") + assert ev["cs_s2"].wait(timeout=5.0), "cmd/ctrl+shift didn't resume after dormant" + + # Segment 4: release Shift -> back to cmd/ctrl (bound). + page.keyboard.up("Shift") + assert ev["ctrl_s2"].wait(timeout=5.0), "cmd/ctrl didn't restart on Shift release" + + # Release. + page.mouse.up() + page.keyboard.up("Control") + assert ev["ctrl_e2"].wait(timeout=5.0), ( + "final cmd/ctrl segment didn't end on release" + ) + + # Exactly two clean segments per combo across the whole gesture. + with lock: + assert ctrl_starts[0] == 2, ctrl_starts[0] + assert ctrl_ends[0] == 2, ctrl_ends[0] + assert cs_starts[0] == 2, cs_starts[0] + assert cs_ends[0] == 2, cs_ends[0] + + def test_scene_node_drag_filter_rejects_wrong_modifier( page: Page, viser_server: viser.ViserServer, diff --git a/tests/test_drag_modifier_segments.py b/tests/test_drag_modifier_segments.py index 97da7fc36..d801515d5 100644 --- a/tests/test_drag_modifier_segments.py +++ b/tests/test_drag_modifier_segments.py @@ -170,3 +170,58 @@ async def _(event: viser.SceneNodeDragEvent) -> None: assert ctrl_phases == ["start", "end"] assert not server.scene._is_drag_active_for("/unbound_box") + + +def test_many_segments_one_drag_routes_and_cycles( + server: viser.ViserServer, +) -> None: + """A single drag that switches modifiers SEVERAL times -- including + re-entering a previously-used combo -- routes each segment to its + callback as a clean, ordered start...end pair, and the active-drag + bookkeeping registers/pops correctly across every boundary. + + Dormant (unbound) gaps produce no wire messages (the client suppresses + them), so a realistic multi-stage stream is just the bound segments + back to back. We interleave cmd/ctrl and cmd/ctrl+shift four times to + exercise repeated switching and re-entry of both combos in one drag.""" + box = server.scene.add_box("/many_box", dimensions=(1.0, 1.0, 1.0)) + + ctrl_phases: list[str] = [] + ctrl_shift_phases: list[str] = [] + + @box.on_drag("left", modifier="cmd/ctrl") + async def _(event: viser.SceneNodeDragEvent) -> None: + ctrl_phases.append(event.phase) + assert event.modifier == "cmd/ctrl" + + @box.on_drag("left", modifier="cmd/ctrl+shift") + async def _(event: viser.SceneNodeDragEvent) -> None: + ctrl_shift_phases.append(event.phase) + assert event.modifier == "cmd/ctrl+shift" + + client = cast(ClientId, 13) + server._connected_clients[client] = Mock() + + # ctrl -> ctrl+shift -> ctrl -> ctrl+shift, all in one physical drag. + sequence: list[tuple[_messages._DragPhase, _messages.KeyModifier]] = [ + ("start", "cmd/ctrl"), + ("update", "cmd/ctrl"), + ("end", "cmd/ctrl"), + ("start", "cmd/ctrl+shift"), + ("update", "cmd/ctrl+shift"), + ("end", "cmd/ctrl+shift"), + ("start", "cmd/ctrl"), # re-enter ctrl + ("end", "cmd/ctrl"), + ("start", "cmd/ctrl+shift"), # re-enter ctrl+shift + ("end", "cmd/ctrl+shift"), + ] + for phase, modifier in sequence: + _dispatch(server, client, _drag_msg("/many_box", phase, modifier)) + + # Each callback sees its two segments as clean, ordered, paired phases + # -- no cross-talk, no dropped or duplicated start/end across the four + # switches. + assert ctrl_phases == ["start", "update", "end", "start", "end"] + assert ctrl_shift_phases == ["start", "update", "end", "start", "end"] + # Bookkeeping fully released after the final end. + assert not server.scene._is_drag_active_for("/many_box") From 30afe03ae46015c4758b22a4cd1cc8e00c70a7ef Mon Sep 17 00:00:00 2001 From: Brent Yi Date: Fri, 17 Jul 2026 18:15:10 -0700 Subject: [PATCH 4/8] Prettier-format dragUtils.test.ts (CI eslint gate) --- src/viser/client/src/dragUtils.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/viser/client/src/dragUtils.test.ts b/src/viser/client/src/dragUtils.test.ts index 719efb33d..785152b42 100644 --- a/src/viser/client/src/dragUtils.test.ts +++ b/src/viser/client/src/dragUtils.test.ts @@ -105,12 +105,20 @@ describe("planModifierTransition", () => { planModifierTransition("cmd/ctrl", "cmd/ctrl", bindings, "left", true), ).toBeNull(); // Even when nothing is held and nothing changes. - expect(planModifierTransition(null, null, bindings, "left", false)).toBeNull(); + expect( + planModifierTransition(null, null, bindings, "left", false), + ).toBeNull(); }); it("ends the old segment and starts a new one between two bound combos", () => { expect( - planModifierTransition("cmd/ctrl", "cmd/ctrl+shift", bindings, "left", true), + planModifierTransition( + "cmd/ctrl", + "cmd/ctrl+shift", + bindings, + "left", + true, + ), ).toEqual({ emitEnd: true, emitStart: true }); }); From ed7bcbe6a4b0fee4313d5d30454dd1a7cc883310 Mon Sep 17 00:00:00 2001 From: Brent Yi Date: Fri, 17 Jul 2026 21:06:28 -0700 Subject: [PATCH 5/8] Honor pre-promotion modifier changes; read drag bindings live at switches Two review findings on the mid-drag modifier-switch mechanism: 1. The opening segment is now attributed to the modifier held on the promoting pointermove, not the one sampled at pointerdown. The drag layer's key listeners only install at promotion, so a change inside the pointerdown->threshold window was invisible: a promote-and-release gesture opened (and closed) a segment under a stale modifier. When the promotion-time combo is unbound the drag now begins dormant and resumes on the next bound combo (new planDragStart, unit-pinned). 2. transitionDragModifier reads bindings live from the scene-tree store instead of a drag-start snapshot, so callbacks registered or removed mid-drag take effect at the next switch. The snapshot field is gone from ActiveDragState. E2E pins for both: a pre-promotion Ctrl release must not fire a stale segment (and must resume on re-press), and a binding registered mid-drag must own the post-switch segment. --- src/viser/client/src/DragLayer.tsx | 41 ++++-- src/viser/client/src/SceneTree.tsx | 6 +- src/viser/client/src/dragLayerContext.ts | 9 +- src/viser/client/src/dragUtils.test.ts | 32 +++++ src/viser/client/src/dragUtils.ts | 25 +++- src/viser/client/src/pointer/gestures.ts | 16 ++- tests/e2e/test_scene_node_drag.py | 156 +++++++++++++++++++++++ 7 files changed, 260 insertions(+), 25 deletions(-) diff --git a/src/viser/client/src/DragLayer.tsx b/src/viser/client/src/DragLayer.tsx index 68f6c5d5d..badde93b0 100644 --- a/src/viser/client/src/DragLayer.tsx +++ b/src/viser/client/src/DragLayer.tsx @@ -36,6 +36,7 @@ import { computeInstanceWorldMatrix, isInstancedMesh2VendoredMessage, keyModifierFromEvent, + planDragStart, planModifierTransition, } from "./dragUtils"; import { DragLayerApi, DragLayerContext } from "./dragLayerContext"; @@ -255,10 +256,17 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { // ``planModifierTransition``. */ const transitionDragModifier = React.useCallback( (activeDrag: ActiveDragState, nextModifier: KeyModifier | null) => { + // Bindings are read live from the scene-tree store (not a + // drag-start snapshot): a callback the server registers or removes + // *mid-drag* takes effect at the next modifier switch. A missing + // node (removed mid-gesture) reads as no bindings -- the switch + // goes dormant, and the revoke path ends the drag separately. + const liveBindings = + viewer.useSceneTree.get(activeDrag.nodeName)?.dragBindings ?? []; const plan = planModifierTransition( activeDrag.input.modifier, nextModifier, - activeDrag.bindings, + liveBindings, activeDrag.input.button, activeDrag.segmentActive, ); @@ -277,7 +285,7 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { ? sendDragMessage(activeDrag, "start", false) : false; }, - [flushDragsThrottled, sendDragMessage], + [flushDragsThrottled, sendDragMessage, viewer], ); type EndInfo = { @@ -334,10 +342,18 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { pointerId, input, bindings, + promotionModifier, }) => { if (activeDragRef.current !== null) return false; if (!anyBindingMatches(bindings, input)) return false; + // The gate above validates the POINTERDOWN input (the combo that + // made this gesture a drag candidate). The opening segment is + // attributed to the PROMOTION-TIME modifier, which may differ -- + // and may be unbound, in which case the drag begins dormant and + // the key/pointermove listeners below pick up the next switch. + const opening = planDragStart(input, promotionModifier, bindings); + // Convert the raycast hit point to world coords. The frame of // ``eventPoint`` depends on which raycast produced it: // - ``BatchedMeshesMessage`` / ``BatchedGlbMessage`` use the @@ -482,13 +498,12 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { // / end_* fields agree). endPointWorld: startWorld.clone(), endPointerXy: [pointerXy[0], pointerXy[1]], - input, - bindings, - // Set from the initial ``start`` send below. The pointerdown - // path only calls ``beginDrag`` when ``input`` matches a - // binding, so the opening segment is bound -- but the send can - // still fail if the live grab point is unavailable, so we trust - // its return value rather than assuming ``true``. + input: opening.input, + // Set from the initial ``start`` send below. The opening + // segment is dormant when the promotion-time combo is unbound + // (``opening.emitStart`` false); even when bound, the send can + // still fail if the live grab point is unavailable, so we + // trust its return value rather than assuming ``true``. segmentActive: false, releaseCameraLock: null, cleanup, @@ -502,11 +517,9 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { window.addEventListener("blur", handleWindowBlur); window.addEventListener("keydown", handleWindowKeyChange); window.addEventListener("keyup", handleWindowKeyChange); - activeDragRef.current.segmentActive = sendDragMessage( - activeDragRef.current, - "start", - false, - ); + activeDragRef.current.segmentActive = opening.emitStart + ? sendDragMessage(activeDragRef.current, "start", false) + : false; return true; }, stopIfNodeIs: (nodeName) => { diff --git a/src/viser/client/src/SceneTree.tsx b/src/viser/client/src/SceneTree.tsx index f3164e29f..536b29a75 100644 --- a/src/viser/client/src/SceneTree.tsx +++ b/src/viser/client/src/SceneTree.tsx @@ -1053,7 +1053,11 @@ export function SceneNodeThreeObject(props: { name: string }) { onPromote: beginDragArgs === null || dragLayer === null ? null - : () => dragLayer.beginDrag(beginDragArgs), + : (promotionModifier) => + dragLayer.beginDrag({ + ...beginDragArgs, + promotionModifier, + }), }); if (dragMatches) e.nativeEvent.preventDefault(); } diff --git a/src/viser/client/src/dragLayerContext.ts b/src/viser/client/src/dragLayerContext.ts index a6d821fa1..9218f0691 100644 --- a/src/viser/client/src/dragLayerContext.ts +++ b/src/viser/client/src/dragLayerContext.ts @@ -6,7 +6,7 @@ import React from "react"; import * as THREE from "three"; -import { DragBinding, DragInput } from "./dragUtils"; +import { DragBinding, DragInput, KeyModifier } from "./dragUtils"; export type BeginDragArgs = { nodeName: string; @@ -21,6 +21,13 @@ export type BeginDragArgs = { pointerId: number; input: DragInput; bindings: DragBinding[]; + /** Modifier held at promotion time (the threshold-crossing + * pointermove). May differ from ``input.modifier``, which was sampled + * at pointerdown: DragLayer's key listeners only install at promotion, + * so a change inside the pointerdown-to-promotion window is only + * visible through this value. The opening segment is attributed to + * this combo (dormant when it's unbound). */ + promotionModifier: KeyModifier | null; }; export interface DragLayerApi { diff --git a/src/viser/client/src/dragUtils.test.ts b/src/viser/client/src/dragUtils.test.ts index 785152b42..615dd5a0e 100644 --- a/src/viser/client/src/dragUtils.test.ts +++ b/src/viser/client/src/dragUtils.test.ts @@ -7,6 +7,7 @@ import { anyBindingMatches, hasCmdCtrl, motionExceedsThreshold, + planDragStart, planModifierTransition, MOTION_THRESHOLD_PX, } from "./dragUtils"; @@ -152,6 +153,37 @@ describe("planModifierTransition", () => { }); }); +describe("planDragStart", () => { + const bindings = [ + { button: "left" as const, modifier: "cmd/ctrl" as const }, + { button: "left" as const, modifier: "cmd/ctrl+shift" as const }, + ]; + const down = { button: "left" as const, modifier: "cmd/ctrl" as const }; + + it("keeps the pointerdown input when the modifier is unchanged", () => { + const plan = planDragStart(down, "cmd/ctrl", bindings); + expect(plan.input).toBe(down); // same reference -- no realloc + expect(plan.emitStart).toBe(true); + }); + + it("attributes the opening segment to the promotion-time modifier", () => { + // Shift added inside the pointerdown->promotion window: the opening + // start carries cmd/ctrl+shift, never a stale cmd/ctrl. + const plan = planDragStart(down, "cmd/ctrl+shift", bindings); + expect(plan.input).toEqual({ button: "left", modifier: "cmd/ctrl+shift" }); + expect(plan.emitStart).toBe(true); + }); + + it("begins dormant when the promotion-time combo is unbound", () => { + // Ctrl released before the threshold crossing: no segment starts (no + // degenerate stale-modifier start/end pair), but the drag itself + // begins -- a later switch back to a bound combo resumes it. + const plan = planDragStart(down, null, bindings); + expect(plan.input).toEqual({ button: "left", modifier: null }); + expect(plan.emitStart).toBe(false); + }); +}); + describe("motionExceedsThreshold", () => { it("is false for movement at or under the threshold (L-infinity)", () => { expect(motionExceedsThreshold([0, 0], [0, 0])).toBe(false); diff --git a/src/viser/client/src/dragUtils.ts b/src/viser/client/src/dragUtils.ts index 5b18b8c80..42cb213f9 100644 --- a/src/viser/client/src/dragUtils.ts +++ b/src/viser/client/src/dragUtils.ts @@ -139,6 +139,27 @@ export function planModifierTransition( }; } +/** Decide the opening segment for a drag promoted at the motion + * threshold. ``pointerdownInput`` was sampled at pointerdown, but the + * held modifier may have changed before the threshold-crossing + * pointermove (the promotion event) -- the drag layer's key listeners + * only install at promotion, so that window is otherwise invisible. The + * opening segment is attributed to the promotion-time modifier; when + * that combo is unbound the drag begins *dormant* (no ``start`` sent), + * exactly like a mid-drag switch to an unbound combo (see + * :func:`planModifierTransition`). */ +export function planDragStart( + pointerdownInput: DragInput, + promotionModifier: KeyModifier | null, + bindings: DragBinding[], +): { input: DragInput; emitStart: boolean } { + const input = + promotionModifier === pointerdownInput.modifier + ? pointerdownInput + : { ...pointerdownInput, modifier: promotionModifier }; + return { input, emitStart: anyBindingMatches(bindings, input) }; +} + /** True when the held modifier includes cmd/ctrl. Used to gate browser * context-menu suppression: macOS raises a ``contextmenu`` event on * ctrl+click, and we only want to suppress it when the gesture is a @@ -213,10 +234,6 @@ export type ActiveDragState = { * held modifier changes mid-drag, partitioning the gesture into one * segment per combo (see :func:`planModifierTransition`). */ input: DragInput; - /** Registered drag bindings for this node, captured at drag-start. - * Used to decide whether a new modifier combo owns a segment or is - * dormant when the held modifier changes mid-drag. */ - bindings: DragBinding[]; /** Whether an in-flight segment is currently active (a ``start`` was * sent and its ``end`` hasn't). ``false`` while dormant -- the gesture * is physically held but the current modifier matches no binding, so diff --git a/src/viser/client/src/pointer/gestures.ts b/src/viser/client/src/pointer/gestures.ts index 257a34ae4..4064aabb0 100644 --- a/src/viser/client/src/pointer/gestures.ts +++ b/src/viser/client/src/pointer/gestures.ts @@ -1,4 +1,5 @@ import { + keyModifierFromEvent, matchesModifierFilter, motionExceedsThreshold, pointerButtonFromNative, @@ -274,7 +275,10 @@ type NodeCandidate = { startClientXy: [number, number]; release: (() => void) | null; cleanup: () => void; - onPromote: (() => boolean) | null; + /** Called with the modifier held on the promoting pointermove -- the + * modifier may have changed since pointerdown, and the drag layer's + * own key listeners only install once the drag begins. */ + onPromote: ((promotionModifier: KeyModifier | null) => boolean) | null; }; export class NodeGestureController { @@ -300,7 +304,7 @@ export class NodeGestureController { nodeKey: string; startClientXy: [number, number]; lockCamera: boolean; - onPromote: (() => boolean) | null; + onPromote: ((promotionModifier: KeyModifier | null) => boolean) | null; }): void { this.cancelCandidate(); @@ -315,7 +319,7 @@ export class NodeGestureController { ) { return; } - this.promoteOrCancelCandidate(); + this.promoteOrCancelCandidate(keyModifierFromEvent(event)); }; const handlePointerUp = (event: PointerEvent) => { const candidate = this.candidate; @@ -384,12 +388,14 @@ export class NodeGestureController { this.cancelAny(); } - private promoteOrCancelCandidate(): void { + private promoteOrCancelCandidate( + promotionModifier: KeyModifier | null, + ): void { const candidate = this.candidate; if (candidate === null) return; const promote = candidate.onPromote; this.cancelCandidate(); - if (promote !== null) promote(); + if (promote !== null) promote(promotionModifier); } private cancelCandidate(): void { diff --git a/tests/e2e/test_scene_node_drag.py b/tests/e2e/test_scene_node_drag.py index 99c01aff9..433a7bbda 100644 --- a/tests/e2e/test_scene_node_drag.py +++ b/tests/e2e/test_scene_node_drag.py @@ -365,6 +365,162 @@ def _on_end(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: assert ends[0] == 2, ends[0] +def test_scene_node_drag_modifier_change_before_promotion( + page: Page, + viser_server: viser.ViserServer, +) -> None: + """A modifier change inside the pointerdown->promotion window is honored. + + The drag candidate samples its modifier at pointerdown, but the drag + only *promotes* at the motion threshold -- and the drag layer's key + listeners install at promotion. Releasing Ctrl in that window used to + be invisible: the promoted drag opened a stale ``cmd/ctrl`` segment + (start + end) even though no modifier was held for any of the actual + motion. + + Now the opening segment is attributed to the promotion-time modifier: + with Ctrl released before the threshold crossing, the drag begins + DORMANT -- no start fires -- and re-pressing Ctrl mid-gesture resumes + it with a properly-attributed segment (proving the gesture began as a + live drag rather than being dropped).""" + viser_server.initial_camera.position = (0.0, 0.0, 4.0) + viser_server.initial_camera.look_at = (0.0, 0.0, 0.0) + + starts = [0] + ends = [0] + started = threading.Event() + ended = threading.Event() + lock = threading.Lock() + + box = viser_server.scene.add_box( + "/prepromo_box", + dimensions=(4.0, 4.0, 0.2), + color=(255, 180, 90), + ) + + def _on_start(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + starts[0] += 1 + started.set() + + def _on_end(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + ends[0] += 1 + ended.set() + + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl")(_on_start) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl")(_on_end) + + wait_for_connection(page, viser_server.get_port()) + wait_for_scene_node(page, "/prepromo_box") + + (start_x, start_y), (end_x, end_y) = _get_canvas_drag_points(page) + + # Pointerdown under the bound combo, then release Ctrl BEFORE any + # motion -- the change lands inside the promotion blind spot. + page.keyboard.down("Control") + page.mouse.move(start_x, start_y) + page.mouse.down() + page.keyboard.up("Control") + + # Cross the threshold and keep dragging with nothing held. The drag + # must begin dormant: no stale cmd/ctrl start (the old bug fired a + # degenerate start/end pair here). + page.mouse.move(end_x, end_y, steps=12) + assert not started.wait(timeout=1.0), ( + "a cmd/ctrl segment started even though Ctrl was released before " + "the promotion threshold (stale pointerdown modifier)" + ) + + # Re-press Ctrl with the button still down: the dormant drag resumes + # with a real cmd/ctrl segment -- distinguishing "began dormant" from + # "never began at all". + page.keyboard.down("Control") + assert started.wait(timeout=5.0), ( + "re-pressing the bound modifier mid-gesture did not resume the " + "dormant drag -- was the pre-promotion gesture dropped entirely?" + ) + + page.mouse.up() + page.keyboard.up("Control") + assert ended.wait(timeout=5.0), "resumed segment didn't end on release" + with lock: + assert starts[0] == 1 and ends[0] == 1, (starts[0], ends[0]) + + +def test_scene_node_drag_binding_added_mid_drag( + page: Page, + viser_server: viser.ViserServer, +) -> None: + """A binding registered while a drag is in progress takes effect at the + next modifier switch. + + Bindings are read live at each switch (not snapshotted at drag + start): registering a ``cmd/ctrl+shift`` callback mid-``cmd/ctrl``-drag + and then pressing Shift must end the cmd/ctrl segment and START a + cmd/ctrl+shift one. With a drag-start snapshot, the switch would land + in a dormant gap and the new callback would never fire without a fresh + mouse-down.""" + viser_server.initial_camera.position = (0.0, 0.0, 4.0) + viser_server.initial_camera.look_at = (0.0, 0.0, 0.0) + + ctrl_started = threading.Event() + ctrl_ended = threading.Event() + ctrl_shift_started = threading.Event() + ctrl_shift_ended = threading.Event() + + box = viser_server.scene.add_box( + "/live_bindings_box", + dimensions=(4.0, 4.0, 0.2), + color=(90, 180, 255), + ) + + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl")( + lambda _: ctrl_started.set() + ) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl")(lambda _: ctrl_ended.set()) + + wait_for_connection(page, viser_server.get_port()) + wait_for_scene_node(page, "/live_bindings_box") + + (start_x, start_y), (end_x, end_y) = _get_canvas_drag_points(page) + mid_x = (start_x + end_x) / 2 + mid_y = (start_y + end_y) / 2 + + # cmd/ctrl segment: press and drag partway. + page.keyboard.down("Control") + page.mouse.move(start_x, start_y) + page.mouse.down() + page.mouse.move(mid_x, mid_y, steps=8) + assert ctrl_started.wait(timeout=5.0), "cmd/ctrl segment didn't start" + + # Register the cmd/ctrl+shift binding MID-DRAG, and give the update + # message a beat to reach the client. + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl+shift")( + lambda _: ctrl_shift_started.set() + ) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl+shift")( + lambda _: ctrl_shift_ended.set() + ) + page.wait_for_timeout(500) + + # Shift lands the switch on the freshly-registered combo. + page.keyboard.down("Shift") + assert ctrl_ended.wait(timeout=5.0), "cmd/ctrl segment didn't end on switch" + assert ctrl_shift_started.wait(timeout=5.0), ( + "the mid-drag-registered cmd/ctrl+shift binding didn't own the new " + "segment -- bindings were snapshotted at drag start instead of read live" + ) + + page.mouse.move(end_x, end_y, steps=8) + page.mouse.up() + page.keyboard.up("Shift") + page.keyboard.up("Control") + assert ctrl_shift_ended.wait(timeout=5.0), ( + "cmd/ctrl+shift segment didn't end on release" + ) + + def test_scene_node_drag_many_stages_one_drag( page: Page, viser_server: viser.ViserServer, From 413846c0d0f186651f614c3e9e3141112caf11d1 Mon Sep 17 00:00:00 2001 From: Brent Yi Date: Fri, 17 Jul 2026 21:13:14 -0700 Subject: [PATCH 6/8] Plan the opening drag segment against live bindings too The pointerdown->promotion window fix attributed the opening segment to the promotion-time modifier but still planned it against the bindings array captured at pointerdown. A binding added or removed inside that window was invisible to the opening plan (e.g. a combo bound mid-window began dormant instead of owning the segment). beginDrag now reads dragBindings live from the scene-tree store for the opening plan; the pointerdown snapshot remains only as the candidacy gate. --- src/viser/client/src/DragLayer.tsx | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/viser/client/src/DragLayer.tsx b/src/viser/client/src/DragLayer.tsx index badde93b0..df5e537f1 100644 --- a/src/viser/client/src/DragLayer.tsx +++ b/src/viser/client/src/DragLayer.tsx @@ -347,12 +347,18 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { if (activeDragRef.current !== null) return false; if (!anyBindingMatches(bindings, input)) return false; - // The gate above validates the POINTERDOWN input (the combo that - // made this gesture a drag candidate). The opening segment is - // attributed to the PROMOTION-TIME modifier, which may differ -- - // and may be unbound, in which case the drag begins dormant and - // the key/pointermove listeners below pick up the next switch. - const opening = planDragStart(input, promotionModifier, bindings); + // The gate above validates the POINTERDOWN input against the + // POINTERDOWN bindings (the combo that made this gesture a drag + // candidate). The opening segment is instead attributed to the + // PROMOTION-TIME modifier, planned against the LIVE bindings -- + // both may have changed inside the pointerdown->promotion window + // (a binding-clear cancels the candidate before promotion, but a + // partial edit doesn't). An unbound promotion-time combo begins + // the drag dormant; the key/pointermove listeners below pick up + // the next switch. + const liveBindings = + viewer.useSceneTree.get(nodeName)?.dragBindings ?? []; + const opening = planDragStart(input, promotionModifier, liveBindings); // Convert the raycast hit point to world coords. The frame of // ``eventPoint`` depends on which raycast produced it: From 6b63aefb39b19ce83506082f9d8da05effcab03c Mon Sep 17 00:00:00 2001 From: Brent Yi Date: Sat, 18 Jul 2026 00:51:51 -0700 Subject: [PATCH 7/8] Defer switch-created segment starts until confirmed (100ms or motion) Ending a modifier-drag the natural way -- modifier up a beat before the button -- previously fired a degenerate start/end pair on whatever combo remained bound (e.g. a bare on_drag registered alongside a modifier binding), since humans can't release both within the same frame. This reintroduced exactly the degenerate pair the promotion motion-threshold eliminated, one boundary later. A mid-drag switch now ends the old segment immediately but holds the new segment's start PENDING until confirmed: a 100ms timer (SWITCH_START_DELAY_MS), or sooner by pointer motion past the promotion threshold from the switch point. Release/cancel/blur inside the window discards the pending start outright -- no message was sent, none is owed -- and a further modifier change abandons it silently, which also swallows key-rollover pass-through combos when releasing multi-key combos one key at a time. Stationary switches still fire (timer path, imperceptibly later); the opening segment is never deferred (promotion's motion threshold is its confirmation). E2E pins the release order: bare + cmd/ctrl both bound, ctrl-drag ended modifier-first -> bare sees nothing, ctrl sees one clean pair, and a follow-up bare drag proves the binding was live. --- src/viser/_scene_handles.py | 9 +++ src/viser/client/src/DragLayer.tsx | 96 +++++++++++++++++++++++---- src/viser/client/src/dragUtils.ts | 27 +++++++- tests/e2e/test_scene_node_drag.py | 102 +++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 13 deletions(-) diff --git a/src/viser/_scene_handles.py b/src/viser/_scene_handles.py index e7c561cd5..2cc104bf7 100644 --- a/src/viser/_scene_handles.py +++ b/src/viser/_scene_handles.py @@ -637,6 +637,15 @@ def on_drag( separate ``on_drag`` for each modifier combo. ``event.modifier`` identifies the active segment. + A switch-created segment's ``start`` is confirmed briefly + (~100ms, or sooner on pointer motion) before it fires; releasing + the mouse button within that window discards the segment + entirely. In particular, releasing the modifier a beat before + the button -- the natural way to end a modifier-drag -- does + *not* fire a spurious start/end pair on the combo left behind + (e.g. a bare ``on_drag`` registered alongside a modifier + binding). + Usable as a bare decorator (``@handle.on_drag``, defaults to ``button="left"`` and no modifiers) or with arguments (``@handle.on_drag("left", modifier="cmd/ctrl")``). diff --git a/src/viser/client/src/DragLayer.tsx b/src/viser/client/src/DragLayer.tsx index df5e537f1..ece09be86 100644 --- a/src/viser/client/src/DragLayer.tsx +++ b/src/viser/client/src/DragLayer.tsx @@ -36,8 +36,10 @@ import { computeInstanceWorldMatrix, isInstancedMesh2VendoredMessage, keyModifierFromEvent, + motionExceedsThreshold, planDragStart, planModifierTransition, + SWITCH_START_DELAY_MS, } from "./dragUtils"; import { DragLayerApi, DragLayerContext } from "./dragLayerContext"; import { useDragArrow } from "./useDragArrow"; @@ -46,6 +48,15 @@ import { useDragArrow } from "./useDragArrow"; // DragLayer component. // ============================================================================= +/** Discard a not-yet-confirmed switch-created ``start`` (no-op when none + * is pending). No wire message was ever sent for the pending segment, so + * nothing needs ending -- the segment simply never existed. */ +function discardPendingStart(activeDrag: ActiveDragState): void { + if (activeDrag.pendingStart === null) return; + clearTimeout(activeDrag.pendingStart.timerId); + activeDrag.pendingStart = null; +} + /** In playback / embedded / static viewers ``sendMessage`` is a no-op, * so dragging makes no sense (it would disable camera controls and * swallow left-clicks with nothing on the receiving end). The outer @@ -248,11 +259,30 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { [buildDragMessage, sendDragsThrottled, viewerMutable], ); - // Apply a mid-drag modifier change. Ends the current segment (if any), - // switches ownership to ``nextModifier``, and starts a fresh segment - // when the new combo is bound. Geometry is untouched, so the drag - // continues without a visual jump -- only the addressed callback set - // changes. Dormant (unbound) modifiers send nothing; see + // Send a pending switch-created ``start`` now (no-op when none is + // pending). Called when the switch is confirmed: the deferral timer + // fires, or the pointer moves past the threshold from the switch + // point. */ + const flushPendingStart = React.useCallback( + (activeDrag: ActiveDragState) => { + if (activeDrag.pendingStart === null) return; + clearTimeout(activeDrag.pendingStart.timerId); + activeDrag.pendingStart = null; + activeDrag.segmentActive = sendDragMessage(activeDrag, "start", false); + }, + [sendDragMessage], + ); + + // Apply a mid-drag modifier change. Ends the current segment (if any) + // immediately, switches ownership to ``nextModifier``, and -- when the + // new combo is bound -- schedules a fresh segment whose ``start`` is + // DEFERRED until confirmed (``SWITCH_START_DELAY_MS`` timer, or pointer + // motion; see ``flushPendingStart``). Releasing the button first + // discards the pending start, so releasing the modifier a beat before + // the mouse button never fires a degenerate start/end pair on the + // combo left behind. Geometry is untouched, so the drag continues + // without a visual jump -- only the addressed callback set changes. + // Dormant (unbound) modifiers send nothing; see // ``planModifierTransition``. */ const transitionDragModifier = React.useCallback( (activeDrag: ActiveDragState, nextModifier: KeyModifier | null) => { @@ -271,6 +301,11 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { activeDrag.segmentActive, ); if (plan === null) return; + // Switching away from a combo whose start is still pending + // abandons that segment before it ever hit the wire -- a sub-delay + // pass-through combo (e.g. key rollover while releasing a + // multi-key combo) produces no messages at all. + discardPendingStart(activeDrag); if (plan.emitEnd) { // Flush queued throttled updates so the old segment's pending // update lands *before* its synthetic end -- preserves wire @@ -278,14 +313,26 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { flushDragsThrottled(); sendDragMessage(activeDrag, "end", false); } - // Switch ownership before emitting the new ``start`` so the start - // message carries the new modifier. ``button`` is unchanged. + // Switch ownership before scheduling the new ``start`` so the + // start message carries the new modifier. ``button`` is unchanged. activeDrag.input = { ...activeDrag.input, modifier: nextModifier }; - activeDrag.segmentActive = plan.emitStart - ? sendDragMessage(activeDrag, "start", false) - : false; + activeDrag.segmentActive = false; + if (plan.emitStart) { + activeDrag.pendingStart = { + switchPointerXy: [ + activeDrag.endPointerXy[0], + activeDrag.endPointerXy[1], + ], + timerId: setTimeout(() => { + // The drag may have ended since (stopActiveDrag clears the + // timer, but guard against a same-tick race anyway). + if (activeDragRef.current !== activeDrag) return; + flushPendingStart(activeDrag); + }, SWITCH_START_DELAY_MS), + }; + } }, - [flushDragsThrottled, sendDragMessage, viewer], + [flushDragsThrottled, flushPendingStart, sendDragMessage, viewer], ); type EndInfo = { @@ -298,6 +345,13 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { const activeDrag = activeDragRef.current; if (activeDrag === null) return; + // A release (or cancel/blur/node-removal) while a switch-start is + // still pending discards it: the modifier change and the release + // were one gesture. No start was sent, so no end is owed -- this + // is the deferral's whole purpose (no degenerate start/end pair + // when the modifier lifts a beat before the button). + discardPendingStart(activeDrag); + if (endInfo !== undefined) { // Refresh end fields from the final pointer position; if the // ray misses the plane (rare grazing case) we keep whatever was @@ -425,10 +479,25 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { transitionDragModifier(activeDrag, liveModifier); } + // Real motion confirms a pending switch-start early: the + // pointer leaving the switch point past the promotion + // threshold proves the new segment is a deliberate + // continuation, not the leading edge of a release. + if ( + activeDrag.pendingStart !== null && + motionExceedsThreshold( + activeDrag.pendingStart.switchPointerXy, + activeDrag.endPointerXy, + ) + ) { + flushPendingStart(activeDrag); + } + // ``start_position`` is recomputed live inside buildDragMessage, // so the wire payload always reflects the click point's // current world position (tracking the moving object). Skip - // while dormant -- the current modifier matches no binding. + // while dormant -- the current modifier matches no binding -- + // or while a switch-start is still pending confirmation. if (activeDrag.segmentActive) { sendDragMessage(activeDrag, "update", true); } @@ -511,6 +580,9 @@ function DragLayerActive({ children }: { children?: React.ReactNode }) { // still fail if the live grab point is unavailable, so we // trust its return value rather than assuming ``true``. segmentActive: false, + // The OPENING segment is never deferred -- crossing the + // motion threshold at promotion is already its confirmation. + pendingStart: null, releaseCameraLock: null, cleanup, }; diff --git a/src/viser/client/src/dragUtils.ts b/src/viser/client/src/dragUtils.ts index 42cb213f9..a88c15e4d 100644 --- a/src/viser/client/src/dragUtils.ts +++ b/src/viser/client/src/dragUtils.ts @@ -177,6 +177,19 @@ export function hasCmdCtrl(modifier: KeyModifier | null): boolean { * sites by reference, not by repeated literal. */ export const MOTION_THRESHOLD_PX = 3; +/** How long a *switch-created* drag segment's ``start`` is held back + * before being sent. A mid-drag modifier change ends the old segment + * immediately, but the new segment's ``start`` only goes out once the + * switch is confirmed: by this timer expiring, or sooner by pointer + * motion past :data:`MOTION_THRESHOLD_PX` from the switch point. If the + * button is released first, the pending ``start`` is discarded -- + * releasing the modifier a beat before the mouse button (the natural + * order for ending a modifier-drag) must not fire a degenerate + * ``start``/``end`` pair on whatever combo remains. The same philosophy + * as the motion threshold at drag promotion, applied at segment + * boundaries. */ +export const SWITCH_START_DELAY_MS = 100; + /** ``true`` when the L∞ distance between ``start`` and ``end`` exceeds * :data:`MOTION_THRESHOLD_PX`. Equivalent to the duplicated inline * ``Math.abs(end[0] - start[0]) > N || Math.abs(end[1] - start[1]) > N`` @@ -237,8 +250,20 @@ export type ActiveDragState = { /** Whether an in-flight segment is currently active (a ``start`` was * sent and its ``end`` hasn't). ``false`` while dormant -- the gesture * is physically held but the current modifier matches no binding, so - * no messages are sent. */ + * no messages are sent -- and while a switch-created ``start`` is + * still pending confirmation (see ``pendingStart``). */ segmentActive: boolean; + /** A switch-created segment whose ``start`` has not been sent yet + * (see :data:`SWITCH_START_DELAY_MS`). Confirmed -- and the ``start`` + * sent -- by the timer or by pointer motion past the threshold from + * ``switchPointerXy`` (canvas-relative, captured at the switch); + * discarded wholesale if the drag ends or the modifier changes again + * first, in which case no message was ever sent for the segment. + * ``null`` when no start is pending. */ + pendingStart: { + timerId: ReturnType; + switchPointerXy: [number, number]; + } | null; /** Release for the camera-control lock held for the lifetime of * this drag. Called in `stopActiveDrag` (and on every cancel * path). Routing through `cameraLock` keeps a concurrent diff --git a/tests/e2e/test_scene_node_drag.py b/tests/e2e/test_scene_node_drag.py index 433a7bbda..a52bcdda5 100644 --- a/tests/e2e/test_scene_node_drag.py +++ b/tests/e2e/test_scene_node_drag.py @@ -521,6 +521,108 @@ def test_scene_node_drag_binding_added_mid_drag( ) +def test_scene_node_drag_modifier_release_before_button( + page: Page, + viser_server: viser.ViserServer, +) -> None: + """Releasing the modifier a beat before the mouse button must not fire + a degenerate segment on the combo left behind. + + With both a bare binding (no modifier) and ``cmd/ctrl`` registered, + ending a Ctrl-drag "modifier first, button second" (the natural + order) switches to the bare combo for the few milliseconds before + release. A switch-created segment's ``start`` is deferred + (``SWITCH_START_DELAY_MS``) and discarded when the button comes up + inside the window -- so the bare callback sees NOTHING, and the ctrl + callback sees one clean pair. A follow-up bare drag proves the bare + binding itself is live (the zero above isn't vacuous).""" + viser_server.initial_camera.position = (0.0, 0.0, 4.0) + viser_server.initial_camera.look_at = (0.0, 0.0, 0.0) + + bare_starts = [0] + bare_ends = [0] + ctrl_starts = [0] + ctrl_ends = [0] + lock = threading.Lock() + ctrl_started = threading.Event() + ctrl_ended = threading.Event() + bare_started = threading.Event() + bare_ended = threading.Event() + + box = viser_server.scene.add_box( + "/release_order_box", + dimensions=(4.0, 4.0, 0.2), + color=(240, 120, 120), + ) + + def _bare_start(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + bare_starts[0] += 1 + bare_started.set() + + def _bare_end(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + bare_ends[0] += 1 + bare_ended.set() + + def _ctrl_start(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + ctrl_starts[0] += 1 + ctrl_started.set() + + def _ctrl_end(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: + with lock: + ctrl_ends[0] += 1 + ctrl_ended.set() + + _on_drag_phase(box, "start", "left")(_bare_start) + _on_drag_phase(box, "end", "left")(_bare_end) + _on_drag_phase(box, "start", "left", modifier="cmd/ctrl")(_ctrl_start) + _on_drag_phase(box, "end", "left", modifier="cmd/ctrl")(_ctrl_end) + + wait_for_connection(page, viser_server.get_port()) + wait_for_scene_node(page, "/release_order_box") + + (start_x, start_y), (end_x, end_y) = _get_canvas_drag_points(page) + + # Ctrl-drag, then end it modifier-first: keyup immediately followed by + # mouse-up, well inside the deferral window. + page.keyboard.down("Control") + page.mouse.move(start_x, start_y) + page.mouse.down() + page.mouse.move(end_x, end_y, steps=8) + assert ctrl_started.wait(timeout=5.0), "cmd/ctrl segment didn't start" + page.keyboard.up("Control") + page.mouse.up() + assert ctrl_ended.wait(timeout=5.0), "cmd/ctrl segment didn't end" + + # Give any (buggy) deferred bare start ample time to fire, then pin: + # the bare binding saw nothing, the ctrl binding saw one clean pair. + page.wait_for_timeout(600) + with lock: + assert bare_starts[0] == 0 and bare_ends[0] == 0, ( + "modifier-first release fired a degenerate segment on the bare " + f"binding: {bare_starts[0]} starts, {bare_ends[0]} ends" + ) + assert ctrl_starts[0] == 1 and ctrl_ends[0] == 1, ( + ctrl_starts[0], + ctrl_ends[0], + ) + + # Positive control: a plain unmodified drag drives the bare binding. + page.mouse.move(start_x, start_y) + page.mouse.down() + page.mouse.move(end_x, end_y, steps=8) + assert bare_started.wait(timeout=5.0), "bare segment didn't start" + page.mouse.up() + assert bare_ended.wait(timeout=5.0), "bare segment didn't end" + with lock: + assert bare_starts[0] == 1 and bare_ends[0] == 1, ( + bare_starts[0], + bare_ends[0], + ) + + def test_scene_node_drag_many_stages_one_drag( page: Page, viser_server: viser.ViserServer, From 83cab2170d02bd205cee7e24d32e443b2778dc64 Mon Sep 17 00:00:00 2001 From: Brent Yi Date: Sat, 18 Jul 2026 01:02:21 -0700 Subject: [PATCH 8/8] Strengthen the release-order e2e; fix start-at-press docstrings The discard pin alone was satisfiable with the keyup path broken entirely (the pointerup would end the ctrl segment and bare would still read zero), and its keyup->mouseup gap raced the 100ms deferral timer on slow CI. Split into two gestures: A holds the button through the window so the keyup's pending bare start CONFIRMS on the timer (proving the keyup creates a pending segment, and doubling as the bare-binding positive control); B dispatches keyup + pointerup in one JS task -- both listeners are window-level -- making the inside-the-window release deterministic before asserting bare never budged. Also correct the on_drag docs: start fires when the press is confirmed as a drag (motion threshold), not at press. --- src/viser/_scene_handles.py | 10 ++- tests/e2e/test_scene_node_drag.py | 103 +++++++++++++++++++++++------- 2 files changed, 86 insertions(+), 27 deletions(-) diff --git a/src/viser/_scene_handles.py b/src/viser/_scene_handles.py index 2cc104bf7..23d9daae7 100644 --- a/src/viser/_scene_handles.py +++ b/src/viser/_scene_handles.py @@ -474,8 +474,10 @@ class SceneNodeDragEvent(Generic[TSceneNodeHandle]): target: TSceneNodeHandle """Scene node that is being dragged.""" phase: DragPhase - """Drag lifecycle phase: ``"start"`` at press, ``"update"`` on - every throttled pointermove (~20Hz), ``"end"`` at release. + """Drag lifecycle phase: ``"start"`` once a press is confirmed as a + drag (the pointer travels past a small motion threshold -- a + stationary press/release fires nothing), ``"update"`` on every + throttled pointermove (~20Hz), ``"end"`` at release. A gesture is partitioned into one *segment* per held modifier-combo. Each segment fires exactly one ``"start"``, zero or more @@ -621,7 +623,9 @@ def on_drag( ) -> Any: """Attach a callback for the full drag lifecycle. - Fires once with ``event.phase == "start"`` at press, zero or + Fires once with ``event.phase == "start"`` when a press is + confirmed as a drag (the pointer travels past a small motion + threshold; a stationary press/release fires nothing), zero or more times with ``"update"`` (throttled pointermove), and once with ``"end"`` at release. ``end`` fires even on cancellation paths (window blur, pointer cancel, node removed mid-drag) so diff --git a/tests/e2e/test_scene_node_drag.py b/tests/e2e/test_scene_node_drag.py index a52bcdda5..bc921dce5 100644 --- a/tests/e2e/test_scene_node_drag.py +++ b/tests/e2e/test_scene_node_drag.py @@ -528,14 +528,23 @@ def test_scene_node_drag_modifier_release_before_button( """Releasing the modifier a beat before the mouse button must not fire a degenerate segment on the combo left behind. - With both a bare binding (no modifier) and ``cmd/ctrl`` registered, - ending a Ctrl-drag "modifier first, button second" (the natural - order) switches to the bare combo for the few milliseconds before - release. A switch-created segment's ``start`` is deferred - (``SWITCH_START_DELAY_MS``) and discarded when the button comes up - inside the window -- so the bare callback sees NOTHING, and the ctrl - callback sees one clean pair. A follow-up bare drag proves the bare - binding itself is live (the zero above isn't vacuous).""" + With both a bare binding (no modifier) and ``cmd/ctrl`` registered, a + Ctrl keyup mid-drag switches to the bare combo -- but the + switch-created segment's ``start`` is deferred + (``SWITCH_START_DELAY_MS``) and discarded if the button comes up + inside the window. Two gestures pin both halves: + + A. CONFIRM: Ctrl keyup with the button HELD. The ctrl segment ends on + the keyup (asserted before any release), and the pending bare + start fires on the deferral timer -- proving the keyup really + creates a pending segment (gesture B's zero isn't vacuous: the + identical keyup, un-released, produces a bare segment). + B. DISCARD: Ctrl keyup + pointerup dispatched in the SAME JS task + (both listeners are window-level, so synthetic events reach them; + the same-task dispatch makes the sub-window gap deterministic -- + real key/mouse calls race the 100ms timer on slow CI). The keyup + schedules the pending bare start; the pointerup discards it before + any timer can fire. Bare must not budge.""" viser_server.initial_camera.position = (0.0, 0.0, 4.0) viser_server.initial_camera.look_at = (0.0, 0.0, 0.0) @@ -585,41 +594,87 @@ def _ctrl_end(_event: viser.SceneNodeDragEvent[viser.BoxHandle]) -> None: (start_x, start_y), (end_x, end_y) = _get_canvas_drag_points(page) - # Ctrl-drag, then end it modifier-first: keyup immediately followed by - # mouse-up, well inside the deferral window. + # Record the mouse pointerId for gesture B's same-task synthetic + # release (handleWindowPointerUp filters on it). + page.evaluate( + """() => { window.__pid = null; + window.addEventListener('pointermove', + (e) => { window.__pid = e.pointerId; }, true); }""" + ) + + # --- Gesture A (CONFIRM): Ctrl keyup with the button HELD. page.keyboard.down("Control") page.mouse.move(start_x, start_y) page.mouse.down() page.mouse.move(end_x, end_y, steps=8) assert ctrl_started.wait(timeout=5.0), "cmd/ctrl segment didn't start" page.keyboard.up("Control") + # The ctrl end comes from the KEYUP -- asserted before any release. + assert ctrl_ended.wait(timeout=5.0), ( + "cmd/ctrl segment didn't end on keyup (button still held)" + ) + # The keyup scheduled a pending bare start; held stationary past the + # deferral window, the timer confirms it. + assert bare_started.wait(timeout=5.0), ( + "held past the deferral window, the pending bare start should have " + "fired on the timer" + ) page.mouse.up() - assert ctrl_ended.wait(timeout=5.0), "cmd/ctrl segment didn't end" - - # Give any (buggy) deferred bare start ample time to fire, then pin: - # the bare binding saw nothing, the ctrl binding saw one clean pair. - page.wait_for_timeout(600) + assert bare_ended.wait(timeout=5.0), "bare segment didn't end on release" with lock: - assert bare_starts[0] == 0 and bare_ends[0] == 0, ( - "modifier-first release fired a degenerate segment on the bare " - f"binding: {bare_starts[0]} starts, {bare_ends[0]} ends" - ) assert ctrl_starts[0] == 1 and ctrl_ends[0] == 1, ( ctrl_starts[0], ctrl_ends[0], ) + assert bare_starts[0] == 1 and bare_ends[0] == 1, ( + bare_starts[0], + bare_ends[0], + ) - # Positive control: a plain unmodified drag drives the bare binding. + # --- Gesture B (DISCARD): the identical keyup, but the button comes + # up in the SAME JS task -- inside the window, deterministically. + page.keyboard.down("Control") page.mouse.move(start_x, start_y) page.mouse.down() page.mouse.move(end_x, end_y, steps=8) - assert bare_started.wait(timeout=5.0), "bare segment didn't start" + for _ in range(50): + with lock: + if ctrl_starts[0] == 2: + break + page.wait_for_timeout(100) + with lock: + assert ctrl_starts[0] == 2, "second cmd/ctrl segment didn't start" + pid = page.evaluate("() => window.__pid") + assert pid is not None, "pointer id not observed" + page.evaluate( + """([x, y, pid]) => { + window.dispatchEvent(new KeyboardEvent('keyup', { bubbles: true })); + window.dispatchEvent(new PointerEvent('pointerup', { + bubbles: true, clientX: x, clientY: y, button: 0, + pointerId: pid, pointerType: 'mouse', isPrimary: true })); + }""", + [end_x, end_y, pid], + ) + # Reset the real input state (the drag already ended; both no-op). page.mouse.up() - assert bare_ended.wait(timeout=5.0), "bare segment didn't end" + page.keyboard.up("Control") + + for _ in range(50): + with lock: + if ctrl_ends[0] == 2: + break + page.wait_for_timeout(100) + # Ample time for any (buggy) surviving pending start to fire. + page.wait_for_timeout(600) with lock: + assert ctrl_starts[0] == 2 and ctrl_ends[0] == 2, ( + ctrl_starts[0], + ctrl_ends[0], + ) assert bare_starts[0] == 1 and bare_ends[0] == 1, ( - bare_starts[0], - bare_ends[0], + "modifier-first release fired a degenerate segment on the bare " + f"binding: {bare_starts[0]} starts, {bare_ends[0]} ends " + "(expected the gesture-A pair only)" )