Skip to content

Fix increment number field drag jump at start#3915

Open
afrdbaig7 wants to merge 2 commits intoGraphiteEditor:masterfrom
afrdbaig7:fix-number-input-drag-jump
Open

Fix increment number field drag jump at start#3915
afrdbaig7 wants to merge 2 commits intoGraphiteEditor:masterfrom
afrdbaig7:fix-number-input-drag-jump

Conversation

@afrdbaig7
Copy link
Contributor

Fixes #2807
The PointerLock API has been known to report some pretty wild movementX values (often around ±1000px) during the first few events right after you activate the lock, particularly in Chrome. Previously, the code only ignored the very first event, but it turned out that the 2nd or 3rd events could also be off, leading to those crazy jumps. To tackle this issue, I introduced a brief settling window: - We still skip the first event without question. - For the next few events (up to 5 events or 80ms to accommodate high-polling-rate mice), we’ll zero out any delta that exceeds 50px. - This 50px threshold effectively catches those huge ~1000px outliers while still allowing for genuine human drag speeds right at the start of a movement. I applied this same approach to both the web pointerMove and desktop pointerLockMove handlers.

Keavon and others added 2 commits March 19, 2026 02:24
…ents to GitHub environments

Restore the CI "View deployment" button in PRs after building

Consolidate release.yml functionality into build.yml

Move build from ci.yml to a delegated run in build.yml
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses an issue where the NumberInput component experienced erratic value jumps when dragging due to unreliable initial movementX values from the PointerLock API, particularly in Chrome. The solution implements a robust filtering mechanism to stabilize the input during the start of a drag operation, ensuring a smoother and more predictable user experience by ignoring initial erratic movements and clamping large deltas within a settling window.

Highlights

  • Stabilized PointerLock Dragging: Implemented a 'settling window' for pointermove and pointerLockMove events to filter out erroneous initial movementX values, which previously caused erratic value jumps in the NumberInput component.
  • Refined Event Filtering Logic: The first movement event after pointer lock is now always skipped. For subsequent events within a defined settling window (up to 5 events or 80ms), any delta exceeding 50px is zeroed out, effectively preventing large, incorrect jumps while allowing genuine human drag speeds.
  • Updated Platform Detection: Replaced the isPlatformNative() check with isDesktop() for clearer intent and consistency in determining whether to use pointer lock or desktop app window pointer lock.
  • Improved Snapping Behavior: Modified the number input's snapping logic to only round values when the snapping property is explicitly enabled, rather than also implicitly for integers.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request effectively addresses the number input drag jump issue by implementing a settling window to filter out erratic initial mouse movements. The logic is applied to both web and desktop handlers. My review includes a critical note about a potentially missing isDesktop function, which could break the build. I've also included a couple of medium-severity suggestions: one regarding a possible unintended change in behavior for integer inputs, and another to refactor duplicated code for better maintainability.

import { PRESS_REPEAT_DELAY_MS, PRESS_REPEAT_INTERVAL_MS } from "@graphite/io-managers/input";
import { browserVersion } from "@graphite/utility-functions/platform";
import type { NumberInputMode, NumberInputIncrementBehavior, ActionShortcut } from "@graphite/messages";
import { browserVersion, isDesktop } from "@graphite/utility-functions/platform";
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The function isDesktop is imported from @graphite/utility-functions/platform, but it does not appear to be defined or exported in the provided context for frontend/src/utility-functions/platform.ts. This will likely cause a build or runtime error. Please ensure isDesktop is correctly implemented and exported. It seems to be replacing isPlatformNative, which was previously imported from the wasm package.

Comment on lines +448 to 454
if (initialValueBeforeDragging !== undefined && e instanceof CustomEvent) {
let delta = (e.detail as { x: number }).x;
const settling = moveEvents <= SETTLING_EVENTS || performance.now() - dragStart < SETTLING_MS;
if (settling && Math.abs(delta) > MAX_DELTA) delta = 0;

pointerLockMoveUpdate(delta, shiftKeyDown, ctrlKeyDown, initialValueBeforeDragging);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This block of logic for sanitizing the delta is duplicated from the pointerMove handler above. To improve maintainability and avoid redundancy, you could extract this logic into a single helper function within the beginDrag scope and call it from both places.

For example, you could define a function like this before pointerUp:

const sanitizeDelta = (delta: number): number => {
    const settling = moveEvents <= SETTLING_EVENTS || performance.now() - dragStart < SETTLING_MS;
    if (settling && Math.abs(delta) > MAX_DELTA) return 0;
    return delta;
};

Then you could call sanitizeDelta() from both pointerMove and pointerLockMove.


const combined = initialValue + cumulativeDragDelta;
const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
const combineSnapped = snapping ? Math.round(combined) : combined;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Removing || isInteger from this condition changes the dragging behavior for integer inputs. Previously, the value would snap to the nearest integer while dragging. Now, it will only snap when holding Ctrl, and will otherwise show decimal values during the drag, snapping to an integer only when the drag is complete (inside updateValue). Was this change in user experience intentional? The previous behavior seems more intuitive for fields marked as integer-only.

const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 1 file

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="frontend/src/components/widgets/inputs/NumberInput.svelte">

<violation number="1" location="frontend/src/components/widgets/inputs/NumberInput.svelte:489">
P2: Removing `|| isInteger` here changes the dragging behavior for integer-typed inputs: previously the value snapped to the nearest integer during the drag, but now it will show decimals until the drag completes. This seems unintentional given that the PR targets only the pointer-lock jump fix. Restore the original condition to preserve integer-field snapping while dragging.

```suggestion
		const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
```</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.


const combined = initialValue + cumulativeDragDelta;
const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
const combineSnapped = snapping ? Math.round(combined) : combined;
Copy link
Contributor

Choose a reason for hiding this comment

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

P2: Removing || isInteger here changes the dragging behavior for integer-typed inputs: previously the value snapped to the nearest integer during the drag, but now it will show decimals until the drag completes. This seems unintentional given that the PR targets only the pointer-lock jump fix. Restore the original condition to preserve integer-field snapping while dragging.

Suggested change
const combineSnapped = snapping ? Math.round(combined) : combined;
const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
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 489:

<comment>Removing `|| isInteger` here changes the dragging behavior for integer-typed inputs: previously the value snapped to the nearest integer during the drag, but now it will show decimals until the drag completes. This seems unintentional given that the PR targets only the pointer-lock jump fix. Restore the original condition to preserve integer-field snapping while dragging.

```suggestion
		const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
```</comment>

<file context>
@@ -469,7 +486,7 @@
 
 		const combined = initialValue + cumulativeDragDelta;
-		const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
+		const combineSnapped = snapping ? Math.round(combined) : combined;
 
 		const newValue = updateValue(combineSnapped);
</file context>
Suggested change
const combineSnapped = snapping ? Math.round(combined) : combined;
const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JS debugging: dragging the increment mode number field sometimes jumps at the start

2 participants