omridevk / ng-keyboard-shortcuts

Dead Simple Keyboard Shortcuts Management for Angular
https://ng-keyboard-shortcuts.vercel.app/
147 stars 34 forks source link

A shortcut works either in INPUT or outside of it, but never for both cases #5

Closed pawelsalawa closed 6 years ago

pawelsalawa commented 6 years ago

Describe the bug Current implementation of "isAllowed" function prevents single shortcut to work globally. What I mean is that if you define global shortcut for top-level component, it works for all contents of the page, except of INPUT/TEXTAREA/SELECT, which is explained in docs - we need explicit config option to be set to make it work. Problem is that if I provide all 3 input types in "allowIn" array, then this shortcut does not work anymore outside of these inputs.

        if (shortcut.allowIn.length) {
            return shortcut.allowIn.includes(target.nodeName);
        }
        return !this._ignored.includes(target.nodeName);

The problem with this condition is that when anyone defines ANY content of allowIn array, then only these become valid. It's no longer allowed to handle that shortcut in any other node types.

To Reproduce

*.ts:

   this.keyboard.add([
         {
             key: 'alt enter',
             //allowIn: ['TEXTAREA', 'INPUT', 'SELECT'],
             command: () => console.log('xxx ')
         }
    ]);

In html just have few non-input fields and some input fields. Depending on commented/uncommented line of allowIn, only input fields react to the input, or in other case all except the inputs.

Expected behavior The allowIn option should complement the default list of allowed elements, not overwrite it and limit to just these few. For limiting scope we already have the target property.

Desktop (please complete the following information):

omridevk commented 6 years ago

@pawelsalawa Thanks a lot for reporting this issue, hopefully i'll be able to look into it this week, either way, please feel free to submit a PR. Either way, thanks for the feedback :)

omridevk commented 6 years ago

Seems like a trivial fix, but might break previous behavior, i'll look into it and provide an update soon enough.

omridevk commented 6 years ago

And just to make sure I understand the behavior you expect:

this.keyboard.add([
         {
             key: 'alt enter',
             allowIn: ['TEXTAREA', 'INPUT', 'SELECT'],
             command: () => console.log('xxx ')
         }
    ]);

You want the above shortcut, to work on all elements, including the "ignored" one, i.e TEXTAREA, INPUT and SELECT elements?

omridevk commented 6 years ago

If so, it makes total sense, and need to be changed. again, thanks for the input.

pawelsalawa commented 6 years ago

Currently, If you don't specify "allowIn", the shortcut will work everywhere, except for inputs (such as textarea, input and select). If I specify the "allowsIn", my intention would be that I add speficied inputs to the list of allowed elements.

If I define allowedIn, I overwrite "all" elements with only these specified explicitly in the property. This prevents me from having global shortcut, because it works either for whole page, except inputs, or in other case - for the specified inputs only.

My current workaround is that I printed all nodeNames that I use on my page and I added them to the allowIn property, but I don't think this is elegant solution, because new nodeName may appear some day and it would not respond to the key shortcut.

Functionality of allowIn should not apply additional limitations, but rather relax the limitation. For adding more limitation we have "target" property. In my hamble opinion, of course.

omridevk commented 6 years ago

I agree with you, so basically, the allowIn will provide an escape hatch (it was suppose to, it was an error in my code): currently blacklisted nodes are: ["INPUT", "TEXTAREA", "SELECT"]

so if you set:

{
             key: 'alt enter',
             allowIn: ['INPUT', 'SELECT'],
             command: () => console.log('xxx ')
}

The command will trigger everywhere, except in TEXTAREA, makes sense?

omridevk commented 6 years ago

Please confirm, I will make a fix and will bump major version, as this is a breaking change.

omridevk commented 6 years ago

Fixed in https://github.com/omridevk/ng-keyboard-shortcuts/pull/6

omridevk commented 6 years ago

@pawelsalawa Released version 6.1.0 Please verify that the fix works on your end.

Thank you for submitting the bug.

pawelsalawa commented 6 years ago

Sorry for rather delayed response, but I wasn't around. Yes, the idea was exactly that. I will test it in few moments and confirm here.

pawelsalawa commented 6 years ago

Works as expected. Thanks a lot for your timely reaction!

omridevk commented 6 years ago

No problem :) happy to help, have a wonderful weekend friend.