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

Software Keymaps are not correctly interpreted / listened to. #88

Merged

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Sep 1, 2018

This PR depends on #87
Diff vs #87

Comparing your current branch, key-combo-tests, to upgrade-dependencies
 addon-test-support/key-event.js                             | 17 +++++++++---
 addon-test-support/test-helpers.js                          |  4 +--
 addon/services/keyboard.js                                  |  5 ++++
 addon/utils/get-code.js                                     | 30 ++++++++++++++++++++-
 tests/acceptance/keyboard-combo-test.js                     | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/dummy/app/components/keyboard-press.js                | 25 ++++++++++++++++++
 tests/dummy/app/controllers/.gitkeep                        |  0
 tests/dummy/app/controllers/test-scenario/keyboard-combo.js | 19 +++++++++++++
 tests/dummy/app/router.js                                   |  1 +
 tests/dummy/app/templates/test-scenario/keyboard-combo.hbs  | 23 ++++++++++++++++
 10 files changed, 184 insertions(+), 6 deletions(-)

I think this'll resove #84, and superscede #85 as well as account for any other keyboard layout.

The gist is that on different keyboard layouts (esp those that aren't hardware enforced (like tho OS-language/region / keyboard layout settings), the KeyboardEvents code` was reporting the hardware key.

Here is a test that demonstrates the event

      test('keydown: Ctrl+k on a Dvorak Keyboard', async function(assert) {
        const ctrlKDownProperties = {
          code: 'KeyV',
          key: 'k',
          keyCode: 75,
          which: 75,
          ctrlKey: true
        };

        await textChanged(
          assert,
          () => triggerEvent(document.body, 'keydown', ctrlKDownProperties), {
            selectorName: 'ctrl-k',
            beforeValue: 'Ctrl+K not pressed',
            afterValue: 'Ctrl+K pressed'
          });
      });

This PR depends on #87, but the main fix is this:

import codeMap from 'ember-keyboard/fixtures/code-map';

export default function getCode(event) {
  const { code, key, keyCode } = event;

  // Note that keyCode is deprecated
  // https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode
  if (!code) {
    return codeMap[keyCode];
  }

  if (!code.includes('Key') || !key) {
    return code;
  }

  // If we have a software-applied key-remapping
  // For example:
  //   in Dvorak:
  //     pressing 'k'
  //       will give a code of 'KeyV'
  //       and a key of 'k'
  const codeLetter = code.charAt(code.length - 1);
  const keyboardLetter = codeLetter.toLowerCase();
  const typedLetter = key.toLowerCase();

  if (typedLetter === keyboardLetter) {
    return code;
  }

  const newCode = `Key${typedLetter.toUpperCase()}`;

  return newCode;
}

@NullVoxPopuli NullVoxPopuli changed the title Failing test: Ctrl+K does not get captured. Software Keymaps are not correctly interpreted / listened to. Sep 1, 2018
…indings. Fix an issue with software key-remappings. Fixes adopted-ember-addons#84, superscedes adopted-ember-addons#85

inevitably revert to keyCode, because of characters other than letters
// event.preventDefault();
// event.stopPropagation();
// }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little dead code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about this; you've done enough. I'll fix it during the release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a ton!!!

@briarsweetbriar briarsweetbriar merged commit bba28a2 into adopted-ember-addons:master Sep 5, 2018
@briarsweetbriar
Copy link
Collaborator

My only concern about this approach is that some devs may want strict binding to a key, rather than to a code. For instance, if the dev wanted WASD to navigate through the app, regardless of key mappings.

That being said, this seems like a minority use case, and perhaps there's nobody actually using ember-keyboard this way. For the time being, this warrants a major version change, and if anybody needs binding to a key, they'll still have 3.0.2 until we figure out a solution.

Either way, thanks for all your work on this! You've leveled up ember-keyboard in a pretty big way.

@NullVoxPopuli
Copy link
Contributor Author

My only concern about this approach is that some devs may want strict binding to a key, rather than to a code. For instance, if the dev wanted WASD to navigate through the app, regardless of key mappings.

yeah, I thought about that, too. Like, Blizzard games have their key-bindings by physical code from the keyboard, whereas slack, discord, vim, etc are all software key codes.

So... not sure how to support both styles. :-\

You've leveled up ember-keyboard in a pretty big way.

<3

@aabzhanova
Copy link

Hello friends!! @NullVoxPopuli @patience-tema-baron
This update broke my keyDown event 'KeyK+ctrl+alt' - it still works when I try
'KeyK+ctrl' but any variation with 'alt' is not working. I'm on a mac/chrome

@NullVoxPopuli
Copy link
Contributor Author

@aabzhanova do you have a reproduction?

@aabzhanova
Copy link

aabzhanova commented Jul 29, 2019

@NullVoxPopuli:
this doesn't fire

on(keyPress('KeyK+ctrl+alt'), function() {
      //some action
    })

this fires:

on(keyPress('KeyK+ctrl'), function() {
      //some action
    })

anything with alt doesn't fire

@NullVoxPopuli
Copy link
Contributor Author

I mean, can you make a reproduction here: https://codesandbox.io/s/github/mike-north/ember-new-output (or in a standalone github repo)

it would help us out a ton <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants