Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to use browser shortcuts #81

Open
zannkukai opened this issue Jul 22, 2020 · 22 comments · Fixed by #86 or #88
Open

Unable to use browser shortcuts #81

zannkukai opened this issue Jul 22, 2020 · 22 comments · Fixed by #86 or #88
Assignees
Labels
bug configuration configuration issue

Comments

@zannkukai
Copy link

zannkukai commented Jul 22, 2020

Describe the bug
Hi
I use a shortcut on c key, the linked command is very simple : just navigate on a specific url. The problem is that when I tried to use a copy (Command|CTLR + C), this shortcut is fired and my application navigate to this url, and my clipboard stay empty :'(
Is it a way to use « c » as a shortcut and keep the possibility to keep copy/paste behavior ?

{
    key: ['c'],
    label: 'Global shortcuts',
    description: 'Open my custom page',
    preventDefault: true,
    command: () => this._router.navigate([‘/my_url', ‘page'])
}

It's the same if I defined a shortcut on s or q key. the corresponding "command + S" (save) and "command + q" (quit) are unusable.

@omridevk
Copy link
Owner

@zannkukai
This seems like a bug.
I'll look into this weekend.
i'll try to think of a workaround in the meantime.

@omridevk omridevk self-assigned this Jul 22, 2020
@omridevk
Copy link
Owner

Thank you for reporting the issue 👍

@omridevk omridevk added the bug label Aug 7, 2020
@omridevk
Copy link
Owner

omridevk commented Aug 7, 2020

@zannkukai
Can you please specify what is your desired behavior in this case?
since you put preventDefault on the event, it won't cause copy and paste, can you try and remove the preventDefault? or set it to false?
Thank y oiu
Please re-open if the issue persist.

@omridevk omridevk closed this as completed Aug 7, 2020
@omridevk omridevk added configuration configuration issue and removed bug investigating labels Aug 7, 2020
@zannkukai
Copy link
Author

zannkukai commented Aug 10, 2020

@omridevk
If i set preventDefault:false, I can use the "copy" (my clipboard is filled). Thanks for this trick.
But the main problem is that when I used cmd+c (copy) the shortcut defined on c is still fired.
So in my previous example, I will navigate to '/my_url/page' when I would just copy some text in my clipboard.

PS : I doesn't have any option to re-open this issue

@omridevk omridevk reopened this Aug 10, 2020
@omridevk
Copy link
Owner

sorry wasn't aware that you can't re-open, I re-opened it :)

@omridevk
Copy link
Owner

omridevk commented Aug 10, 2020

@zannkukai
Okay I see your point, and it is a valid one, I'll try to think of a solution for that. Will keep you posted, hopefully I'll get to it rather sooner than later.

@omridevk
Copy link
Owner

fixed in versions:
10.1.13, 9.1.13 and 7.1.13
Let me know if the issue persist.
Thank you again for reporting the bug

@omridevk omridevk added the bug label Aug 10, 2020
@omridevk
Copy link
Owner

@zannkukai
Should be that if you registered a command with one key, if any of modifiers are clicked as well, the event will not fire. only when no modifiers are clicked, the event will fire.
I assume this is the behavior you expected, correct?

@omridevk
Copy link
Owner

omridevk commented Aug 10, 2020

If you enjoy the library, please give it a Star up :) Don't have to of course

@zannkukai
Copy link
Author

@zannkukai
Should be that if you registered a command with one key, if any of modifiers are clicked as well, the event will not fire. only when no modifiers are clicked, the event will fire.
I assume this is the behavior you expected, correct?

That's right 👍 Thanks for your quick feedback and solution.

@omridevk
Copy link
Owner

the fix I made seems to cause a small regression, hopefully i'll fix it soon otherwise I might have to revert until I find a proper fix.
I will try to solve it later on today, if not by this weekend.

@zannkukai
Copy link
Author

You will say i'm a "tricky man" but now I can't use the key associated with the "shift" command :'(
By example, on my french keyboard, to type a '?' character, I need to push on "shift + ," (shift + comma)
image
So now, I can't use the character "?" to open the shortcuts help panel :'(

@omridevk
Copy link
Owner

yes I am aware that's the regression I was referring to.
I will try to solve it today.

@omridevk
Copy link
Owner

Not tricky man :) valid complaints, it was my bad for merging the code without verifying this basic regression test.
I still have not had the time to write some tests to ease modifying the code.
if by EOD I won't have a valid solution I will revert.
In the meantime, if you decide the new issue is problematic, feel free to downgrade to previous version until I release a new one.
Thanks again and sorry for the issues.

@omridevk
Copy link
Owner

@zannkukai
Please let me know if the issue persist and i'll re-open though I did make sure to verify all the various working scenarios and your newly reported bug were working properly.
I'll re-open if it still there.
And thank you for the patience 💯

@zannkukai
Copy link
Author

It's almost perfect. I had still one problem : I use a shortcut ? to open the panel and a shortcut / to place focus on my main search box. As / is the revert of ? (https://github.com/omridevk/ng-keyboard-shortcuts/pull/88/files#diff-8d9297a8b0f0b9d3d25a47313ad05bbbR118). The action performed is the same when I click on both keys. On my belgian azerty keyboard, both keys characters are not present on the same key (see screenshot above).

At this time to solve my problem, I will change the / key to use another one.
But if you still has the strength to work on this very small issue, it will be wonderfull. don't know how to solve this issue without having any keyboard template mapping.

@omridevk omridevk reopened this Aug 17, 2020
@omridevk
Copy link
Owner

Not nit picking at all, you expected it to work and it should. I'll do my best to resolve all these issues, again your feedback is very welcome :)

@omridevk
Copy link
Owner

Forgive me if this might take a bit longer to fix, i am a bit busy in work related stuff in the upcoming week/s

@omridevk
Copy link
Owner

@zannkukai
wish we had only one layout :P
Hopefully i'll find a more general solution as I wrote a big TODO there as I really do not like the complexity added there which is really hard to follow and is very bug pruned.
I'll install your layout and play around with it.
And i'll try to think of a way to test all these keyboards layout automatically, thus reducing regressions like these.

Can you let me know how important this bug affects your current application? I mean can I have some sometime to think of a more generic and bullet-proof solution?
Thank you :)

@zannkukai
Copy link
Author

Not "really" important. Our application is still under development and shortcuts can wait a little bit. For me it's always more interesting to have a generic solution ;-)
Thanks for time you passed on our problem.

@zannkukai
Copy link
Author

Hi @omridevk. Did you have any feedback about this problem ? I will have a planning meeting about our project in a couple of days and this topic will be addressed. Thx

@omridevk
Copy link
Owner

Sorry I am a bit busy due to personal reasons, hopefully I'll get to it soon enough. Again sorry the inconvenience

@omridevk omridevk closed this as completed Jan 7, 2023
@omridevk omridevk reopened this Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment