Conversation
…limited graphs (#3340) ## Summary: On unlimited point and polygon graphs, using the arrow keys to move a point leaves `isCurrentlyDragging` `true`. This makes it so that clicking the graph to add new point no longer works. This happens because we call `setIsCurrentlyDragging(true)` from `onMove` which fires for _both_ mouse drags and keyboard movement. As a result, any keyboard interaction leaves the graph thinking a drag was still in progress. This PR adds a dedicated `onDragStart` callback to `useDraggable` that only fires at the very start of a mouse drag. Issue: LEMS-3977 ## Test plan: - Test via Storybook by clicking to add a point, then using the keyboard keys to move the point, then clicking elsewhere on the graph again to create a new point. - Added tests to verify that `onDragStart` fires on mouse drag but not on keyboard movement. Author: jonmarkprice Reviewers: claude[bot], SonicScrewdriver, jonmarkprice Required Reviewers: Approved By: SonicScrewdriver Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful Pull Request URL: #3340
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +56 B (+0.01%) Total Size: 486 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (a93d628) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3355If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3355If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3355 |
SonicScrewdriver
left a comment
SonicScrewdriver
left a comment
There was a problem hiding this comment.
Already approved earlier :)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| if (first) { | ||
| pickup.current = vec.transform(point, userTransform); | ||
| dragStarted.current = false; | ||
| } | ||
| if (vec.mag(pixelMovement) === 0) { | ||
| return; | ||
| } | ||
| // Don't start a drag if no movement; a click with zero | ||
| // movement should not set isCurrentlyDragging and block | ||
| // subsequent click-to-add-point events. | ||
| if (!dragStarted.current) { | ||
| dragStarted.current = true; | ||
| onDragStart?.(); | ||
| } |
There was a problem hiding this comment.
🟡 Race condition: moving setIsCurrentlyDragging(true) from onMove to onDragStart loses the self-correction that protected against a stale dragEndCallbackTimer resetting the flag mid-drag. If a user clicks a point (no movement) and then starts dragging another point within 400ms, the stale timer fires and sets isCurrentlyDragging to false during the active drag, defeating the iOS phantom click protection. Consider clearing the timer in onDragStart (e.g., dragEndCallbackTimer.clear()) to prevent the stale callback from interfering.
Extended reasoning...
What the bug is
The PR moves setIsCurrentlyDragging(true) from onMove (called every animation frame during a drag) to the new onDragStart callback (called only once when a drag begins). This introduces a race condition with the 400ms dragEndCallbackTimer used to block phantom clicks on iOS Safari.
The asymmetry that enables the race
In use-draggable.ts, onDragEnd is called at line 167-168 when last is true — this happens on mouseup before the zero-movement early return at line 175-176. However, onDragStart is called at line 181-183 after the zero-movement guard. This means a click on a point with no mouse movement triggers onDragEnd (starting the 400ms timer) but does not trigger onDragStart. The PR's own test "does not call onDragStart on a click with no mouse movement" confirms this intentional asymmetry.
Step-by-step proof
-
t=0ms: User clicks/taps on point A with no mouse movement.
useDragfires withlast=trueandpixelMovement=[0,0]. At line 167,onDragEnd()is called, which callsdragEndCallbackTimer.set()starting a 400ms timer to setisCurrentlyDragging = false. The zero-movement check at line 175 causes an early return, soonDragStartis never called. -
t=200ms: User begins dragging point B.
useDragfires withfirst=trueand then with actual movement. At line 181-183,onDragStart()fires, callingsetIsCurrentlyDragging(true). The drag is now in progress. -
t=400ms: The stale timer from step 1 fires, calling
setIsCurrentlyDragging(false). The drag on point B is still active, but the flag is now incorrectlyfalse. -
t=500ms+: User releases point B. On iOS Safari, a phantom click event fires at the original touch location. The
onClickhandler on the<rect>checksisCurrentlyDragging(line 133 in point.tsx), finds itfalse, and adds an unwanted new point.
Why the old code did not have this problem
In the old code, setIsCurrentlyDragging(true) was called inside onMove, which fires on every animation frame (~60fps). Even if the stale timer fired mid-drag, the very next onMove call (~16ms later) would reset isCurrentlyDragging back to true, providing continuous self-correction. The new code calls onDragStart only once, so there is no self-correction mechanism.
Impact and fix
This affects both UnlimitedPointGraph (point.tsx:157) and UnlimitedPolygonGraph (polygon.tsx:557), which both use the same onDragStart + onDragEnd + dragEndCallbackTimer pattern. The timing window is narrow (click a point then start dragging within 400ms) and only manifests on iOS Safari where phantom clicks occur, but it is a regression introduced by this PR. The simplest fix would be to clear the timer when onDragStart fires in both point.tsx and polygon.tsx. For example: onDragStart={() => { dragEndCallbackTimer.clear(); setIsCurrentlyDragging(true); }}.
…ragEndCallbackTimer when starting a new drag (#3363) ## Summary: Clears any pending `dragEndCallbackTimer` when a new drag begins in the unlimited point and polygon graphs, preventing a stale timer (started by a stationary click) from resetting `isCurrentlyDragging` mid-drag and defeating the iOS Safari phantom click protection introduced in #2895. This issue was called out by a late [Claude Code review on another PR](#3355 (comment)) that was unfortunately missed by myself. ## Test plan: - Tests pass Author: SonicScrewdriver Reviewers: jonmarkprice, claude[bot] Required Reviewers: Approved By: jonmarkprice Checks: ✅ 10 checks were successful, ⏭️ 1 check has been skipped, ⚪️ 1 check is neutral Pull Request URL: #3363
…ragEndCallbackTimer when starting a new drag (#3363) ## Summary: Clears any pending `dragEndCallbackTimer` when a new drag begins in the unlimited point and polygon graphs, preventing a stale timer (started by a stationary click) from resetting `isCurrentlyDragging` mid-drag and defeating the iOS Safari phantom click protection introduced in #2895. This issue was called out by a late [Claude Code review on another PR](#3355 (comment)) that was unfortunately missed by myself. ## Test plan: - Tests pass Author: SonicScrewdriver Reviewers: jonmarkprice, claude[bot] Required Reviewers: Approved By: jonmarkprice Checks: ✅ 10 checks were successful, ⏭️ 1 check has been skipped, ⚪️ 1 check is neutral Pull Request URL: #3363
3 participants
I used
git landto land a PR on my deploy branch, but realized I need another one to merge it tomain.[LEMS-3977] Fix keyboard arrow keys blocking click-to-add-point on unlimited graphs #3340