diff --git a/apps/desktop/src/main/ipc/input.test.ts b/apps/desktop/src/main/ipc/input.test.ts index 6044a6e..693b469 100644 --- a/apps/desktop/src/main/ipc/input.test.ts +++ b/apps/desktop/src/main/ipc/input.test.ts @@ -12,7 +12,7 @@ vi.mock('../input/injector', () => ({ enabled: false, backend: 'nut-js', backendSupported: true, - stats: { received: 0, injected: 0, rejected: 0, errors: 0 }, + stats: { received: 0, injected: 0, rejected: 0, errors: 0, coalesced: 0 }, heldButtons: 0, heldKeys: 0, }), @@ -100,7 +100,7 @@ describe('IPC Input Handlers', () => { enabled: true, backend: 'nut-js', backendSupported: true, - stats: { received: 0, injected: 0, rejected: 0, errors: 0 }, + stats: { received: 0, injected: 0, rejected: 0, errors: 0, coalesced: 0 }, heldButtons: 0, heldKeys: 0, }); @@ -135,7 +135,7 @@ describe('IPC Input Handlers', () => { enabled: true, backend: 'nut-js', backendSupported: true, - stats: { received: 1, injected: 1, rejected: 0, errors: 0 }, + stats: { received: 1, injected: 1, rejected: 0, errors: 0, coalesced: 0 }, heldButtons: 0, heldKeys: 0, }); diff --git a/apps/desktop/src/renderer/components/control/InputCapture.test.tsx b/apps/desktop/src/renderer/components/control/InputCapture.test.tsx index 61bd55f..4e7e032 100644 --- a/apps/desktop/src/renderer/components/control/InputCapture.test.tsx +++ b/apps/desktop/src/renderer/components/control/InputCapture.test.tsx @@ -99,58 +99,41 @@ describe('InputCapture pointer lock', () => { vi.clearAllMocks(); }); - it('sends real coordinates when there is no pointer lock', () => { + // The guest's complaint that drove this: their real cursor walks out of the + // video the moment they push toward an edge, so they cannot reach a menu bar, + // a corner, or a window's controls. Clicking the picture takes the pointer. + it('captures the pointer when the guest clicks the picture', () => { const browser = installBrowserApis(); - const onInputEvent = vi.fn(); - const { container } = renderCapture(onInputEvent); + const { container } = renderCapture(vi.fn()); act(() => { clickAt(container, 250, 250); }); - expect(browser.lockRequests).toHaveLength(0); - expect(onInputEvent).toHaveBeenCalledWith( - expect.objectContaining({ action: 'down', x: 0.25, y: 0.5 }) - ); + expect(browser.lockRequests).toHaveLength(1); }); - // The regression. Fullscreen is granted, the lock is refused — Chromium does - // exactly this during the cooldown after an Escape — and the click must still - // carry where the guest actually clicked. - it('keeps using real coordinates when the lock is refused', async () => { - const browser = installBrowserApis(); + // That first click is how the guest reaches for the window, not for whatever + // happens to be under the pointer at the time. + it('does not forward the click that takes control', () => { + installBrowserApis(); const onInputEvent = vi.fn(); - const { container, getByRole } = renderCapture(onInputEvent); - - await act(async () => { - getByRole('button').click(); - await browser.grantFullscreen(); - }); - - act(() => { - browser.denyLock(); - }); + const { container } = renderCapture(onInputEvent); - onInputEvent.mockClear(); act(() => { clickAt(container, 250, 250); }); - expect(onInputEvent).toHaveBeenCalledWith( - expect.objectContaining({ action: 'down', x: 0.25, y: 0.5 }) - ); + expect(onInputEvent).not.toHaveBeenCalled(); }); - it('uses the virtual position once the lock is granted', async () => { + it('uses the virtual position once the lock is granted', () => { const browser = installBrowserApis(); const onInputEvent = vi.fn(); - const { container, getByRole } = renderCapture(onInputEvent); + const { container } = renderCapture(onInputEvent); - await act(async () => { - getByRole('button').click(); - await browser.grantFullscreen(); - }); act(() => { + clickAt(container, 250, 250); browser.grantLock(container); }); @@ -172,61 +155,94 @@ describe('InputCapture pointer lock', () => { ); }); - // Escape releases the lock without leaving fullscreen. The lifecycle effect - // used to see "fullscreen, granted, unlocked" on the next render and ask for - // the lock straight back, so Escape appeared to do nothing. - it('does not grab the pointer back after the guest presses Escape', async () => { + // A refusal must not leave the guest unable to do anything. Chromium refuses + // during the cooldown after an Escape, and some embeddings refuse outright. + it('falls back to absolute coordinates when the lock is refused', () => { + vi.useFakeTimers(); + try { + const browser = installBrowserApis(); + const onInputEvent = vi.fn(); + const { container } = renderCapture(onInputEvent); + + act(() => { + clickAt(container, 250, 250); + browser.denyLock(); + }); + + // The component waits briefly before concluding the lock is not coming. + act(() => { + vi.advanceTimersByTime(1500); + }); + + onInputEvent.mockClear(); + act(() => { + clickAt(container, 250, 250); + }); + + expect(onInputEvent).toHaveBeenCalledWith( + expect.objectContaining({ action: 'down', x: 0.25, y: 0.5 }) + ); + } finally { + vi.useRealTimers(); + } + }); + + // Escape releases the lock. The lifecycle effect used to see "granted, not + // locked" on the next render and ask for it straight back, so Escape appeared + // to do nothing. + it('does not grab the pointer back after the guest presses Escape', () => { const browser = installBrowserApis(); - const onInputEvent = vi.fn(); - const { container, getByRole } = renderCapture(onInputEvent); + const { container } = renderCapture(vi.fn()); - await act(async () => { - getByRole('button').click(); - await browser.grantFullscreen(); - }); act(() => { + clickAt(container, 250, 250); browser.grantLock(container); }); const requestsBefore = browser.lockRequests.length; act(() => { - // Escape: the browser drops the lock and leaves fullscreen alone. document.exitPointerLock(); }); expect(browser.lockRequests).toHaveLength(requestsBefore); + }); + + it('re-captures when the guest clicks again after releasing', () => { + const browser = installBrowserApis(); + const { container } = renderCapture(vi.fn()); - // And the guest's real cursor drives coordinates again. - onInputEvent.mockClear(); act(() => { clickAt(container, 250, 250); + browser.grantLock(container); + document.exitPointerLock(); }); - expect(onInputEvent).toHaveBeenCalledWith( - expect.objectContaining({ action: 'down', x: 0.25, y: 0.5 }) - ); + + const requestsBefore = browser.lockRequests.length; + act(() => { + clickAt(container, 250, 250); + }); + + expect(browser.lockRequests.length).toBe(requestsBefore + 1); }); - it('re-captures the pointer when the guest asks for it again', async () => { + it('goes back to asking for control when control is revoked', () => { const browser = installBrowserApis(); - const { container, getByRole } = renderCapture(vi.fn()); + const { container, rerender } = renderCapture(vi.fn()); - await act(async () => { - getByRole('button').click(); - await browser.grantFullscreen(); - }); act(() => { + clickAt(container, 250, 250); browser.grantLock(container); - document.exitPointerLock(); }); - const requestsBefore = browser.lockRequests.length; act(() => { - getByRole('button').click(); + rerender( + + + ); }); - expect(browser.lockRequests.length).toBe(requestsBefore + 1); - // Still fullscreen: the click asked for the pointer, not for the exit. - expect(document.fullscreenElement).not.toBeNull(); + expect(document.pointerLockElement).toBeNull(); }); }); diff --git a/apps/desktop/src/renderer/components/control/InputCapture.tsx b/apps/desktop/src/renderer/components/control/InputCapture.tsx index 7be0c4c..37c6b71 100644 --- a/apps/desktop/src/renderer/components/control/InputCapture.tsx +++ b/apps/desktop/src/renderer/components/control/InputCapture.tsx @@ -48,13 +48,28 @@ export function InputCapture({ onMove: handlePointerMove, }); + // The guest has asked to drive, by clicking into the picture. + // + // Capture is deliberately not tied to fullscreen. Without the pointer held, + // the guest's real cursor walks straight out of the video the moment they + // push toward an edge — so they cannot reach a menu bar, a corner, or a + // window's controls, which is most of what taking control is *for*. Making + // that conditional on entering fullscreen first put the fix behind a step + // nobody found. + const [captureRequested, setCaptureRequested] = useState(false); + // The browser refused the lock. Rather than leaving the guest unable to do + // anything, fall back to plain absolute coordinates — worse at the edges, + // but working. + const [lockDenied, setLockDenied] = useState(false); + // Wanting the lock and having it are different states, and conflating them is // a silent failure: a request can be denied (no user gesture, or Chromium's - // cooldown after an Escape) and then the guest sees their real cursor over a - // fullscreen video while every click is sent from a virtual position they - // are not steering. So the lock must be *granted* before coordinates come - // from anywhere but the event. - const wantsLock = controlState === 'granted' && isFullscreen && !wasReleasedByUser; + // cooldown after an Escape) and then the guest sees their real cursor over + // the video while every click is sent from a virtual position they are not + // steering. So the lock must be *granted* before coordinates come from + // anywhere but the event. + const wantsLock = + controlState === 'granted' && captureRequested && !wasReleasedByUser && !lockDenied; const { isCapturing, startCapture, stopCapture } = useRemoteControl({ enabled, @@ -84,6 +99,53 @@ export function InputCapture({ } }, [isCapturing]); + // Clicking the picture takes control of the pointer. + // + // Capture phase, so this runs before useRemoteControl's own mousedown on the + // same container and can stop the click that *acquires* control from also + // being sent to the host — the guest is reaching for the window, not for + // whatever happens to be under it. + useEffect(() => { + const container = containerRef.current; + if (!container) return; + if (controlState !== 'granted' || !enabled) return; + + const onMouseDown = (event: MouseEvent) => { + if (isLocked || lockDenied) return; + event.stopPropagation(); + event.preventDefault(); + // Clears the "they pressed Escape" latch, so asking again works. + resetPosition(); + setCaptureRequested(true); + lock(container); + }; + + container.addEventListener('mousedown', onMouseDown, { capture: true }); + return () => { + container.removeEventListener('mousedown', onMouseDown, { capture: true }); + }; + }, [controlState, enabled, isLocked, lockDenied, lock, resetPosition]); + + // A refusal is not permanent, but it must not be retried on a loop either. + // Record it so coordinates stay absolute and the guest keeps working. + useEffect(() => { + if (!captureRequested || isLocked) return; + const timer = setTimeout(() => { + if (!document.pointerLockElement) setLockDenied(true); + }, 1000); + return () => { + clearTimeout(timer); + }; + }, [captureRequested, isLocked]); + + // Control ending resets the whole interaction, so the next grant starts from + // "click to take control" rather than from whatever state this was left in. + useEffect(() => { + if (controlState === 'granted') return; + setCaptureRequested(false); + setLockDenied(false); + }, [controlState]); + // Pointer lock lifecycle: request when fullscreen and control is granted, // release when either ends. // @@ -173,6 +235,14 @@ export function InputCapture({
)} + {controlState === 'granted' && !isLocked && !lockDenied && ( +
+
+ Click to control · Esc to release +
+
+ )} + {allowFullscreen && controlState === 'granted' && ( + )}
); } diff --git a/apps/web/src/hooks/usePointerLock.test.ts b/apps/web/src/hooks/usePointerLock.test.ts new file mode 100644 index 0000000..d38aa15 --- /dev/null +++ b/apps/web/src/hooks/usePointerLock.test.ts @@ -0,0 +1,226 @@ +/** + * The pointer lock lifecycle, which shipped untested. + * + * The pure delta maths had thorough tests; the state machine around it had + * none, and that is where the failures were: a lock that was wanted but never + * granted still redirected every coordinate, and an Escape was answered by + * immediately asking for the lock back. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import { usePointerLock } from './usePointerLock'; + +/** + * jsdom implements neither pointer lock nor its events, so the browser half is + * modelled here: requestPointerLock is granted or denied on demand, and the + * corresponding event is dispatched the way Chromium does. + */ +function installPointerLock(): { + grant: (element: Element) => void; + deny: () => void; + release: () => void; + requests: Element[]; +} { + const requests: Element[] = []; + let current: Element | null = null; + + Object.defineProperty(document, 'pointerLockElement', { + configurable: true, + get: () => current, + }); + + Element.prototype.requestPointerLock = function requestPointerLock(this: Element) { + requests.push(this); + return undefined as unknown as Promise; + }; + + document.exitPointerLock = () => { + current = null; + document.dispatchEvent(new Event('pointerlockchange')); + }; + + return { + grant(element: Element) { + current = element; + document.dispatchEvent(new Event('pointerlockchange')); + }, + deny() { + current = null; + document.dispatchEvent(new Event('pointerlockerror')); + }, + release() { + current = null; + document.dispatchEvent(new Event('pointerlockchange')); + }, + requests, + }; +} + +describe('usePointerLock', () => { + let browser: ReturnType; + let element: HTMLDivElement; + + beforeEach(() => { + browser = installPointerLock(); + element = document.createElement('div'); + document.body.append(element); + }); + + it('is not locked until the browser says so', () => { + const { result } = renderHook(() => usePointerLock({ onMove: vi.fn() })); + + act(() => { + result.current.lock(element); + }); + + // Requested, but not granted. Anything that treats this as locked will send + // coordinates from a virtual pointer the guest is not steering. + expect(browser.requests).toHaveLength(1); + expect(result.current.isLocked).toBe(false); + + act(() => { + browser.grant(element); + }); + expect(result.current.isLocked).toBe(true); + }); + + // Chromium refuses a lock during the ~1s cooldown after an Escape, and + // without pointerlockerror a refusal is indistinguishable from a pending + // request that never resolves. + it('stays unlocked when the request is denied', () => { + const { result } = renderHook(() => usePointerLock({ onMove: vi.fn() })); + + act(() => { + result.current.lock(element); + browser.deny(); + }); + + expect(result.current.isLocked).toBe(false); + }); + + // Escape exits the lock but not fullscreen, so the caller would see + // "fullscreen, granted, unlocked" and ask again. This flag is what tells it + // the guest wants their cursor back. + it('reports a release the guest initiated', () => { + const { result } = renderHook(() => usePointerLock({ onMove: vi.fn() })); + + act(() => { + result.current.lock(element); + browser.grant(element); + }); + expect(result.current.wasReleasedByUser).toBe(false); + + act(() => { + browser.release(); + }); + + expect(result.current.isLocked).toBe(false); + expect(result.current.wasReleasedByUser).toBe(true); + }); + + // Our own unlock (leaving fullscreen, control revoked) is not the guest + // pressing Escape, and latching it as such would block the next deliberate + // lock for the rest of the session. + it('does not mistake its own unlock for the guest pressing Escape', () => { + const { result } = renderHook(() => usePointerLock({ onMove: vi.fn() })); + + act(() => { + result.current.lock(element); + browser.grant(element); + }); + + act(() => { + result.current.unlock(); + }); + + expect(result.current.isLocked).toBe(false); + expect(result.current.wasReleasedByUser).toBe(false); + }); + + it('clears the release latch on reset so the guest can re-capture', () => { + const { result } = renderHook(() => usePointerLock({ onMove: vi.fn() })); + + act(() => { + result.current.lock(element); + browser.grant(element); + browser.release(); + }); + expect(result.current.wasReleasedByUser).toBe(true); + + act(() => { + result.current.resetPosition(); + }); + expect(result.current.wasReleasedByUser).toBe(false); + }); + + it('accumulates movement deltas into a clamped virtual position', () => { + const onMove = vi.fn(); + const { result } = renderHook(() => usePointerLock({ onMove })); + + // The hook measures the surface to convert pixels into a fraction of it. + vi.spyOn(element, 'getBoundingClientRect').mockReturnValue({ + width: 1000, + height: 500, + } as DOMRect); + + act(() => { + result.current.lock(element); + browser.grant(element); + }); + + act(() => { + document.dispatchEvent( + Object.assign(new MouseEvent('mousemove'), { movementX: 100, movementY: 50 }) + ); + }); + + expect(onMove).toHaveBeenCalledWith(0.6, 0.6, true); + expect(result.current.positionRef.current).toEqual({ x: 0.6, y: 0.6 }); + }); + + // The point of the whole feature: pushing past the top edge pins the pointer + // there instead of the guest's real cursor leaving the window. + it('pins at an edge when the guest overshoots', () => { + const onMove = vi.fn(); + const { result } = renderHook(() => usePointerLock({ onMove })); + + vi.spyOn(element, 'getBoundingClientRect').mockReturnValue({ + width: 1000, + height: 500, + } as DOMRect); + + act(() => { + result.current.lock(element); + browser.grant(element); + }); + + act(() => { + document.dispatchEvent( + Object.assign(new MouseEvent('mousemove'), { movementX: 0, movementY: -5000 }) + ); + }); + + expect(result.current.positionRef.current.y).toBe(0); + }); + + it('ignores movement once the lock is gone', () => { + const onMove = vi.fn(); + const { result } = renderHook(() => usePointerLock({ onMove })); + + act(() => { + result.current.lock(element); + browser.grant(element); + browser.release(); + }); + onMove.mockClear(); + + act(() => { + document.dispatchEvent( + Object.assign(new MouseEvent('mousemove'), { movementX: 100, movementY: 100 }) + ); + }); + + expect(onMove).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/hooks/usePointerLock.ts b/apps/web/src/hooks/usePointerLock.ts new file mode 100644 index 0000000..a3ff337 --- /dev/null +++ b/apps/web/src/hooks/usePointerLock.ts @@ -0,0 +1,175 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; +import { getContainRect } from '@pairux/shared-types'; +import { advanceVirtualPointer } from '@/lib/virtualPointer'; + +interface UsePointerLockOptions { + /** Called with the current virtual position on every movement. */ + onMove: (x: number, y: number, visible: boolean) => void; +} + +interface UsePointerLockReturn { + /** True only once the browser has actually granted the lock. */ + isLocked: boolean; + /** + * The guest released the lock themselves (Escape, or switching apps). + * + * Distinct from "not locked": the caller must not immediately ask for it + * back, or Escape does nothing and the cursor never returns. + */ + wasReleasedByUser: boolean; + /** Live virtual position. A ref, because it changes far too often to render. */ + positionRef: React.RefObject<{ x: number; y: number }>; + /** Request pointer lock on this element. Must be called from a user gesture. */ + lock: (element: Element) => void; + /** Exit pointer lock. */ + unlock: () => void; + /** Reset the virtual position to centre and allow locking again. */ + resetPosition: () => void; +} + +/** + * Pointer lock with virtual position tracking. + * + * Under pointer lock there is no cursor position to read — the browser reports + * movement deltas instead — so the remote position is accumulated and clamped + * to the screen. This fixes the core problem with absolute-coordinate mapping: + * the guest's real cursor must be exactly on the video edge to reach the host's + * screen edge, and a few pixels further leaves the window entirely. With + * pointer lock the guest can push past an edge and stay there, which is what + * reaching a menu bar or a corner requires. + */ +export function usePointerLock({ onMove }: UsePointerLockOptions): UsePointerLockReturn { + const [isLocked, setIsLocked] = useState(false); + const [wasReleasedByUser, setWasReleasedByUser] = useState(false); + const positionRef = useRef({ x: 0.5, y: 0.5 }); + const lockedElementRef = useRef(null); + // Set while we are the ones exiting, so the exit is not misread as the guest + // pressing Escape — which would latch `wasReleasedByUser` and stop the next + // deliberate lock from ever being granted. + const exitingRef = useRef(false); + // A request is in flight. The browser answers asynchronously, so without this + // the lifecycle effect fires a second request in the render between asking + // and being answered — and a burst of requests is exactly what Chromium's + // abuse heuristics refuse. + const pendingRef = useRef(false); + + // The size of the *picture*, not of the element around it. + // + // The remote screen is letterboxed by `object-contain` whenever its aspect + // ratio differs from this window's, and movement has to be scaled against + // what the guest can actually see. Measuring the element instead makes the + // pointer travel too slowly along whichever axis carries the dead space, so + // crossing the screen takes further than it should on one axis and not the + // other — the drift that reads as "the cursor doesn't go where I push it". + const getSurfaceSize = useCallback((): { width: number; height: number } => { + const el = lockedElementRef.current; + if (!el) return { width: 1, height: 1 }; + + const video = el.querySelector('video'); + const rect = (video ?? el).getBoundingClientRect(); + const content = getContainRect( + rect.width, + rect.height, + video?.videoWidth ?? 0, + video?.videoHeight ?? 0 + ); + + return { width: content.width || 1, height: content.height || 1 }; + }, []); + + const handleMovement = useCallback( + (diffX: number, diffY: number) => { + const { width, height } = getSurfaceSize(); + positionRef.current = advanceVirtualPointer(positionRef.current, diffX, diffY, width, height); + onMove(positionRef.current.x, positionRef.current.y, true); + }, + [onMove, getSurfaceSize] + ); + + // Under pointer lock the browser keeps firing mousemove on the document, but + // clientX/Y stop advancing and movementX/Y carry the actual motion. That is + // the whole reason a virtual position has to be accumulated by hand. + useEffect(() => { + if (!isLocked) return; + + const onPointerMove = (event: MouseEvent) => { + handleMovement(event.movementX, event.movementY); + }; + + document.addEventListener('mousemove', onPointerMove); + return () => { + document.removeEventListener('mousemove', onPointerMove); + }; + }, [isLocked, handleMovement]); + + // Detect when the lock is granted, and when it goes away. + useEffect(() => { + const onChange = () => { + pendingRef.current = false; + const locked = + lockedElementRef.current !== null && + document.pointerLockElement === lockedElementRef.current; + setIsLocked(locked); + + if (locked) { + setWasReleasedByUser(false); + return; + } + + lockedElementRef.current = null; + // Escape, Cmd+Tab, or the browser deciding it has had enough. Whichever + // it was, the guest now has their real cursor back and must not have it + // taken away again until they ask. Chromium also refuses a re-lock for + // about a second after an Escape, so retrying here would fail silently + // and strand the session with neither a lock nor real coordinates. + if (!exitingRef.current) setWasReleasedByUser(true); + exitingRef.current = false; + }; + + document.addEventListener('pointerlockchange', onChange); + // Chromium fires this instead when the request is denied — during the + // post-Escape cooldown, or without a user gesture. Without handling it, a + // failed lock looks exactly like one that simply has not been granted yet. + document.addEventListener('pointerlockerror', onChange); + return () => { + document.removeEventListener('pointerlockchange', onChange); + document.removeEventListener('pointerlockerror', onChange); + }; + }, []); + + const lock = useCallback((element: Element) => { + if (pendingRef.current || document.pointerLockElement === element) return; + pendingRef.current = true; + lockedElementRef.current = element; + exitingRef.current = false; + // Pointer lock requires a user gesture, which the button click provides. + // The promise is intentionally unawaited: older Chromium returns undefined + // here, and the outcome is observed through the events above either way. + void (element.requestPointerLock() as unknown as Promise | undefined)?.catch(() => { + // Denied. The event handler has already put us back in the unlocked + // state; swallowing keeps it off the console as an unhandled rejection. + pendingRef.current = false; + }); + }, []); + + const unlock = useCallback(() => { + if (document.pointerLockElement) { + exitingRef.current = true; + document.exitPointerLock(); + } + }, []); + + const resetPosition = useCallback(() => { + positionRef.current = { x: 0.5, y: 0.5 }; + setWasReleasedByUser(false); + }, []); + + return { + isLocked, + wasReleasedByUser, + positionRef, + lock, + unlock, + resetPosition, + }; +} diff --git a/apps/web/src/lib/virtualPointer.test.ts b/apps/web/src/lib/virtualPointer.test.ts new file mode 100644 index 0000000..a65cec7 --- /dev/null +++ b/apps/web/src/lib/virtualPointer.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from 'vitest'; +import { advanceVirtualPointer } from './virtualPointer'; + +const centre = { x: 0.5, y: 0.5 }; + +describe('advanceVirtualPointer', () => { + it('converts a pixel delta into a normalized one', () => { + expect(advanceVirtualPointer(centre, 100, 50, 1000, 500)).toEqual({ x: 0.6, y: 0.6 }); + }); + + it('moves back for negative deltas', () => { + expect(advanceVirtualPointer(centre, -100, -50, 1000, 500)).toEqual({ x: 0.4, y: 0.4 }); + }); + + // The whole point: pushing past the top must pin the pointer there rather + // than stopping short or wrapping, so a menu bar can actually be reached by + // overshooting instead of by pixel-perfect aim. + it('pins at an edge when pushed past it', () => { + expect(advanceVirtualPointer({ x: 0.5, y: 0.02 }, 0, -500, 1000, 500)).toEqual({ + x: 0.5, + y: 0, + }); + }); + + it('pins at the far corner too', () => { + expect(advanceVirtualPointer({ x: 0.99, y: 0.99 }, 900, 900, 1000, 500)).toEqual({ + x: 1, + y: 1, + }); + }); + + // A surface with no size yet (not laid out) must not produce NaN and send + // the host's pointer somewhere undefined. + it('stays put when the surface has no size', () => { + expect(advanceVirtualPointer(centre, 100, 100, 0, 0)).toEqual(centre); + }); +}); diff --git a/apps/web/src/lib/virtualPointer.ts b/apps/web/src/lib/virtualPointer.ts new file mode 100644 index 0000000..2f39ee6 --- /dev/null +++ b/apps/web/src/lib/virtualPointer.ts @@ -0,0 +1,49 @@ +/** + * Where the guest is pointing on the host's screen, tracked independently of + * where their real cursor is. + * + * Mapping the guest's cursor position inside the video element straight onto + * the host's screen sounds right and is unusable in practice: to reach the top + * of the host's screen the guest's real cursor has to sit exactly on the top + * edge of the video, and a few pixels further leaves the window altogether. + * Screen corners are effectively unreachable, and overshooting silently stops + * input. Reported as "my mouse leaves the window when I try to go up to the + * top". + * + * Under pointer lock there is no cursor position to read — the browser reports + * movement deltas instead — so the remote position is accumulated here and + * clamped to the screen. The guest can then push past an edge and stay there, + * which is what reaching a menu bar or a corner requires. + */ + +export interface NormalizedPoint { + x: number; + y: number; +} + +function clamp01(value: number): number { + return Math.min(1, Math.max(0, value)); +} + +/** + * Advance a normalized position by a pixel delta over a surface of `width` x + * `height` pixels. + * + * Clamping rather than wrapping is deliberate: a pointer pushed at the top of + * the screen must stay pinned there, the way a real one does, so menu bars and + * corners are reachable by overshooting instead of requiring precision. + */ +export function advanceVirtualPointer( + from: NormalizedPoint, + movementX: number, + movementY: number, + width: number, + height: number +): NormalizedPoint { + if (width <= 0 || height <= 0) return from; + + return { + x: clamp01(from.x + movementX / width), + y: clamp01(from.y + movementY / height), + }; +} diff --git a/packages/remote-input/src/backends/waylandYdotool.test.ts b/packages/remote-input/src/backends/waylandYdotool.test.ts index e690ce6..615b31e 100644 --- a/packages/remote-input/src/backends/waylandYdotool.test.ts +++ b/packages/remote-input/src/backends/waylandYdotool.test.ts @@ -361,3 +361,38 @@ describe('WaylandYdotoolInputBackend', () => { expect(run).toHaveBeenNthCalledWith(2, 'ydotool', ['click', '198']); }); }); + +// The click that never landed. `mousemove` and `click` are two separate +// ydotool invocations writing to uinput, and the compositor processes that +// stream asynchronously — so without a gap the press can be delivered while +// the pointer is still where the host left it. The guest sees their cursor +// over a button, clicks, and nothing happens. The nut.js backend has waited a +// frame here since the same bug was found on macOS. +describe('WaylandYdotoolInputBackend click timing', () => { + it('lets the pointer arrive before pressing', async () => { + const order: string[] = []; + const run = vi.fn(async (_command: string, args: string[]) => { + order.push(args[0] === 'mousemove' ? 'move' : 'click'); + }); + const sleep = vi.fn(async () => { + order.push('settle'); + }); + + const backend = new WaylandYdotoolInputBackend( + run, + { hasBinary: true, hasSocket: true, socketPath: '/tmp/.ydotool_socket' }, + { sleep } + ); + + await backend.inject({ + type: 'mouse', + action: 'down', + button: 'left', + x: 0.5, + y: 0.5, + }); + + expect(order).toEqual(['move', 'settle', 'click']); + expect(sleep).toHaveBeenCalledWith(16); + }); +}); diff --git a/packages/remote-input/src/backends/waylandYdotool.ts b/packages/remote-input/src/backends/waylandYdotool.ts index fc8fc2d..efd6e26 100644 --- a/packages/remote-input/src/backends/waylandYdotool.ts +++ b/packages/remote-input/src/backends/waylandYdotool.ts @@ -205,6 +205,14 @@ function clickCode(action: MouseButtonEvent['action'], button: MouseButton): str } } +/** + * How long to wait for a synthetic pointer move to be applied before acting on + * it. Roughly one frame — long enough for the compositor, short enough that + * remote input still feels immediate. Mirrors POSITION_SETTLE_MS in the nut.js + * backend, which exists for the same reason. + */ +const POINTER_SETTLE_MS = 16; + function scrollClickCode(base: number, repeat: number): string[] { return repeat > 1 ? ['--repeat', String(repeat), String(0xc0 | base)] : [String(0xc0 | base)]; } @@ -599,6 +607,17 @@ export class WaylandYdotoolInputBackend implements InputBackend { } await this.run(this.ydotoolCommand, move); + // Let the pointer actually arrive before pressing. + // + // These are two separate ydotool invocations writing to uinput, and the + // compositor processes that stream asynchronously — so without a gap the + // press can be delivered while the pointer is still where the host left + // it, and the click lands somewhere the guest never aimed. The guest sees + // their cursor over a button, clicks, and nothing happens. + // + // The nut.js backend has waited a frame here since the same bug was found + // on macOS; this path never got it. + await this.sleep(POINTER_SETTLE_MS); await this.run(this.ydotoolCommand, click); } diff --git a/packages/remote-input/src/injector.test.ts b/packages/remote-input/src/injector.test.ts index 639d2d6..239b14e 100644 --- a/packages/remote-input/src/injector.test.ts +++ b/packages/remote-input/src/injector.test.ts @@ -817,3 +817,147 @@ describe('RemoteInputInjector edge margin', () => { expect(backend.inject).toHaveBeenCalledWith(expect.objectContaining({ x: 0, y: 0 })); }); }); + +/** + * Why the host felt laggy and unclickable. + * + * Every injection is serialized, and on Wayland each one costs a ydotool + * process spawn. A viewer streams pointer movement at their display's refresh + * rate, which arrives faster than that drains, so the queue grew without bound: + * the host's pointer trailed further and further behind and a click sat behind + * hundreds of stale positions before it was even attempted. + */ +describe('move coalescing', () => { + /** A backend whose injections only finish when the test says so. */ + function blockingBackend(): { + backend: InputBackend; + release: () => void; + injected: InputEvent[]; + } { + const injected: InputEvent[] = []; + let pending: (() => void)[] = []; + + const backend = fakeBackend({ + inject: vi.fn((event: InputEvent) => { + injected.push(event); + return new Promise((resolve) => { + pending.push(resolve); + }); + }), + }); + + return { + backend, + release: () => { + const waiting = pending; + pending = []; + for (const resolve of waiting) resolve(); + }, + injected, + }; + } + + function move(x: number): InputEvent { + return { type: 'mouse', action: 'move', x, y: 0.5 }; + } + + /** + * Let a blocked queue finish. Each release frees whatever is in flight, and + * the next entry needs a turn of the event loop to reach the backend, so this + * alternates until everything settles rather than guessing at a count. + */ + async function drain(release: () => void, promises: Promise[]): Promise { + // Releasing an already-empty queue is a no-op, so this just runs enough + // rounds to outlast any queue these tests build rather than tracking when + // to stop. + for (let i = 0; i < 20; i += 1) { + release(); + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + await Promise.all(promises); + } + + it('drops moves that a newer position has already replaced', async () => { + const { backend, release, injected } = blockingBackend(); + const injector = makeInjector(backend); + injector.enable(); + + // One injection in flight, then a burst behind it. + const inFlight = injector.inject(move(0.1)); + await Promise.resolve(); + const queued = [injector.inject(move(0.2)), injector.inject(move(0.3))]; + + await drain(release, [inFlight, ...queued]); + + // The middle position is gone: it was obsolete before it ever ran. + const xs = injected.filter((e) => 'x' in e).map((e) => (e as { x: number }).x); + expect(xs).not.toContain(0.2); + expect(injected.length).toBeLessThan(3); + }); + + it('never drops a click, however far behind the queue is', async () => { + const { backend, release, injected } = blockingBackend(); + const injector = makeInjector(backend); + injector.enable(); + + const all = [ + injector.inject(move(0.1)), + injector.inject(move(0.2)), + injector.inject(click), + injector.inject(move(0.3)), + ]; + + await drain(release, all); + + expect(injected.some((e) => 'action' in e && e.action === 'down')).toBe(true); + }); + + // Intermediate motion is the entire content of a drag: anything tracking one + // (text selection, canvas apps, file managers) needs the path, not just its + // endpoints. + it('keeps every move while a button is held', async () => { + const { backend, release, injected } = blockingBackend(); + const injector = makeInjector(backend); + injector.enable(); + + const down = injector.inject(click); + await drain(release, [down]); + + const drag = [injector.inject(move(0.2)), injector.inject(move(0.3))]; + await drain(release, drag); + + const xs = injected.filter((e) => 'x' in e).map((e) => (e as { x: number }).x); + expect(xs).toContain(0.2); + expect(xs).toContain(0.3); + }); + + it('reports where the viewer stopped even when the last move was dropped', async () => { + const { backend, release } = blockingBackend(); + const injector = makeInjector(backend); + injector.enable(); + + const inFlight = injector.inject(move(0.1)); + await Promise.resolve(); + const dropped = injector.inject(move(0.9)); + + await drain(release, [inFlight, dropped]); + + // The overlay still has to draw the guest's cursor where they left it. + expect(injector.getRemoteCursorPosition().x).toBe(0.9); + }); + + it('counts what it dropped, so a laggy host can be told apart from a busy one', async () => { + const { backend, release } = blockingBackend(); + const injector = makeInjector(backend); + injector.enable(); + + const inFlight = injector.inject(move(0.1)); + await Promise.resolve(); + const dropped = injector.inject(move(0.2)); + + await drain(release, [inFlight, dropped]); + + expect(injector.getDiagnostics().stats.coalesced).toBeGreaterThan(0); + }); +}); diff --git a/packages/remote-input/src/injector.ts b/packages/remote-input/src/injector.ts index e1c6367..eabfd71 100644 --- a/packages/remote-input/src/injector.ts +++ b/packages/remote-input/src/injector.ts @@ -22,6 +22,7 @@ import type { InputStats, MouseButton, MouseInputEvent, + MouseMoveEvent, } from './types.js'; export interface RemoteInputInjectorOptions { @@ -84,7 +85,13 @@ export class RemoteInputInjector { private readonly rateLimiter: InputRateLimiter; private readonly onRejected: RemoteInputInjectorOptions['onRejected']; private readonly logger: Pick; - private readonly stats: InputStats = { received: 0, injected: 0, rejected: 0, errors: 0 }; + private readonly stats: InputStats = { + received: 0, + injected: 0, + rejected: 0, + errors: 0, + coalesced: 0, + }; // What this injector is currently holding down on the host. Tracked so it // can always be released — a button left down puts the desktop into a @@ -128,6 +135,13 @@ export class RemoteInputInjector { // chains through this promise. disable() and emergencyStop() wait on it so // releaseAll always runs *after* trackHeldState has recorded the press. private pendingInject: Promise | null = null; + /** + * Pointer moves waiting on the queue. See `supersededMove`. + * + * A counter rather than a flag: the in-flight move and the one behind it are + * different things, and only the second is safe to drop. + */ + private queuedMoves = 0; constructor(options: RemoteInputInjectorOptions = {}) { this.selection = options.selection ?? getInputBackendSelection(); @@ -530,6 +544,31 @@ export class RemoteInputInjector { this.onRejected?.(reason, event, detail); } + /** + * Is this move already obsolete? + * + * Every injection is serialized, and on some hosts each one costs a process + * spawn. A viewer streams pointer movement at their display's refresh rate, + * which arrives faster than that drains — so the queue grows without bound, + * the host's pointer trails further and further behind, and a click ends up + * stuck behind hundreds of stale positions before it is even attempted. The + * session reads as "laggy and I cannot click anything". + * + * Nothing is lost by dropping a superseded move: only the newest position + * matters, because a move is a statement about where the pointer *is*, not a + * step along a path. Clicks, scrolls and keystrokes are never dropped — + * those are discrete actions, and one of them going missing is a bug. + * + * A move that is part of a drag is also kept: there the intermediate motion + * is the point, since anything tracking a drag (text selection, canvas apps, + * file managers) needs to see the path and not just its endpoints. + */ + private supersededMove(event: InputEvent): event is MouseMoveEvent { + if (event.type !== 'mouse' || event.action !== 'move') return false; + if (this.heldButtons.size > 0) return false; + return this.queuedMoves > 0; + } + async inject(event: InputEvent): Promise { this.stats.received += 1; @@ -538,6 +577,19 @@ export class RemoteInputInjector { return; } + // Drop a move that a newer one has already replaced, before it can take a + // place in the queue. Tracked rather than inferred, because the queue is a + // promise chain with no length to inspect. + const isMove = event.type === 'mouse' && event.action === 'move'; + if (this.supersededMove(event)) { + // Remember it anyway: if this turns out to be the last move the viewer + // sends, the cursor overlay still needs to know where they stopped. + this.remotePosition = { x: event.x, y: event.y }; + this.stats.coalesced += 1; + return; + } + if (isMove) this.queuedMoves += 1; + // Serialize every injection so disable() and emergencyStop() can wait for // us to finish (including trackHeldState) before they release everything. // Without this, releaseAll can check heldButtons while dispatch is @@ -579,6 +631,7 @@ export class RemoteInputInjector { error: error instanceof Error ? error.message : String(error), }); } finally { + if (isMove) this.queuedMoves = Math.max(0, this.queuedMoves - 1); resolve?.(); } } diff --git a/packages/remote-input/src/types.ts b/packages/remote-input/src/types.ts index 62ac10c..30c65ac 100644 --- a/packages/remote-input/src/types.ts +++ b/packages/remote-input/src/types.ts @@ -158,6 +158,15 @@ export interface InputStats { injected: number; rejected: number; errors: number; + /** + * Pointer moves dropped because a newer position had already replaced them. + * + * Expected to be large during normal use and is not a fault: a viewer streams + * movement far faster than any host can inject it, and only the newest + * position means anything. Worth watching all the same — if this is zero on a + * host that feels laggy, the backlog is somewhere else. + */ + coalesced: number; } export interface InputDiagnostics {