menpo / landmarker.io

Image and mesh annotation web application
https://www.landmarker.io
BSD 3-Clause "New" or "Revised" License
114 stars 21 forks source link

Caps lock effects keyboard shortcuts #110

Closed csagonas closed 8 years ago

csagonas commented 8 years ago

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?

lirsacc commented 8 years ago

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

jabooth commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.