From 701b0d3b64e3431daea33facf52cc0ca44ef0f16 Mon Sep 17 00:00:00 2001
From: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com>
Date: Fri, 20 Sep 2019 11:06:57 -0400
Subject: [PATCH] use key rather than keyCode for comma (#10902)

---
 .../src/metabase/components/TokenField.jsx    | 10 +++++---
 frontend/src/metabase/lib/keyboard.js         |  2 +-
 .../components/TokenField.unit.spec.js        | 25 ++++++++++++++++---
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/frontend/src/metabase/components/TokenField.jsx b/frontend/src/metabase/components/TokenField.jsx
index 0490750a3e3..d4122b0453c 100644
--- a/frontend/src/metabase/components/TokenField.jsx
+++ b/frontend/src/metabase/components/TokenField.jsx
@@ -13,11 +13,11 @@ import Popover from "metabase/components/Popover";
 import {
   KEYCODE_ESCAPE,
   KEYCODE_ENTER,
-  KEYCODE_COMMA,
   KEYCODE_TAB,
   KEYCODE_UP,
   KEYCODE_DOWN,
   KEYCODE_BACKSPACE,
+  KEY_COMMA,
 } from "metabase/lib/keyboard";
 import { isObscured } from "metabase/lib/dom";
 
@@ -266,7 +266,7 @@ export default class TokenField extends Component {
       this.props.onInputKeyDown(event);
     }
 
-    const keyCode = event.keyCode;
+    const { key, keyCode } = event;
 
     const { filteredOptions, selectedOptionValue } = this.state;
 
@@ -274,7 +274,11 @@ export default class TokenField extends Component {
     if (
       keyCode === KEYCODE_ESCAPE ||
       keyCode === KEYCODE_TAB ||
-      keyCode === KEYCODE_COMMA ||
+      // We check event.key for comma presses because some keyboard layouts
+      // (e.g. Russian) have a letter on that key and require a modifier to type
+      // ",". Similarly, if you want to type "<" on the US keyboard layout, you
+      // need to look at `key` to distinguish it from ",".
+      key === KEY_COMMA ||
       keyCode === KEYCODE_ENTER
     ) {
       if (this.addSelectedOption(event)) {
diff --git a/frontend/src/metabase/lib/keyboard.js b/frontend/src/metabase/lib/keyboard.js
index 73fb4250460..d5697baca4d 100644
--- a/frontend/src/metabase/lib/keyboard.js
+++ b/frontend/src/metabase/lib/keyboard.js
@@ -8,5 +8,5 @@ export const KEYCODE_UP = 38;
 export const KEYCODE_RIGHT = 39;
 export const KEYCODE_DOWN = 40;
 
-export const KEYCODE_COMMA = 188;
+export const KEY_COMMA = ",";
 export const KEYCODE_FORWARD_SLASH = 191;
diff --git a/frontend/test/metabase/components/TokenField.unit.spec.js b/frontend/test/metabase/components/TokenField.unit.spec.js
index 838a0a3b972..3dfc013ddb3 100644
--- a/frontend/test/metabase/components/TokenField.unit.spec.js
+++ b/frontend/test/metabase/components/TokenField.unit.spec.js
@@ -11,7 +11,7 @@ import {
   KEYCODE_DOWN,
   KEYCODE_TAB,
   KEYCODE_ENTER,
-  KEYCODE_COMMA,
+  KEY_COMMA,
 } from "metabase/lib/keyboard";
 
 const DEFAULT_OPTIONS = ["Doohickey", "Gadget", "Gizmo", "Widget"];
@@ -168,6 +168,19 @@ describe("TokenField", () => {
     expect(value()).toEqual(["bar"]);
   });
 
+  it("should type a character that's on the comma key", () => {
+    component = mount(
+      <TokenFieldWithStateAndDefaults value={[]} options={["fooбar"]} />,
+    );
+
+    focus();
+    type("foo");
+    // 188 is comma on most layouts
+    input().simulate("keydown", { keyCode: 188, key: "б" });
+    // if that keydown was interpreted as a comma, the value would be "fooбar"
+    expect(input().props().value).toEqual("foo");
+  });
+
   describe("when updateOnInputChange is provided", () => {
     beforeEach(() => {
       component = mount(
@@ -340,8 +353,12 @@ describe("TokenField", () => {
   });
 
   describe("key selection", () => {
-    [KEYCODE_TAB, KEYCODE_ENTER, KEYCODE_COMMA].map(key =>
-      it(`should allow the user to use arrow keys and then ${key} to select a recipient`, () => {
+    [
+      ["keyCode", KEYCODE_TAB],
+      ["keyCode", KEYCODE_ENTER],
+      ["key", KEY_COMMA],
+    ].map(([keyType, keyValue]) =>
+      it(`should allow the user to use arrow keys and then ${keyType}: ${keyValue} to select a recipient`, () => {
         const spy = jest.fn();
 
         component = mount(
@@ -367,7 +384,7 @@ describe("TokenField", () => {
         expect(component.state().selectedOptionValue).toBe(DEFAULT_OPTIONS[2]);
 
         input().simulate("keydown", {
-          keyCode: key,
+          [keyType]: keyValue,
           preventDefalut: jest.fn(),
         });
 
-- 
GitLab