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

Caps lock effects keyboard shortcuts #110

Closed
csagonas opened this issue Oct 13, 2015 · 7 comments
Closed

Caps lock effects keyboard shortcuts #110

csagonas opened this issue Oct 13, 2015 · 7 comments
Assignees
Labels

Comments

@csagonas
Copy link

Notes by @jabooth:

We care about the capitalisation of keys when looking for all keyboard shortcuts, which is a pain when caps lock is depressed. I think most software actually doesn't care about the caps state of the key, as they manually check for if shift is depressed if needed in the shortcut for instance.

The answer here is probs to wrap all our keyboard shortcuts and make them case insensitive - in case we can think of any downside to that?

@jabooth jabooth added the bug label Oct 17, 2015
@lirsacc
Copy link
Contributor

lirsacc commented Oct 17, 2015

We are using both keypress and keydown events. The keypress event will give us characters (a <> A), while keydown gives keys (A == a). A quick solution would be to stop using keypress and use the key codes. I'll try and push something when I have time to test all shortcuts and keys.

EDIT: branch https://github.com/menpo/landmarker.io/tree/keyboard-issues uses lowercase characters and seem to work, will do more testing later on but it looks like it fixes the problem

@lirsacc lirsacc self-assigned this Oct 17, 2015
@jabooth
Copy link
Member

jabooth commented Oct 18, 2015

Direction sounds promising, just took a look at https://www.landmarker.io/staging/keyboard-issues
Now 'shift + a is just interpreted as 'a', which is different behaviour - somehow we still need to make sure we respect the modifier keys being pressed or not pressed?

@lirsacc
Copy link
Contributor

lirsacc commented Oct 19, 2015

Well we can always catch the shift key in https://github.com/menpo/landmarker.io/blob/keyboard-issues/src/js/app/view/keyboard.js#L45 for different behaviours on the same key. However I took the quick route and made it so a and A have the same behaviour (hence shift+a as well).

Now I don't see any behaviour change on shift+a but do you mean it should remain as is and shift should somehow block the keyboard shortcut from being triggered ? I guess this makes sense for a and other selection keys (g, d, q) but what about z and y ? Should we keep undo available while doing a mouse selection ? To extend the consideration, what about group mode ? Should we keep the selection shortcuts available when in groupe mode ? Currently if you select a few points and then push a, you'll change the selection, which I guess doesn't really make sense given the direction of the group mode...

@jabooth
Copy link
Member

jabooth commented Oct 20, 2015

Now I don't see any behaviour change on shift+a but do you mean it should remain as is and shift should somehow block the keyboard shortcut from being triggered ?

I think shift + A and A (which is truly {A/a}) are semantically different - the addition of the shift key changes the shortcut in my mind.

If the keyboard shortcut is a single key, it should only work when the single key is pressed without any modifier.

but what about z and y ? Should we keep undo available while doing a mouse selection ?

Not sure I follow...do you mean the user has depressed shift to select many points, and whilst they are still holding down shift they go to hit z to undo?

If so, I think this shouldn't work. The user should let go of the shift key and then hit z to undo.

To extend the consideration, what about group mode ? Should we keep the selection shortcuts available when in groupe mode ?

This is a very good question - should the keyboard shortcuts be in a sense global or context sensitive?

My gut feeling is wherever possible they should have a single global meaning. If a is select all, it should always select all. Like we currently have, group completion is a separate key, g.

Context aware keyboard shortcuts are great if there is a consistent set of contexts and the user knows the rules (I'm thinking Vim here) but for our purposes I think it adds confusion. We've got enough single letter keys to go at, so we have the luxury of spending a and g on selection.

Currently if you select a few points and then push a, you'll change the selection, which I guess doesn't really make sense given the direction of the group mode...

I don't know if that would be such a concern? Sure, you're going to alter the group to be all the points, but I don't see why that is an issue?

@lirsacc
Copy link
Contributor

lirsacc commented Oct 20, 2015

If the keyboard shortcut is a single key, it should only work when the single key is pressed without any modifier.

So we're on the same page here. The thing is that making it work for A in with caps lock (original bug report) on means we now have to catch the shift key and disable shortcuts for that case as shift + a and A are the same. Is the original problem such an issue that it justifies the added complexity ?

Not sure I follow...do you mean the user has depressed shift to select many points, and whilst they are still holding down shift they go to hit z to undo?

Exactly, the commit made it possible (try shift dragging and hitting z), but previous point cancels this one.

For the group mode, I am thinking down the line with scaling and rotation, where using shortcuts may affect the covered area, mostly by selecting point outside of the original selection area or shrinking the area by removing border points. What's the truth here: the boundary of selected points or the area I specifically selected when dragging ?

@lirsacc
Copy link
Contributor

lirsacc commented Oct 26, 2015

Ok, I have changed the way shortcuts are declared to avoid checking for shift all over. You now have a dictionary of shortcuts (tuples) for the keypress event indexed by the actual symbol (simple letters, no key combos as those go in keydown for greater flexibility).

All existing shortcuts work on my test and the basic ones (a, g, j, k, ...) work as expected (caps lock on and off) and don't work with shift pressed.

@jabooth
Copy link
Member

jabooth commented Oct 26, 2015

that sounds great. Is this all in the same group of changes as the ctrl+S stuff as well? if so happy to try it out and merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants