Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
issacgerges opened this issue Dec 21, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@issacgerges
Copy link

commented Dec 21, 2018

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:

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

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

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

This comment has been minimized.

Copy link

commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

@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

This comment has been minimized.

Copy link

commented Feb 6, 2019

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.

@no-response no-response bot closed this Feb 6, 2019

@rsese

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

@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?

@rsese

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

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 ✌️

@rsese rsese reopened this Feb 6, 2019

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

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

jasonrudolph added a commit to jasonrudolph/dotfiles that referenced this issue Feb 7, 2019

@daviwil daviwil closed this in #238 Feb 18, 2019

jasonrudolph added a commit to jasonrudolph/dotfiles that referenced this issue Mar 6, 2019

Revert ":atom: Stick with Atom beta until atom/atom-keymap#237 is resolved"
atom/atom-keymap#237 is resolved, so let's hop back on the atom-nightly
train!

This reverts commit 9fc14c8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.