atom/atom-keymap

Multiple modifiers are not recognized correctly with non-latin characters due to uppercase checking

issacgerges opened this issue · 8 comments

issacgerges commented

Edit by @rsese to mention Jason's repro details

I see this issue using Atom 1.36.0-nightly11 on macOS 10.14.2.

Steps to reproduce

I can reproduce it as follows:

  1. Open two tabs in Atom
  2. Turn on the keybinding resolver so you can see how keybindings are being interpreted. In the command palette, run Key Binding Resolver: Toggle.
  3. Hit command+shift+[ to move to the previous tab (i.e., previous pane item)

Expected behavior: In the Key Binding Resolver, you should see that the keystroke resolves to command+shift+[ and Atom should focus the previous tab (i.e., previous pane item).

Actual behavior: In the Key Binding Resolver, you see that the keystroke incorrectly resolves to command+[, which triggers the Editor: Outdent Selected Rows command.

Demo

In the gif below, I have Mouseposé enabled to show which keys are being pressed. We can see Mouseposé showing command+shift+[ and command+shift+] being pressed, and we can see Atom interpreting them as command+[ and command+]. (See Atom's Key Binding Resolver output in the bottom left corner of the Atom window.)

demo

Prerequisites

Description

When using multiple modifiers, for example Command+Shift+], atom-keymap will only recognize cmd+]. In these cases keymaps including keycodes for the latter will be caught by the former.

Steps to Reproduce

  1. Register a keymap for keycode cmd+[
  2. Press Command+Shift+[

Expected behavior: The supplied event isn't dispatched

Actual behavior: The supplied event is dispatched

Reproduces how often: Consistently

Additional Information

The issue seems to be caused by a check here:

atom-keymap/src/helpers.coffee

Line 248 in 1cb3a7a

if key is 'shift' or (shiftKey and event.type isnt 'keyup' and (isNonCharacterKey or (isLatinCharacter(key) and isUpperCaseCharacter(key))))

Specifically isUpperCaseCharacter rules out the keystroke since '[' is equal to '['. toLowerCase(). It may be worth including a check for isNonCased that explicitly checks if key.toUpperCase() === key. toLowerCase()

issacgerges commented

It's also incredibly likely this was never hit in the past since Chromium used to report these keys as their shifted version when shit+command were pressed. This recently changed, although I can't find the Chromium ticket. Now, these are reported as their unsifted versions if both command and shift are pressed. That Chromium change hit Electron in 3.x

rsese commented

Thanks for the report @issacgerges 🙇 I might be misunderstanding, but I wasn't able to reproduce on 1.34.0 with macOS 10.12.6. I added this to my keymap:

'body':
 'cmd-{': 'unset!'

'atom-text-editor':
 'cmd-[': 'core:move-down'

I added that first one because otherwise in step 2 from your repro steps, Atom will show the previous pane on macOS. But now if I follow your step 2, nothing happens.

Also what version of Atom are you running (atom -v)?

VerteDinde commented

Hey @rsese! I actually think I'm seeing this too - we just upgraded to Electron 3 with Chromium 66, and I noticed that our shortcut for cmd+plus now only works if I type in cmd+shift+plus. Changing the shortcut to cmd+= works.

I'm running atom-keymap: 8.2.12. Happy to give more info if needed.

rsese commented

@VerteDinde - are running Atom on Arch Linux? We're not on Electron 3 yet, that's being worked on in atom/atom#18603 (we can mention this issue there).

no-response commented

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

rsese commented

@issacgerges @VerteDinde - the Electron 3 upgrade for Atom will be in the upcoming 1.36 beta release. If you can reproduce there can you let us know if you can reproduce the problem there?

  • 🎉1
rsese commented

One of our maintainers is seeing this on our nightly release which is on Electron 3, going to reopen. Thanks again for the heads up @issacgerges @VerteDinde ✌️

jasonrudolph commented

I see this issue using Atom 1.36.0-nightly11 on macOS 10.14.2.

Steps to reproduce

I can reproduce it as follows:

  1. Open two tabs in Atom
  2. Turn on the keybinding resolver so you can see how keybindings are being interpreted. In the command palette, run Key Binding Resolver: Toggle.
  3. Hit command+shift+[ to move to the previous tab (i.e., previous pane item)

Expected behavior: In the Key Binding Resolver, you should see that the keystroke resolves to command+shift+[ and Atom should focus the previous tab (i.e., previous pane item).

Actual behavior: In the Key Binding Resolver, you see that the keystroke incorrectly resolves to command+[, which triggers the Editor: Outdent Selected Rows command.

Demo

In the gif below, I have Mouseposé enabled to show which keys are being pressed. We can see Mouseposé showing command+shift+[ and command+shift+] being pressed, and we can see Atom interpreting them as command+[ and command+]. (See Atom's Key Binding Resolver output in the bottom left corner of the Atom window.)

demo

  • 👍1