fix(keymaps): apply prevent option without relying on CanvasView (#6804)

The `prevent` option delegated to `CanvasView.preventDefault`, resolved
through `Canvas.getCanvasView()` with optional chaining, so the default
browser action was silently kept whenever the canvas view was not
available (editor not rendered yet, canvas removed). `Commands`,
`Canvas` and the editor instance were also captured when the keymap was
added instead of when it runs.

Move the prevention to a shared `preventDefault` DOM util, which also
handles the original event of the canvas frame (`_parentEvent`), and
resolve the modules inside the handler.

packages/core/src/canvas/view/CanvasView.ts

@@ -12,6 +12,7 @@ import {
   isTextNode,
   off,
   on,
+  preventDefault,
 } from '../../utils/dom';
 import { getComponentView, getElement, getUiClass } from '../../utils/mixins';
 import Canvas from '../model/Canvas';
@@ -145,10 +146,7 @@ export default class CanvasView extends ModuleView<Canvas> {
   }
 
   preventDefault(ev: Event) {
-    if (ev) {
-      ev.preventDefault();
-      (ev as any)._parentEvent?.preventDefault();
-    }
+    preventDefault(ev);
   }
 
   toggleListeners(enable: boolean) {

packages/core/src/keymaps/index.ts

@@ -38,6 +38,7 @@ import { isFunction, isString } from 'underscore';
 import { Module } from '../abstract';
 import EditorModel from '../editor/model/Editor';
 import keymaster from '../utils/keymaster';
+import { preventDefault } from '../utils/dom';
 import { hasWin } from '../utils/mixins';
 import defConfig, { Keymap, KeymapOptions, KeymapsConfig } from './config';
 import { KeymapsEvents } from './types';
@@ -99,9 +100,6 @@ export default class KeymapsModule extends Module<KeymapsConfig & { name?: strin
    */
   add(id: Keymap['id'], keys: Keymap['keys'], handler: Keymap['handler'], opts: KeymapOptions = {}) {
     const { em, events } = this;
-    const cmd = em.Commands;
-    const editor = em.getEditor();
-    const canvas = em.Canvas;
     const keymap: Keymap = { id, keys, handler };
     const pk = this.keymaps[id];
     pk && this.remove(id);
@@ -110,11 +108,15 @@ export default class KeymapsModule extends Module<KeymapsConfig & { name?: strin
       keys,
       (e: any, h: any) => {
         // It's safer putting handlers resolution inside the callback
+        const cmd = em.Commands;
+        const editor = em.getEditor();
         const opt = { event: e, h };
         const handlerRes = isString(handler) ? cmd.get(handler) : handler;
-        const ableTorun = !em.isEditing() && !editor.Canvas.isInputFocused();
+        const ableTorun = !em.isEditing() && !em.Canvas.isInputFocused();
         if (ableTorun || opts.force) {
-          opts.prevent && canvas.getCanvasView()?.preventDefault(e);
+          // Prevent as soon as possible, the default action of the key has to be
+          // avoided even if the handler is missing or throws.
+          opts.prevent && preventDefault(e);
           isFunction(handlerRes) ? handlerRes(editor, 0, opt) : cmd.runCommand(handlerRes, opt);
           const args = [id, h.shortcut, e];
           // @ts-ignore

packages/core/src/utils/dom.ts

@@ -21,6 +21,17 @@ export const motionsEv = 'transitionend oTransitionEnd transitionend webkitTrans
 
 export const isDoc = (el?: Node): el is Document => el?.nodeType === Node.DOCUMENT_NODE;
 
+/**
+ * Prevent the default of an event.
+ * Events coming from the canvas frame are re-dispatched on the main document (see `createCustomEvent`),
+ * so the original one, kept in `_parentEvent`, has to be prevented as well.
+ */
+export const preventDefault = (ev?: Event) => {
+  if (!ev) return;
+  ev.preventDefault();
+  (ev as any)._parentEvent?.preventDefault();
+};
+
 export const removeEl = (el?: HTMLElement) => {
   const parent = el && el.parentNode;
   parent && parent.removeChild(el);

packages/core/test/specs/keymaps/index.js

@@ -13,6 +13,11 @@ describe('Keymaps', () => {
       obj = editor.Keymaps;
     });
 
+    afterEach(() => {
+      // Bindings are kept in a module-level registry, shared between editors
+      obj.removeAll();
+    });
+
     test('Object exists', () => {
       expect(obj).toBeTruthy();
     });
@@ -56,6 +61,51 @@ describe('Keymaps', () => {
       expect(called).toEqual(1);
     });
 
+    describe('Prevent option', () => {
+      const dispatchKey = (props = {}) => {
+        const keyboardEvent = new KeyboardEvent('keydown', {
+          keyCode: 83,
+          which: 83,
+          ctrlKey: true,
+          cancelable: true,
+          bubbles: true,
+        });
+        Object.assign(keyboardEvent, props);
+        document.dispatchEvent(keyboardEvent);
+        return keyboardEvent;
+      };
+
+      beforeEach(() => {
+        em.setEditing(0);
+      });
+
+      it('Should prevent the default action', () => {
+        const handler = jest.fn();
+        obj.add('test', 'ctrl+s', handler, { prevent: true });
+        const event = dispatchKey();
+
+        expect(handler).toHaveBeenCalled();
+        expect(event.defaultPrevented).toBe(true);
+      });
+
+      it('Should prevent the default action of the event coming from the frame', () => {
+        obj.add('test', 'ctrl+s', () => {}, { prevent: true });
+        // Events triggered inside the canvas frame are re-dispatched on the main
+        // document, the original one is kept in `_parentEvent`.
+        const parentEvent = new KeyboardEvent('keydown', { cancelable: true });
+        dispatchKey({ _parentEvent: parentEvent });
+
+        expect(parentEvent.defaultPrevented).toBe(true);
+      });
+
+      it('Should not prevent the default action without the option', () => {
+        obj.add('test', 'ctrl+s', () => {});
+        const event = dispatchKey();
+
+        expect(event.defaultPrevented).toBe(false);
+      });
+    });
+
     describe('Given the edit is not on edit mode', () => {
       beforeEach(() => {
         em.setEditing(0);