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

[davepagurek]

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]

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]

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]

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]

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]

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]

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

[thejon07]

i want to contribute on this issue.please assign me

[davepagurek]

Thanks @thejon07, feel free to open a PR implementing what was discussed above!

Most p5 development is off of the dev-2.0 branch now, which the 2.0 release will be made from. @limzykenneth do you think we should be encouraging PRs into the dev-2.0 branch now?

[JagjeevanAK]

Can i work on this ??

[limzykenneth]

@davepagurek I think for 2.0 its not quite ready yet as I still want to go through one more look into core refactor and we might want to fix the tests we ignored plus converted them to import individual modules first.

[davepagurek]

Ok sounds good, @thejon07 feel free to just work off of main for now then.

@JagjeevanAK, @thejon07 has just volunteered, so we'll let them work on this one first.

[thejon07]

how can i start development server

[davepagurek]

grunt yui:dev starts a dev server and opens a local copy of the reference. If you change the URL of the local reference to just be the root, I believe it will show you other folders you can navigate to, e.g. to run things in the test/manual-test-examples folder.

You can also run npm run build to build the library and then use the built p5.js file however you like, e.g. uploading it to the web editor, or using a local dev server in VSCode.

[thejon07]

Unfortunately my laptop was stolen, so I’ll need some time before I can contribute further, as I don’t currently have a laptop. (((

[msudipta888]

is this issue till open

[limzykenneth]

@msudipta888 This issue is currently already assigned so we are not looking for someone else to work on it at the moment. Thanks.

[thejon07]

My mac was stolen, so I currently don't have mac OS. Can I use Windows to address this issue instead?

[davepagurek]

We've had contributors work on Windows in the past! Let us know if you run into issues.

[woustachemax]

I'd like to contribute to this issue please let me know

[thejon07]

When pressing and holding cmd + z, browsers fire repeated keydown events for the z key. the goal is to ensure the event is triggered only once per keypress. Is my understanding correct?

[Rishab87]

Hi @davepagurek I have created a pull request which resolves this issue please check!

[davepagurek]

@thejon07 I think so. at least that's the current behaviour, so we can maintain that for now.

[davepagurek]

Hi @thejon07, apologies for having this issue be resolved by another PR. For future contributors who may not know, our contributor guidelines say:

You should not "jump the queue" by filing a PR for an issue that either someone else has indicated willingness to submit a contribution or has already been assigned to someone else.

We generally try to abide by that, so I'm sorry your spot in the queue got jumped in this instance. If you're still interested in contributing, let me know if any of the other issues we've tagged as Help Wanted are of interest!

[thejon07]

That's okay,i understand.i'd still love to contribute.Let me take a look at the other help wanted issues and see where i can assist.