Fix so AltGraph is resolved as a modifier and not as a key

[Ben3eeE]

Description of the Change

In Atom 1.24 pressing AltGraph on Windows sends these two events:

KeyboardEvent {
  isTrusted: true,
  key: "Control",
  code: "ControlLeft",
  location: 1,
  ctrlKey: true
  ...
}

KeyboardEvent {
  isTrusted: true,
  key: "Alt",
  code: "AltRight",
  location: 2,
  ctrlKey: true
  ...
}

This changed in Atom 1.25 due to the Electron upgrade so the Alt key is now AltGraph:

KeyboardEvent {
  isTrusted: true,
  key: "Control",
  code: "ControlLeft",
  location: 1,
  ctrlKey: true
  ...
  }

KeyboardEvent {
  isTrusted: true,
  key: "AltGraph",
  code: "AltRight",
  location: 2,
  ctrlKey: true
  ...
  }

This caused atom-keymap to resolve a single AltGraph as ctrl-alt-altgraph because we receive one event for Control which has altKey and ctrlKey true and one event for AltGraph (which also has altKey and ctrlKey true). Since AltGraph is not a modifier Atom resolved this as a key in the following code:

atom-keymap/src/helpers.coffee

Lines 254 to 256 in 802745d

	unless MODIFIERS.has(key)
	keystroke += '-' if keystroke
	keystroke += key

This also causes issues with partial matching because the AltGraph key cancels pending keystrokes. So a keybinding like:

'atom-text-editor:not([mini])':
  'g $': 'editor:move-to-first-character-of-line'

Would resolve to g altgraph and then $ never matching the entire keybinding.

This Pull Request adds AltGraph as a modifier and a special case for when the key is altgraph to correctly resolve when AltGraph is included in the keystroke.

Alternate Designs

I don't really like the design in this PR but I still went with it! Maybe we can just change the key to alt when it is AltGraph using NON_CHARACTER_KEY_NAMES_BY_KEYBOARD_EVENT_KEY

atom-keymap/src/helpers.coffee

Lines 11 to 18 in 802745d

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

?

In the interest of time I did not investigate any alternative designs so I could get this PR up and at least document the issue.

There are quite a few issues about AltGraph misbehaving on Linux even before the Electron upgrade so there might be another bug on Linux. If Linux has a similar issue for another reason than this there might be an alternative design that also fixes these issues.

atom/atom#16242
t9md/atom-vim-mode-plus#661
#126
t9md/atom-vim-mode-plus#1031

Happy to hear any ideas for alternatives!

Benefits

Fixes a regression from the Electron upgrade.

Possible Drawbacks

Verification Process

I added the following keymap:

'atom-text-editor:not([mini])':
  'g $': 'editor:move-to-first-character-of-line'

Since $ is typed using AltGraph+4 on a Swedish layout this command includes the AltGraph character. The expected result is that we can match the keybinding even when AltGraph is included in the keystroke.

I also tested that the command matches if you press g ctrl-alt-4.

Applicable Issues

These are in an earlier version of Atom. There might be some other bug on Linux for AltGraph.

atom/atom#16242
t9md/atom-vim-mode-plus#661
#126
t9md/atom-vim-mode-plus#1031

[Ben3eeE]

@nathansobo Happy to hear if you have any thoughts about how to solve this 💭 I just went with the first thing I could come up with and it seems to work. Still needs some more testing, especially on Linux. And probably a test for this behavior. Opened a PR early to document the issue and get feedback ⚡️

This is something that we should try to get out in an upcoming beta hotfix release.

[Ben3eeE]

Based on t9md/atom-vim-mode-plus#1031 (comment) Linux has the same issue before the Electron upgrade and is getting a KeyboardEvent.key that is AltGraph so we resolve it as a key.

I don't think this PR will work as is on Linux yet. And I don't know if the alternative design I mentioned to make AltGraph -> Alt with NON_CHARACTER_KEY_NAMES_BY_KEYBOARD_EVENT_KEY will work on Linux because AltGraph is a dedicated key? 💭

This is getting really confusing I think we have had really bad AltGraph resolution on Linux for a while.

[Ben3eeE]

I'm going to revisit this when we are on an Electron version using chromium M69 because it's going to include a lot of fun changes to the keyboard APIs.