Don't use KeyboardEvent.code when resolving user keystrokes

[Ben3eeE]

Description

It would be great if we could remove resolving keystrokes using KeyboardEvent.code because the code is the physical position of the key which doesn't change when a user remaps keys for example by using xmodmap. There is also a report where KeyboardEvent.code is incorrect but KeyboardEvent.key is correct. Likely because of a bug in Chrome.

MDN says this about KeyboardEvent.code:

The KeyboardEvent.code property represents a physical key on the keyboard (as opposed to the character generated by pressing the key). In other words, this property returns a value which isn't altered by keyboard layout or the state of the modifier keys.
This property is useful when you want to handle keys based on their physical positions on the input device rather than the characters associated with those keys

Currently all reports where this is a problem are for Backspace:

• There is a regression in Atom 1.12.0-beta2 when we started resolving backspace using KeyboardEvent.code that makes the E key resolve to backspace when using x2go or XQuartz. This is likely due to a bug in Chrome.

• It is quite common for users of the colemak layout to change positions of Tab and Backspace using for example xmodmap. This causes us to resolve both Tab and Backspace to Backspace. This is also a 1.12.0-beta2 regression.

Resolving using KeyboardEvent.key would fix both of these issues without the need for users to work around this using the public keystroke resolver API

Where

atom-keymap has two places where keystrokes are resolved using KeyboardEvent.code.

In KEY_NAMES_BY_KEYBOARD_EVENT_CODE

atom-keymap/src/helpers.coffee

Lines 7 to 10 in 3b17ede

	KEY_NAMES_BY_KEYBOARD_EVENT_CODE = {
	'Space': 'space',
	'Backspace': 'backspace'
	}

Backspace was added to work around a bug with ctrl-backspace on Windows.

Space was added because KeyboardEvent.key is a space character which we don't want to resolve to.

In NUMPAD_KEY_NAMES_BY_KEYBOARD_EVENT_CODE

atom-keymap/src/helpers.coffee

Lines 19 to 30 in 3b17ede

	NUMPAD_KEY_NAMES_BY_KEYBOARD_EVENT_CODE = {
	'Numpad0': 'numpad0',
	'Numpad1': 'numpad1',
	'Numpad2': 'numpad2',
	'Numpad3': 'numpad3',
	'Numpad4': 'numpad4',
	'Numpad5': 'numpad5',
	'Numpad6': 'numpad6',
	'Numpad7': 'numpad7',
	'Numpad8': 'numpad8',
	'Numpad9': 'numpad9'
	}

This was added to resolve the numpad digits differently to avoid changing tabs when trying to enter unicode characters using alt-numpad entry on Windows.

Proposal

Getting rid of KEY_NAMES_BY_KEYBOARD_EVENT_CODE

1. Investigate resolving Space by checking if KeyboardEvent.key is a space character instead of checking if KeyboardEvent.code is Space. It could maybe be added to NON_CHARACTER_KEY_NAMES_BY_KEYBOARD_EVENT_KEY.

atom-keymap/src/helpers.coffee

Lines 11 to 18 in 3b17ede

	NON_CHARACTER_KEY_NAMES_BY_KEYBOARD_EVENT_KEY = {
	'Control': 'ctrl',
	'Meta': 'cmd',
	'ArrowDown': 'down',
	'ArrowUp': 'up',
	'ArrowLeft': 'left',
	'ArrowRight': 'right'
	}

2. Investigate if the Backspace workaround is still needed. Chrome has since fixed a bug where it didn't mask off the event code correctly when Num Lock or Caps Locks was on. I wonder if there are similar fixes that have reached us so we can remove this safely. I have tested this and it looks like the workaround is no longer needed.

Getting rid of NUMPAD_KEY_NAMES_BY_KEYBOARD_EVENT_CODE

Instead of checking if KeyboardEvent.code is Numpad# we could check if KeyboardEvent.key is a digit and use KeyboardEvent.location to identify if the digit comes from the numpad. KeyboardEvent.location is 3 for the numpad.

This would require checking it the location changes accordingly when remapping the numpad.

[nathansobo]

@Ben3eeE Do you want to give this a try? All the nuances of keyboard handling are hard to keep loaded in my brain after a year, but I'm open to reviewing a PR.

[Ben3eeE]

@nathansobo Yeah I started with this but then got sidetracked. I think I will open a series of PRs each removing one instance of resolving using KeyboardEvent.code.

I opened #228 now. I've had that branch around for a while. Not much to review there since it's more testing that needs to be done.

[ghost]

@Ben3eeE I see you duped issue #16350. I'm trying to figure out the correct code to add to my init.coffee to fix that closed/duped issue. When I press key the keybinding resolver tells me resolves to 'space'. What then would be the correct addKeystrokeResolver function to fix that issue? Or will this bug here be fixed anytime soon? Thanks for your time.

[Ben3eeE]

@ncsucharlie Please see my reply on discuss where I helped another user https://discuss.atom.io/t/e-key-acting-as-backspace/37075/15?u=ben3eee If you still have questions I will be happy to help out in that topic or in a new topic on discuss. Feel free to @mention me there 🙂

Edit: I have no idea when this will be fixed. I can't test out the PR I opened as a start for a fix because my keyboards scroll lock key is broken and testing would require you to be able to toggle scroll lock.

[Arcanemagus]

Some likely helpful information for when this can eventually be fixed properly in a far future Electron version:

• Keyboard Map API

It looks like this is being released in Chrome 69.

[Ben3eeE]

Wow thanks for sharing @Arcanemagus this sounds interesting