Deal with caps lock issues on Windows

[Ben3eeE]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions

Description of the Change

When dealing with caps lock issues we do it after dropping modifiers. This causes issues when KeyboardEvent.key is a lower-case letter because of caps lock and shift being depressed during the keystroke because nonAltModifiedKeyForKeyboardEvent returns the upper case letter. Since P !== p the modifiers are dropped.

This PR moves the caps lock issue dealing code up to before we drop modifiers to avoid this issue and adds a simple test for this behavior.

Alternate Designs

Did not consider any alternate designs.

Benefits

Fixes issues with caps lock and shift on Windows.

Possible Drawbacks

I have not linked and tested this in Atom yet so there may be some other issues introduced by this.

Applicable Issues

Fixes #216

/cc: @50Wliu

[nathansobo]

@ungb Could you test this out?

[Ben3eeE]

I will have time to test this next week but I added it to the community triage project to ask for help from @ungb as well ⚡

[ungb]

on windows I can still repro this issue @Ben3eeE

[ungb]

More info:

I mapped:

'body':
  'ctrl-alt-shift-n': 'application:new-file'

without caps log I get the following from keybinding resolver:

with caps lock on:

[ungb]

I've tried the following

cd atom-keymap
npm install
npm link
cd C:\github\atom
npm link atom-keymap
apm rebuild

When I launch atom it still is the same behavior as in #217 (comment) 🤔 .

Also when I try to build atom with script/build I get the following issue

Generating snapshot script at "C:\github\atom\out\startup.js" (66)Error: C:\github\atom\out\app\node_modules\atom-keymap
\node_modules\fs-plus\node_modules\glob\glob.js
Cannot replace with lazy function because the supplied node does not belong to an assignment expression or a variable de
claration!
  at FileRequireTransform.replaceAssignmentOrDeclarationWithLazyFunction (C:\github\atom\script\node_modules\electron-li
nk\lib\file-require-transform.js:180:11)
  at Context.visitIdentifier (C:\github\atom\script\node_modules\electron-link\lib\file-require-transform.js:72:18)
  at Context.invokeVisitorMethod (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:344:49)
  at Visitor.PVp.visitWithoutReset (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:196:32)
  at NodePath.each (C:\github\atom\script\node_modules\ast-types\lib\path.js:101:26)
  at visitChildren (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:219:18)
  at Visitor.PVp.visitWithoutReset (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at NodePath.each (C:\github\atom\script\node_modules\ast-types\lib\path.js:101:26)
  at visitChildren (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:219:18)
  at Visitor.PVp.visitWithoutReset (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at visitChildren (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:246:25)
  at Visitor.PVp.visitWithoutReset (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:204:20)
  at Visitor.PVp.visit (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:133:29)
  at Object.visit (C:\github\atom\script\node_modules\ast-types\lib\path-visitor.js:101:55)
  at FileRequireTransform.replaceDeferredRequiresWithLazyFunctions (C:\github\atom\script\node_modules\electron-link\lib
\file-require-transform.js:68:20)
  at FileRequireTransform.apply (C:\github\atom\script\node_modules\electron-link\lib\file-require-transform.js:25:10)
  at C:\github\atom\script\node_modules\electron-link\lib\generate-snapshot-script.js:52:54
  at undefined.next (native)
  at step (C:\github\atom\script\node_modules\electron-link\lib\generate-snapshot-script.js:3:191)
  at C:\github\atom\script\node_modules\electron-link\lib\generate-snapshot-script.js:3:361

I've also attempted to make sure the package version matches the package.json in atom

[nathansobo]

@ungb you should be able to just build and launch Atom in dev mode. It looks like there's some sort of issue generating the startup snapshot, which doesn't seem like it should be related to this change. Maybe it has something to do with the module being linked?

[winstliu]

Yep, still doesn't work with this PR.

[winstliu]

Never mind about that last comment, I just tried again and now it's working? I think I may have forgotten a step when linking atom-keymap last time.

[Ben3eeE]

@ungb It is working for both me and @50Wliu. Is it ok to merge this and open a PR on atom/atom so you can test from there?

[nathansobo]

Thanks to @Ben3eeE for the fix and @50Wliu for testing it out.

[nathansobo]

Published as v8.2.3 if we wanna get a PR going on atom/atom.

[ungb]

thanks for double checking @50Wliu! I'll give this another go when it's merged and in, unsure why I couldn't get it working.

[Ben3eeE]

@nathansobo Thank you for merging and publishing. I have opened a PR on atom/atom with these changes.

[ungb]

I'm verifying now, I'll merge the pr once I'm done, should be in the next hour or so.

EDIT: sorry for the delay got pulled into something else, I'm building now...