Can not use keybindings while in non-Latin keyboard layout

[skfd]

iD has a typical minor issue -- it requires to switch to English layout to do hotkeying.

Want to create issue before diving into making a pull request.

We're bind to character input. But we should bind to keyboard key pressed.

I understand that we do it to support lower-case upper-case letters synonyms. But we should be checking shift key instead.

Could you please suggest what test module should I add test for keybindings i18n?

How to test

• Turn on Cyrillic or any other non-Latin layout
• press A (which is Ф for Cyrillic layout)

Expected

• help pane is open

Actual

• Nothing happened

Browsers

Chrome 63, E D G E 4 1

iD version

latest master commit

[bhousel]

We're bind to character input. But we should bind to keyboard key pressed.

It looks like code exists in d3.keybinding.js to do both. We recently switched to KeyboardEvent.key because all international keyboards do not put their keys in the same places, making keyCode detection (keyboard key pressed aka "scan code"?) inconsistent.

I actually think all of our targeted browsers support KeyboardEvent.key now, and that keyCode stuff might just be legacy in our code for PhantomJS or something.

cc @tyrasd - can you advise on this? My gut is that what @skfd is asking for is impossible, but I don't know really.

[skfd]

We recently switched to KeyboardEvent.key because all international keyboards do not put their keys in the same places

Here is the commit, but it has no tests or test cases to go through manually. Will try to come up with something.

Will update this issue as I investigate\prototype.

Generally, I would say iD keybinding should behave the same way the other apps do. I know that Microsoft Word or Gmail work the way I expect in my case. But will have to research the case described above.

[tyrasd]

Hmm, tricky issue.

I believe #3572 is not really too relevant for this case, because it just tackled the issue of inconsistent behavior when working different layouts of all latin keyboards (if the help says you should press the x key to achieve something, it should be the key labeled x independently of where this key is located on the different layouts).

Here we have a situation where a keyboard may not have an actual x labeled key (likely because in the script the character doesn't exist).

My intuition would say that for such situations it might make sense to bind the event behind the x key to additional alternative keys for various scripts (maybe Ч for the cyrillic script – but note that this character could again potentially be located on different physical keys depending on the actual keyboard layout).

[bhousel]

I tested with US and Russian keyboards here: http://unixpapa.com/js/testkey.html

keydown  keyCode=65  (A)   which=65  (A)   charCode=0        
         key=a char=undefined location=0 repeat=false
keypress keyCode=97  (a)   which=97  (a)   charCode=97  (a)  
         key=a char=undefined location=0 repeat=false
keyup    keyCode=65  (A)   which=65  (A)   charCode=0        
         key=a char=undefined location=0 repeat=false
keydown  keyCode=65  (A)   which=65  (A)   charCode=0        
         key=ф char=undefined location=0 repeat=false
keypress keyCode=1092      which=1092      charCode=1092     
         key=ф char=undefined location=0 repeat=false
keyup    keyCode=65  (A)   which=65  (A)   charCode=0        
         key=ф char=undefined location=0 repeat=false

It looks like we maybe can improve this. We're currently binding and matching on keydown. So even though event.key does not match 'a', we have other information we can fallback to.

So, looking at this code:

iD/modules/lib/d3.keybinding.js

Lines 54 to 68 in 8720f7e

	if (event.key !== undefined) {
	if (binding.event.key === undefined) {
	return false;
	} else if (Array.isArray(binding.event.key)) {
	if (binding.event.key.map(function(s) { return s.toLowerCase(); }).indexOf(event.key.toLowerCase()) === -1)
	return false;
	} else {
	if (event.key.toLowerCase() !== binding.event.key.toLowerCase())
	return false;
	}
	} else {
	// check keycodes if browser doesn't support KeyboardEvent.key
	if (event.keyCode !== binding.event.keyCode)
	return false;
	}

Rather than an if/else, we could just retest all the matches a third time using event.keyCode instead of event.key. (I say "third time" because we already test them all twice for unshifted and shifted).

[bhousel]

Here is me testing it.. As with anything related to keyboard detection in browsers, this test applies to Chrome 63 on my Mac and nothing else 😬

[tyrasd]

Rather than an if/else, we could just retest all the matches a third time using event.keyCode instead of event.key.

sounds like a good approach to me. 👍

[bhousel]

I believe I was able to fix this, and it seems to work locally with my pop-up Cyrillic keyboard.

Can you confirm by testing in http://preview.ideditor.com/master ?

I would like to write some unit tests for this feature in d3.keybinding.js but happen doesn't support modern DOM3 events or constructing events with the KeyboardEvent constructor. So we can't currently test the behavior of conditional fallback from KeyboardEvent.key to KeyboardEvent.keyCode.

[skfd]

@bhousel I played around with it and it works indeed. Thank you!
I got stuck on triggering events through happen too. Maybe raw events without the wrapper could be used here? Will try that approach.

[bhousel]

I got stuck on triggering events through happen too. Maybe raw events without the wrapper could be used here? Will try that approach.

For testing iD, we could probably accomplish the same thing with d3.selection#dispatch, which I believe was added to D3 more recently than when @tmcw built happen for this purpose. I'll open an issue.

[skfd]

This was working for some time, but now is broken again. Did not figure out the tests, sorry, just noticed while using.

@bhousel Can we re-open the issue, please? Or it's ok to create new one?