Skip to content

Commit

Permalink
Fix 'overhandling' of arrow keypresses (#169)
Browse files Browse the repository at this point in the history
## Summary:

Over in Khan/perseus#1445 we noticed that our vendored (and slightly
customized) version of `useMovable` (which we've renamed `useDraggable`)
had a bug where we handled both `keydown` and `keyup` events, causing a
sort of "double dispatch" of move events.

This is because (from our observations in modern browsers), a single
keypress only triggers a `keydown` (but no `keyup`). If you hold the key
down, you get multiple `keydown` events and a single `keyup` when the
key is released.

This can be observed if you look closely on
https://mafs.dev/guides/interaction/movable-points.

If you select the point and then hold down an arrow key until it start
moving, let go and you'll notice it seems to "overshoot" the stopping
point. This is because the final `keyup` is processed as a distinct
event, causing one extra move. This is subtle but the fix makes the feel
of holding and releasing the arrow keys for movement feel more natural.

It also fixes an issue where movement keypresses on a mobile device
(iPhone) using a connected bluetooth keyboard cause double-movement.

## Test plan:

`yarn start`
Navigate to the "Movable Points" page
Select the movable point with the keyboard
Hold down an arrow key until the point moves multiple times
Release the arrow key
Note that the point stops as you'd expect
(Previously, the point would "overshoot" by one movement)
  • Loading branch information
jeremywiebe authored Oct 15, 2024
1 parent 74fd1da commit 10af3d1
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/interaction/useMovable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@ export function useMovable(args: UseMovableArguments): UseMovable {
const isKeyboard = type.includes("key")
if (isKeyboard) {
event?.preventDefault()

// When a key is held down, we see multiple "keydown" events,
// followed by a single "keyup" event.
// For a single keypress, we only see a "keydown" event, no
// "keyup".
// We never want to process the keyup event as an intent to
// move so we bail on further processing here.
if (type === "keyup") {
return
}

const { direction: yDownDirection, altKey, metaKey, shiftKey } = state

const direction = [yDownDirection[0], -yDownDirection[1]] as vec.Vector2
Expand Down

0 comments on commit 10af3d1

Please sign in to comment.