Fix keyboard shortcuts regression introduced in v1.2.0 (#189)

springload · Mar 6, 2019 · b37c8e5 · b37c8e5
1 parent ff5bfa6
commit b37c8e5
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 34 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 > Documentation: [draftail.org/docs/next/getting-started](https://www.draftail.org/docs/next/getting-started)
 
+### Fixed
+
+- Fix regression introduced in v1.2.0 where Draft.js-defined keyboard shortcuts were available unconditionally ([#189](https://github.com/springload/draftail/pull/189)).
+
 ## [[v1.2.0]](https://github.com/springload/draftail/releases/tag/v1.2.0)
 
 > Documentation: [draftail.org/docs/getting-started](https://www.draftail.org/docs/getting-started)

diff --git a/lib/api/constants.js b/lib/api/constants.js
@@ -43,9 +43,21 @@ export const INLINE_STYLE = {
   SUBSCRIPT: "SUBSCRIPT",
 };
 
-export const BLOCK_TYPES = Object.values(BLOCK_TYPE);
-export const ENTITY_TYPES = Object.values(ENTITY_TYPE);
-export const INLINE_STYLES = Object.values(INLINE_STYLE);
+const BLOCK_TYPES = Object.values(BLOCK_TYPE);
+const ENTITY_TYPES = Object.values(ENTITY_TYPE);
+const INLINE_STYLES = Object.values(INLINE_STYLE);
+
+export const KEY_COMMANDS = [
+  ...BLOCK_TYPES,
+  ...ENTITY_TYPES,
+  ...INLINE_STYLES,
+  // Lowercase identifiers used by Draft.js
+  // See https://github.com/facebook/draft-js/blob/585af35c3a8c31fefb64bc884d4001faa96544d3/src/model/constants/DraftEditorCommand.js#L58-L61.
+  "bold",
+  "italic",
+  "underline",
+  "code",
+];
 
 export const FONT_FAMILY_MONOSPACE =
   "Consolas, Menlo, Monaco, Lucida Console, Liberation Mono, DejaVu Sans Mono, Bitstream Vera Sans Mono, Courier New, monospace, sans-serif";

diff --git a/lib/api/constants.test.js b/lib/api/constants.test.js
@@ -2,9 +2,7 @@ import {
   BLOCK_TYPE,
   ENTITY_TYPE,
   INLINE_STYLE,
-  BLOCK_TYPES,
-  ENTITY_TYPES,
-  INLINE_STYLES,
+  KEY_COMMANDS,
   FONT_FAMILY_MONOSPACE,
   CUSTOM_STYLE_MAP,
   BR_TYPE,
@@ -24,9 +22,7 @@ describe("constants", () => {
   it("#BLOCK_TYPE", () => expect(BLOCK_TYPE).toBeDefined());
   it("#ENTITY_TYPE", () => expect(ENTITY_TYPE).toBeDefined());
   it("#INLINE_STYLE", () => expect(INLINE_STYLE).toBeDefined());
-  it("#BLOCK_TYPES", () => expect(BLOCK_TYPES).toBeDefined());
-  it("#ENTITY_TYPES", () => expect(ENTITY_TYPES).toBeDefined());
-  it("#INLINE_STYLES", () => expect(INLINE_STYLES).toBeDefined());
+  it("#KEY_COMMANDS", () => expect(KEY_COMMANDS).toBeDefined());
   it("#FONT_FAMILY_MONOSPACE", () =>
     expect(FONT_FAMILY_MONOSPACE).toBeDefined());
   it("#CUSTOM_STYLE_MAP", () => expect(CUSTOM_STYLE_MAP).toBeDefined());

diff --git a/lib/components/DraftailEditor.js b/lib/components/DraftailEditor.js
@@ -18,9 +18,7 @@ import decorateComponentWithProps from "decorate-component-with-props";
 import {
   ENTITY_TYPE,
   BLOCK_TYPE,
-  ENTITY_TYPES,
-  BLOCK_TYPES,
-  INLINE_STYLES,
+  KEY_COMMANDS,
   HANDLED,
   NOT_HANDLED,
   UNDO_TYPE,
@@ -557,19 +555,20 @@ class DraftailEditor extends Component<Props, State> {
 
   /* :: handleKeyCommand: (command: DraftEditorCommand) => 'handled' | 'not-handled'; */
   handleKeyCommand(command: DraftEditorCommand) {
+    const { entityTypes, blockTypes, inlineStyles } = this.props;
     const { editorState } = this.state;
 
-    if (ENTITY_TYPES.includes(command)) {
+    if (entityTypes.some((t) => t.type === command)) {
       this.onRequestSource(command);
       return HANDLED;
     }
 
-    if (BLOCK_TYPES.includes(command)) {
+    if (blockTypes.some((t) => t.type === command)) {
       this.toggleBlockType(command);
       return HANDLED;
     }
 
-    if (INLINE_STYLES.includes(command)) {
+    if (inlineStyles.some((t) => t.type === command)) {
       this.toggleInlineStyle(command);
       return HANDLED;
     }
@@ -584,6 +583,11 @@ class DraftailEditor extends Component<Props, State> {
       }
     }
 
+    // If the command is known but not whitelisted for this editor, treat it as handled but don't do anything.
+    if (KEY_COMMANDS.includes(command)) {
+      return HANDLED;
+    }
+
     const newState = RichUtils.handleKeyCommand(editorState, command);
     if (newState) {
       this.onChange(newState);

diff --git a/lib/components/DraftailEditor.test.js b/lib/components/DraftailEditor.test.js
@@ -651,28 +651,62 @@ describe("DraftailEditor", () => {
       RichUtils.handleKeyCommand.mockRestore();
     });
 
-    it("entity type", () => {
-      expect(
-        shallowNoLifecycle(<DraftailEditor />)
-          .instance()
-          .handleKeyCommand("LINK"),
-      ).toBe("handled");
+    it("entity type - active", () => {
+      const wrapper = shallowNoLifecycle(
+        <DraftailEditor entityTypes={[{ type: "LINK", source: () => null }]} />,
+      ).instance();
+      jest.spyOn(wrapper, "onRequestSource");
+      expect(wrapper.handleKeyCommand("LINK")).toBe("handled");
+      expect(wrapper.onRequestSource).toHaveBeenCalled();
     });
 
-    it("block type", () => {
-      expect(
-        shallowNoLifecycle(<DraftailEditor />)
-          .instance()
-          .handleKeyCommand("header-one"),
-      ).toBe("handled");
+    it("entity type - inactive", () => {
+      const wrapper = shallowNoLifecycle(<DraftailEditor />).instance();
+      jest.spyOn(wrapper, "onRequestSource");
+      expect(wrapper.handleKeyCommand("LINK")).toBe("handled");
+      expect(wrapper.onRequestSource).not.toHaveBeenCalled();
     });
 
-    it("inline style", () => {
-      expect(
-        shallowNoLifecycle(<DraftailEditor />)
-          .instance()
-          .handleKeyCommand("BOLD"),
-      ).toBe("handled");
+    it("block type - active", () => {
+      const wrapper = shallowNoLifecycle(
+        <DraftailEditor blockTypes={[{ type: "header-one" }]} />,
+      ).instance();
+      jest.spyOn(wrapper, "toggleBlockType");
+      expect(wrapper.handleKeyCommand("header-one")).toBe("handled");
+      expect(wrapper.toggleBlockType).toHaveBeenCalled();
+    });
+
+    it("block type - inactive", () => {
+      const wrapper = shallowNoLifecycle(<DraftailEditor />).instance();
+      jest.spyOn(wrapper, "toggleBlockType");
+      expect(wrapper.handleKeyCommand("header-one")).toBe("handled");
+      expect(wrapper.toggleBlockType).not.toHaveBeenCalled();
+    });
+
+    it("inline style - active", () => {
+      const wrapper = shallowNoLifecycle(
+        <DraftailEditor inlineStyles={[{ type: "BOLD" }]} />,
+      ).instance();
+      jest.spyOn(wrapper, "toggleInlineStyle");
+      expect(wrapper.handleKeyCommand("BOLD")).toBe("handled");
+      expect(wrapper.toggleInlineStyle).toHaveBeenCalled();
+    });
+
+    it("inline style - inactive", () => {
+      const wrapper = shallowNoLifecycle(<DraftailEditor />).instance();
+      jest.spyOn(wrapper, "toggleInlineStyle");
+      expect(wrapper.handleKeyCommand("BOLD")).toBe("handled");
+      expect(wrapper.toggleInlineStyle).not.toHaveBeenCalled();
+    });
+
+    it("draft-js defaults", () => {
+      const wrapper = shallowNoLifecycle(<DraftailEditor />).instance();
+      jest.spyOn(wrapper, "toggleInlineStyle");
+      expect(wrapper.handleKeyCommand("bold")).toBe("handled");
+      expect(wrapper.handleKeyCommand("italic")).toBe("handled");
+      expect(wrapper.handleKeyCommand("underline")).toBe("handled");
+      expect(wrapper.handleKeyCommand("code")).toBe("handled");
+      expect(wrapper.toggleInlineStyle).not.toHaveBeenCalled();
     });
 
     describe("delete", () => {

diff --git a/tests/performance/markov_draftjs_41.test.js b/tests/performance/markov_draftjs_41.test.js
@@ -48,7 +48,7 @@ describe("performance", () => {
     expect(results.mean).toBeLessThan(2 * PERFORMANCE_BUFFER);
     expect(results.min).toBeLessThan(1 * PERFORMANCE_BUFFER);
     expect(results.median).toBeLessThan(2 * PERFORMANCE_BUFFER);
-    expect(results.max).toBeLessThan(8 * PERFORMANCE_BUFFER);
+    expect(results.max).toBeLessThan(12 * PERFORMANCE_BUFFER);
   });
 
   it("markov_draftjs[41] update", () => {