preventDefault seems to have no effect

[silverwind]

Describe the bug

I'd like to prevent the browser from presenting a "Save Page" dialog when pressingh Ctrl/Cmd + S. With mousetrap, this is possible via simple e.preventDefault(), but this library does not expose such a function and trying to to it on the various originalEvent seems to not work at all.

To Reproduce

1. Open https://codesandbox.io/s/admiring-lamport-jh8k7h?file=/src/index.js
2. Focus the preview frame, hit "Ctrl + S" or "Cmd + S"

Expected behavior

No "Save Page" dialog to be shown.

Additional context

A more ergonomic e.preventDefault without having to use originalEvent would be nice.

[silverwind]

For reference, here is a working demo with mousetrap.

What may also be a bug here is that the Ctrl+S combo is to not being captured on at least Chrome and Safari on MacOS, while in mousetrap, it is.

[RobertWHurst]

Yes, that makes sense. This is a little tricky because, for performance reasons, I only update combo states once per tick. Because of this bucketing of updates, your handler runs in a tick after the one the original event has been dispatched in. This is why the call to preventDefault does nothing - it's already too late by the time your handler is called.

I have a solution in mind, I'll put it together and should be able to make a release in the next little bit.

[silverwind]

I see, thanks. Maybe you could also for convenience expose a preventDefault function directly on the returned event because I'm confused whether I'm supposed to use finalKeyEvent.originalEvent or iterate e.keyEvents in this case, these do seem like undocumented internal APIs.

[RobertWHurst]

So after some experimentation, I've decided to remove the combo update batching. It will resolve this issue and any other event interaction related issues in the browser and otherwise. I've also added preventDefault to the browser event so you don't need to reach into the original event to call it. Checkout #20 if you're curious.

This means you will be able to do exactly what you expected to be able to do when you opened this issue:

bindKeyCombo('ctrl > s', (event) => {
  event.preventDefault()
  // ...
})

I'm going to sit on this for a bit and test a bit more, then do a release this evening.

[RobertWHurst]

@silverwind ok, just released 1.2.0. You will need to reach into the finalKeyEvent property and call prevent default.

bindKeyCombo('control > s', (event) => {
  event.finalKeyEvent.preventDefault()
  // ...
})

Let me know if you have any further issues. Thanks for raising this issue.

[silverwind]

Thanks, preventDefault works and save dialog is prevented. Updated demo:

https://codesandbox.io/s/quizzical-nightingale-c6t37s

There is another issue related to Control key on Mac, I will open a separate issue for it.

[silverwind]

Actually I may have spoken to soon, if I input Command+s in the demo above two times in a row, it seems to only prevent the save dialog on first combo, subsequent combos fail to prevent it.

Edit: Opened #21, I think the underlying root cause may be the same for both issues.

[RobertWHurst]

Hmm, this might be due to a different issue. Likely related to how the meta/command key is handled by Keystrokes on mac; on macOS the command key does not properly fire a keyup event. There is some special logic to handle this issue in src/browser-bindings.ts. I have a mac at home, but I'm away this week. I should be able to take a look when I get back around friday.

Edit: just saw your edit. I'll follow up with you in that issue.

[silverwind]

Thanks. Yes, this is most definitely a Mac-only issue. It seems like something inside the module breaks after the first entry of a sequence with the command key.

No hurries, I'll continue using mousetrap in my project until this is sorted out.

[noahsbrom]

Hi @RobertWHurst

I'm trying to test the behavior of preventDefault in a simple unit test with keystrokes.press(). The binding works as expected in the browser, but the unit test fails with the following error:

TypeError: e.finalKeyEvent.preventDefault is not a function.

I was experimenting with @silverwind 's code, and it can be found here: https://codesandbox.io/s/blazing-silence-6pn9np?file=/src/test.spec.ts

Thanks!

[RobertWHurst]

Ah yes, I see the issue.

By default Keystrokes uses the browser bindings, but the test wrapper for creating a test instance of Keystrokes does not implement the browser bindings. Keystrokes allows setting up any bindings you like, that way it can be used in electron, or games or really anything else for that matter, in a way that is agnostic about how the events are generated. I built the test instance for building keystrokes own unit tests (minus the browser binding tests), but I didn't consider the fact that most people will need a test instance that matches the browser bindings. I'll likely introduce defaults to the test instance that match the browser bindings, and at the same time allow passing overrides.

Let's open another issue for this.