lupas / vue3-keypress

Global keypress event handler component for Vue 3 applications.
26 stars 2 forks source link

@success triggered when modifiers are pressed, and the component didn't define any modifiers. #1

Closed niuage closed 3 years ago

niuage commented 3 years ago

It might be intended, but I had an issue with this code (don't mind the dev-key-press, I just renamed the component).

<dev-key-press
    key-event="keydown"
    :key-code="9"
    @success="incrementSelectedIndex(1)"
    prevent-default>
</dev-key-press>

<dev-key-press
     key-event="keydown"
     :key-code="9"
     :modifiers="['shiftKey']"
     @success="incrementSelectedIndex(-1)"
     prevent-default>
</dev-key-press>

I wanted to have tab move forward, and shift tab move backward in a list of items.

The issue was that pressing Shift + Tab was triggering both callbacks.

What's the proper way to handle that? Would you use the object the @success callback receives (see below) to see if the event had modifiers, and if so, use that in your callback logic?

{
  event: Object, // the official event object
  expectedEvent: Object, // your defined props.
  message: String // A declarative message on error/success.
}

Personally, for now, I just changed the vue-keypress code to this:

if (hasModifiers) {
  const modifiersPressed = supportedModifiers.every(
    (modifier) => event[modifier] == (expectedInput.modifiers.indexOf(modifier) !== -1)
  )
  if (!modifiersPressed) continue
} else {
  const anyModifierPressed = supportedModifiers.some((modifier) => !!event[modifier])
  if (anyModifierPressed) continue
}

so that if the event has no modifiers, but any modifier is pressed, then the @success callback is not run.

lupas commented 3 years ago

Hey @niuage

The proper way to handle such a use case is by using the multiple-keys option:

<template>
  <Keypress
    key-event="keydown"
    :multiple-keys="keyPressProps"
    @success="handleKeypress"
  />
</template>

<script>
import { defineComponent, defineAsyncComponent } from 'vue'

export default defineComponent({
  components: {
    Keypress: defineAsyncComponent(() => import('vue3-keypress')),
  },
  setup() {
    const keyPressProps = [
      {
        keyCode: 9,
        modifiers: [],
        preventDefault: true,
      },
      {
        keyCode: 9,
        modifiers: ['shiftKey'],
        preventDefault: true,
      },
    ]

    function handleKeypress(response) {
      const shiftKeyPressed = response.event.shiftKey

      if (shiftKeyPressed) {
        // move backwards
      } else {
        // move forward
      }
    }

    return {
      handleKeypress,
      keyPressProps,
    }
  },
})
</script>

Let me know if that would not work.

niuage commented 3 years ago

@lupas Ok thx.

But if

<dev-key-press
    key-event="keydown"
    :key-code="9"
    @success="incrementSelectedIndex(1)"
    prevent-default>
</dev-key-press>

actually already captures both Tab and Shift+Tab, there's no need for the multiple events right? You could just define a Tab keypress and have your logic in the @success callback.

What I don't love about this approach is that the 2 actions could not be related at all. Let's say you're building some Photoshop-like app, and you want Z to be "Zoom", but Ctrl+Z to be "Cancel", it would be kinda ugly to have 1 method handling both, with an if statement.

Also, I can't think of many cases where it's beneficial to have a Z keypress also handle Ctrl+Z (or any modifier) for instance, those are 2 different commands.

Worst case, I feel like it could be an option, "exclusive" or something, which would make the event only react to the exact keypress, and not to a super-set of the keys defined. But personally, I'd make that the default. If users want to react to both Z and Ctrl+Z with the same action, then they can use multiple-keys, it's already supported, so I do think a Z Keypress should only react to Z, with no modifiers.

Just my 2 cents :).

lupas commented 3 years ago

@niuage You're right, guess that's a flaw of the module at this point. [Pressing] Shift+9 should actually not trigger success if only "9" without shift as modifier is pressed defined.. Should be easily fixable though.

I would, however, in general advise to not have your @success handler be "incrementSelectedIndex" but simply "handleKeypressEvent". If you happen to need to catch many keypress events, you can do the triage there. You can always call the functions there, where the logic is separated. So the handleKeypressEvent function is simply handling the event and there is no specific logic.

Also since the @success event emits a value (= the response object) I don't think it's good practice to overwrite that value with -1 or 1.

Each component adds an event listener, so I would not advise adding multiple components, that is exactly why we built this logic with the "multiple-events", so we don't end up adding multiple event listeners.

niuage commented 3 years ago

You're right, guess that's a flaw of the module at this point. Shift+9 should actually not trigger success if only "9" without shift as modifier is pressed. Should be easily fixable though.

I think you misunderstood me, it's the opposite: It's the 9 keypress that triggers, even if shift+9 is pressed. What you said above is already working. Shift+9 wont trigger if only 9 is pressed.


Personally, I don't like the idea of having 1 method handle all my events. With that implementation, you need to define all the keys you want to react to, using keycodes, and then in your @success method, you need some kind of switch that again goes over the keycodes and possibly handles modifiers. Feels like it's extra work and bug prone.

What I would like tho is something like this maybe:

<template>
  <Keypress key-event="keyup" :multiple-keys="keyBinds" />
</template>

<script>
export default {
  components: {
    Keypress: defineAsyncComponent(() => import('vue3-keypress')),
  },
  data: () => ({
    keyBinds: [
        {
          keyCode: 65, // A
          modifiers: ['shiftKey'],
          success: handleAPressed,
          preventDefault: true,
        },
        {
          keyCode: 83, // S
          modifiers: ['shiftKey'],
          success: handleBPressed,
          preventDefault: false,
        },
      ],
  }),
}
</script>

This way you can define the method that should handle a given key right when you define that key bind.


Also since the @success event emits a value (= the response object) I don't think it's good practice to overwrite that value with -1 or 1.

You're probably right, in my case, if I wanted to continue using multiple components, then it'd be better to have 2 methods, increment and decrement and not passing the 1 and -1 I guess.

lupas commented 3 years ago

I think you misunderstood me, it's the opposite: It's the 9 keypress that triggers, even if shift+9 is pressed. What you said above is already working. Shift+9 wont trigger if only 9 is pressed.

@niuage Was low on coffee this morning, I meant the same as you but mixed it up in my answer ;) I meant to say "[Pressing] Shift+9 should actually not trigger success if only "9" without shift as modifier is pressed defined."

I agree with you. While having one method to handle all events is better than having to define the components multiple times (imho), having to do the conditionals again later when already defined in the multiple-events array is redundant.

Your proposed solution would clearly be the best approach.

I was anyway thinking that, now with Vue 3, this entire module could be moved from a component-based solution to a pure JavaScript module that can be implemented within the setup function somehow like this:

import useKeypress from 'vue3-keypress'

export default {
  setup() {
    const keyPressConfig = {
      keyEvent: 'keyup',
      keyBinds: [
        {
          keyCode: 65, // A
          modifiers: ['shiftKey'],
          success: handleAPressed,
          preventDefault: true,
        },
        {
          keyCode: 83, // S
          modifiers: ['shiftKey'],
          success: handleBPressed,
          preventDefault: false,
        },
      ],
    }

    useKeypress(keyPressConfig)

    return { }
  },
}

Happy to look into it at some point - feel free to start without me to improve that logic if you have time on my hands, appreciate any help :)

lupas commented 3 years ago

@niuage I fixed the modifier issue mentioned above in v3.0.2.

For the refactoring I created a new issue to separate concerns: https://github.com/lupas/vue3-keypress/issues/3

niuage commented 3 years ago

Sweet, thx.

Btw, i dont think there's a need for a new version for that, but this comment has some issues:

// Check if that no modifiers where pressed, otherwise it's not a match

It should be something like

Check that no modifiers were pressed, otherwise it's not a match

lupas commented 3 years ago

@niuage Thx, fixed these. https://github.com/lupas/vue3-keypress/commit/17b761ec7a3a2701f30e999ae99b3dcacf828d92