Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 41 additions & 8 deletions frontend/src/components/widgets/inputs/NumberInput.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -418,8 +418,33 @@
// while dragging it outside the browser window, because Safari has another bug where the cursor icon is unaffected by that API.
if (isSafari) document.body.classList.add("cursor-hidden");
// Enter dragging state
if (usePointerLock) target.requestPointerLock();
// Tracks whether the drag has already ended (the mouse was released). Used to resolve the race where
// pointer lock finishes engaging only after the user has already released the mouse button.
let dragReleased = false;
// Enter dragging state.
// `requestPointerLock()` is asynchronous: the pointer isn't actually locked until the "pointerlockchange"
// event fires. If the user releases the mouse before that happens (e.g. a rapid click or a quick
// drag-and-release), `endDrag()` cleans up the drag listeners directly, but the still-pending lock may engage
// afterward — by which point our "pointerlockchange" listener has already been removed. We use the returned
// promise (in browsers that support it) to exit such a late-engaging lock so the cursor doesn't get stuck
// locked, and to confirm the drag was already cleaned up if the request is rejected. Without this, the value
// could stay tied to mouse movement. See: <https://github.com/GraphiteEditor/Graphite/issues/4231>
if (usePointerLock) {
const lockRequest = target.requestPointerLock();
if (lockRequest instanceof Promise) {
lockRequest.then(
() => {
// If the user already released before the lock engaged, exit it immediately so we aren't stuck locked.
if (dragReleased && document.pointerLockElement) document.exitPointerLock();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Checking document.pointerLockElement without comparing to target is too broad. If another element has acquired pointer lock between the request and this resolution, calling document.exitPointerLock() here would incorrectly release their lock. Use document.pointerLockElement === target to ensure we only exit our own lock.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/widgets/inputs/NumberInput.svelte, line 439:

<comment>Checking `document.pointerLockElement` without comparing to `target` is too broad. If another element has acquired pointer lock between the request and this resolution, calling `document.exitPointerLock()` here would incorrectly release their lock. Use `document.pointerLockElement === target` to ensure we only exit our own lock.</comment>

<file context>
@@ -418,8 +418,33 @@
+				lockRequest.then(
+					() => {
+						// If the user already released before the lock engaged, exit it immediately so we aren't stuck locked.
+						if (dragReleased && document.pointerLockElement) document.exitPointerLock();
+					},
+					() => {
</file context>
Suggested change
if (dragReleased && document.pointerLockElement) document.exitPointerLock();
if (dragReleased && document.pointerLockElement === target) document.exitPointerLock();

},
Comment on lines +435 to +440

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Checking document.pointerLockElement is too broad here. If another element or widget has requested and acquired pointer lock in the meantime, calling document.exitPointerLock() would incorrectly release their lock instead of ours. We should explicitly check if document.pointerLockElement === target to ensure we only exit the lock if it belongs to this specific input element.

			if (lockRequest instanceof Promise) {
				lockRequest.then(
					() => {
						// If the user already released before the lock engaged, exit it immediately so we aren't stuck locked.
						if (dragReleased && document.pointerLockElement === target) document.exitPointerLock();
					},

() => {
// The lock request was rejected (e.g. the browser's brief cooldown after rapidly re-locking).
// Nothing to do here: `pointerUp` cleans up on release since we never entered pointer lock.
},
);
}
}
if (import.meta.env.MODE === "native") {
editor.appWindowPointerLock();
}
Expand All @@ -443,24 +468,21 @@
initialValueBeforeDragging = value;
cumulativeDragDelta = 0;
if (usePointerLock) document.exitPointerLock();
else pointerLockChange();
endDrag();
};
const pointerMove = (e: PointerEvent) => {
// TODO: Display a fake cursor over the top of the page which wraps around the edges of the editor.
// Abort the drag if right click is down. This works here because a "pointermove" event is fired when right clicking even if the cursor didn't move.
if (e.buttons & BUTTONS_RIGHT) {
if (usePointerLock) document.exitPointerLock();
else pointerLockChange();
endDrag();
return;
}
// If no buttons are down, that means we are stuck in the drag state after having released the mouse, so we should exit.
// For some reason on Firefox in Wayland, `e.buttons` can be 0 while `e.button` is -1, but we don't want to exit in that state.
if (e.buttons === 0 && e.button !== -1) {
if (usePointerLock) document.exitPointerLock();
else pointerLockChange();
endDrag();
return;
}
Expand Down Expand Up @@ -497,6 +519,17 @@
// Close out the transaction `startDragging` opened so the many emits collapse into one history step (covers both confirmed and aborted drags).
commitTransactionIfInProgress();
};
// Ends the drag and runs the cleanup. If the pointer is actually locked, exiting it fires "pointerlockchange",
// which performs the cleanup. Otherwise (we're not using pointer lock, or the lock hasn't engaged yet because
// the interaction ended too quickly) no such event will arrive, so we clean up directly. This prevents a stuck
// drag where the value stays tied to mouse movement (#4231). Setting `dragReleased` also lets a pointer lock
// that engages after this point exit itself immediately (see the `requestPointerLock()` handling above).
const endDrag = () => {
dragReleased = true;
if (usePointerLock && document.pointerLockElement) document.exitPointerLock();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Same issue here: document.pointerLockElement should be compared to target to avoid releasing another element's pointer lock. If the lock belongs to a different element, we should fall through to calling pointerLockChange() directly to clean up our own listeners.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/widgets/inputs/NumberInput.svelte, line 530:

<comment>Same issue here: `document.pointerLockElement` should be compared to `target` to avoid releasing another element's pointer lock. If the lock belongs to a different element, we should fall through to calling `pointerLockChange()` directly to clean up our own listeners.</comment>

<file context>
@@ -497,6 +519,17 @@
+		const endDrag = () => {
+			dragReleased = true;
+
+			if (usePointerLock && document.pointerLockElement) document.exitPointerLock();
+			else pointerLockChange();
+		};
</file context>

else pointerLockChange();
};
Comment on lines +527 to +532

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similarly, we should only call document.exitPointerLock() if the active pointer lock belongs to our target. If it belongs to another element, we should bypass exiting it and directly call pointerLockChange() to clean up our own listeners immediately.

		const endDrag = () => {
			dragReleased = true;

			if (usePointerLock && document.pointerLockElement === target) document.exitPointerLock();
			else pointerLockChange();
		};

addEventListener("pointerup", pointerUp);
addEventListener("pointermove", pointerMove);
Expand Down