From 5896ebf5a616cd1afe5adb8c50f6db139c96f6a5 Mon Sep 17 00:00:00 2001 From: Mr P-Tech Date: Tue, 11 Aug 2026 10:18:28 +0100 Subject: [PATCH] fix(control): hold the pointer, drop stale moves, settle before clicking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three symptoms from a real Mac-to-KDE session — "the mouse leaves the window", "I can't click anything", "it's super slow" — with three separate causes. The host's debug log has all of them. **The pointer escapes the video.** Taking control did not capture the pointer, so the guest's real cursor walked out of the picture the moment they pushed toward an edge. Reaching a menu bar, a corner, or a window's close button — most of what taking control is for — was impossible. Pointer lock existed but only on the desktop viewer, only inside fullscreen, behind a hover button in a corner; the web viewer had none at all. Now clicking the picture takes the pointer and Escape gives it back, no fullscreen involved, with the two words of instruction on screen while control is granted but not yet taken. The click that acquires control is not forwarded — the guest is reaching for the window, not for whatever sits under the pointer. If the browser refuses the lock, it falls back to absolute coordinates rather than leaving the guest unable to act. **Clicks landing nowhere.** On Wayland, positioning the pointer and pressing the button are two separate ydotool invocations writing to uinput, and the compositor processes that stream asynchronously — so the press could be delivered while the pointer was still where the host left it. The guest's cursor sits over a button, they click, nothing happens. The nut.js backend has waited a frame here since the same bug was found on macOS; this path never got it. The host log shows the two commands issued back to back with nothing between them. **Everything lagging.** Every injection is serialized, and on Wayland 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 grew without bound: the pointer trailed further and further behind and a click sat behind hundreds of stale positions before it was even attempted. Moves that a newer position has already replaced are now dropped before they can queue. Nothing is lost — a move says where the pointer *is*, not how it got there — and clicks, scrolls and keystrokes are never dropped, nor is any move during a drag, where the path is the whole point. The count is reported in diagnostics, so a host that feels slow with nothing coalesced points somewhere else. Co-Authored-By: Claude Opus 5 (1M context) --- apps/desktop/src/main/ipc/input.test.ts | 6 +- .../components/control/InputCapture.test.tsx | 136 +++++----- .../components/control/InputCapture.tsx | 80 +++++- apps/web/src/app/session/[id]/page.tsx | 1 + apps/web/src/app/view/[sessionId]/page.tsx | 1 + .../components/control/InputCapture.test.tsx | 248 ++++++++++++++++++ .../src/components/control/InputCapture.tsx | 214 ++++++++++++++- apps/web/src/hooks/usePointerLock.test.ts | 226 ++++++++++++++++ apps/web/src/hooks/usePointerLock.ts | 175 ++++++++++++ apps/web/src/lib/virtualPointer.test.ts | 37 +++ apps/web/src/lib/virtualPointer.ts | 49 ++++ .../src/backends/waylandYdotool.test.ts | 35 +++ .../src/backends/waylandYdotool.ts | 19 ++ packages/remote-input/src/injector.test.ts | 144 ++++++++++ packages/remote-input/src/injector.ts | 55 +++- packages/remote-input/src/types.ts | 9 + 16 files changed, 1359 insertions(+), 76 deletions(-) create mode 100644 apps/web/src/components/control/InputCapture.test.tsx create mode 100644 apps/web/src/hooks/usePointerLock.test.ts create mode 100644 apps/web/src/hooks/usePointerLock.ts create mode 100644 apps/web/src/lib/virtualPointer.test.ts create mode 100644 apps/web/src/lib/virtualPointer.ts diff --git a/apps/desktop/src/main/ipc/input.test.ts b/apps/desktop/src/main/ipc/input.test.ts index 6044a6ef..693b469b 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 61bd55f9..4e7e0321 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 7be0c4c9..37c6b71b 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 00000000..d38aa15d --- /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 00000000..a3ff337c --- /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 00000000..a65cec72 --- /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 00000000..2f39ee69 --- /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 e690ce66..615b31ee 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 fc8fc2db..efd6e26d 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 639d2d63..239b14eb 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 e1c63670..eabfd711 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 62ac10c9..30c65ac0 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 {