Repeated key presses when a modifier key is held do not fire events

davepagurek commented 1 week ago

Most appropriate sub-area of p5.js?

[ ] Accessibility
[ ] Color
[ ] Core/Environment/Rendering
[ ] Data
[ ] DOM
[X] Events
[ ] Image
[ ] IO
[ ] Math
[ ] Typography
[ ] Utilities
[ ] WebGL
[ ] Build process
[ ] Unit testing
[ ] Internationalization
[ ] Friendly errors
[ ] Other (specify if possible)

p5.js version

1.10.0

Web browser and version

Firefox, Chrome

Operating system

MacOS

Steps to reproduce this

The goal here was to have a custom undo handler using the meta key. A single meta-z should be handled, but also holding the meta key and repeatedly hitting the z key should also fire. On my mac, I'm finding that this works for the first press but not repeated presses:

function setup() {
  createCanvas(400, 400);
}

function keyPressed(event) {
  if (event.metaKey && event.keyCode === 90) {
    console.log('undo')
    return false
  }
}

Meanwhile, doing it with vanilla js does work:

function setup() {
  createCanvas(400, 400);
  document.addEventListener('keydown', function(event) {
    if (event.metaKey && event.keyCode === 90) {
      console.log('undo')
      event.stopPropagation()
    }
  })
}

Narrowing it down, it looks like the key up event isn't firing after the first meta-z if I release the z but hold the meta key. I'm not sure yet why that is, but because of that, this._downkeys isn't getting reset to false, so I suspect the second event isn't getting fired because of this condition: https://github.com/processing/p5.js/blob/e9ee594421f02b0659c0c04c1d8ee0de284127b7/src/events/keyboard.js#L443-L447

samarsrivastav commented 3 days ago

Hi @davepagurek, I’m interested in working on this issue but I'm having difficulty reproducing it. Could you kindly provide a more detailed explanation to help me understand the issue here?

davepagurek commented 3 days ago

Sure thing! I'll try to explain the logs in the console and what's expected. In this sketch: https://editor.p5js.org/davepagurek/sketches/BQ2Lmicii If I click on the canvas to give it focus, and then do the following:

Press and hold the cmd key
Press and release the Z key five times
Release the cmd key

I would expect the "undo" log to happen once per z press, for a total of five times. Instead it only happens once. I see the "down" log (logged when a key is depressed) just twice, once for the first press of the meta key and once for the first press of the z key, and not repeatedly after each z press like i would expect.

samarsrivastav commented 2 days ago

Hello @davepagurek,

I reviewed the code and confirmed that the issue stems from the event not firing multiple times.

This problem can be resolved by commenting out the following section in the onKeyDown method:

if (this._downKeys[e.which]) {
  // prevent multiple firings
  return;
}

You can find the relevant code here.

By removing this block of code, I am confident that the issue will be resolved. However, I am uncertain if this change might introduce additional issues elsewhere in the codebase.

davepagurek commented 2 days ago

I suspect it was introduced so that keyPressed only gets called right when the key is pressed. If you press and hold a key, it just gets called once. If you comment out that check, it will continue to fire while you hold it because the browser issues repeated key press events.

There are other ways once can have that same behaviour, such as returning if the KeyboardEvent's repeat property is set to true. This works for the scenario where you press and hold a key (it only fires once), but introduces a new issue: since we still aren't receiving a keyup event, p5 will continue to think the key is down. Here's a sketch to test that cmd-z shortcut, but using the .repeat property: https://editor.p5js.org/davepagurek/sketches/B42za0os4 Although the original test of holding cmd and pressing z multiple times still works, the keyIsDown check never returns back to false until you hit the z key without the cmd key being held.

I was doing some more research into why this event isn't being fired in the first place by the browser, since fixing that would be the ideal scenario. It sounds like it's just how Chrome and Safari work on Mac and that there may not be much we can do about it.

So, knowing this behaviour, it looks like maybe the best way forward is to:

Use the .repeat check to make sure we can capture repeated presses
Record which keys were pressed in combination with the meta key, and then when the meta key is released, manually update _downkeys so that they're all set to false

Here's a quick mockup of that: https://editor.p5js.org/davepagurek/sketches/OeM8Bp42E

How do you feel about this workaround? It means that while you press and hold meta, and then tap and release z, while you continue to hold meta, p5 will log that keyIsDown(90) is still true until you release the meta key. Maybe that's sufficient, since as soon as you release the meta key it goes back to normal, and we may not be able to do better?

samarsrivastav commented 2 days ago

I see the approach you're taking:

You're adding a check for the Meta key combinations.

Using the repeat property helped resolve the issue of continuous firing.
However, the problem arises when the key pressed with the Meta key (such as 'Z') isn't released properly.
To address this, you're incorporating an additional step that tracks the keys pressed with meta key are released when the Meta key is released.

samarsrivastav commented 2 days ago

I suspect it was introduced so that keyPressed only gets called right when the key is pressed. If you press and hold a key, it just gets called once. If you comment out that check, it will continue to fire while you hold it because the browser issues repeated key press events.

There are other ways once can have that same behaviour, such as returning if the KeyboardEvent's repeat property is set to true. This works for the scenario where you press and hold a key (it only fires once), but introduces a new issue: since we still aren't receiving a keyup event, p5 will continue to think the key is down. Here's a sketch to test that cmd-z shortcut, but using the .repeat property: https://editor.p5js.org/davepagurek/sketches/B42za0os4 Although the original test of holding cmd and pressing z multiple times still works, the keyIsDown check never returns back to false until you hit the z key without the cmd key being held.

I was doing some more research into why this event isn't being fired in the first place by the browser, since fixing that would be the ideal scenario. It sounds like it's just how Chrome and Safari work on Mac and that there may not be much we can do about it.

So, knowing this behaviour, it looks like maybe the best way forward is to:

Use the .repeat check to make sure we can capture repeated presses

Record which keys were pressed in combination with the meta key, and then when the meta key is released, manually update _downkeys so that they're all set to false

Here's a quick mockup of that: https://editor.p5js.org/davepagurek/sketches/OeM8Bp42E

How do you feel about this workaround? It means that while you press and hold meta, and then tap and release z, while you continue to hold meta, p5 will log that keyIsDown(90) is still true until you release the meta key. Maybe that's sufficient, since as soon as you release the meta key it goes back to normal, and we may not be able to do better?

I think this solution solves the issue of repeated keypresses with meta key, the manual tracking of "_metaKeys" seems like a solid approach to me

processing / p5.js