Merge pull request #91 from ckeditor/t/90

Fix: `EditingKeystrokeHandler` should prevent default action only for commands. Closes #90.

src/editingkeystrokehandler.js

@@ -16,9 +16,9 @@ import KeystrokeHandler from '@ckeditor/ckeditor5-utils/src/keystrokehandler';
  *
  * E.g. an undo plugin would do this:
  *
- *		editor.keystrokes.set( 'ctrl + Z', 'undo' );
- *		editor.keystrokes.set( 'ctrl + shift + Z', 'redo' );
- *		editor.keystrokes.set( 'ctrl + Y', 'redo' );
+ *		editor.keystrokes.set( 'Ctrl+Z', 'undo' );
+ *		editor.keystrokes.set( 'Ctrl+Shift+Z', 'redo' );
+ *		editor.keystrokes.set( 'Ctrl+Y', 'redo' );
  *
  * @extends utils/keystrokehandler~KeystrokeHandler
  */
@@ -43,38 +43,26 @@ export default class EditingKeystrokeHandler extends KeystrokeHandler {
 	/**
 	 * Registers a handler for the specified keystroke.
 	 *
-	 * * The handler can be specified as a command name or a callback.
+	 * The handler can be specified as a command name or a callback.
 	 *
 	 * @param {String|Array.<String|Number>} keystroke Keystroke defined in a format accepted by
 	 * the {@link module:utils/keyboard~parseKeystroke} function.
-	 * @param {Function} callback If a string is passed, then the keystroke will
+	 * @param {Function|String} callback If a string is passed, then the keystroke will
 	 * {@link module:core/editor/editor~Editor#execute execute a command}.
 	 * If a function, then it will be called with the
 	 * {@link module:engine/view/observer/keyobserver~KeyEventData key event data} object and
-	 * a helper to both `preventDefault` and `stopPropagation` of the event.
+	 * a `cancel()` helper to both `preventDefault()` and `stopPropagation()` of the event.
 	 */
 	set( keystroke, callback ) {
 		if ( typeof callback == 'string' ) {
 			const commandName = callback;
 
-			callback = () => {
+			callback = ( evtData, cancel ) => {
 				this.editor.execute( commandName );
+				cancel();
 			};
 		}
 
 		super.set( keystroke, callback );
 	}
-
-	/**
-	 * @inheritDoc
-	 */
-	listenTo( emitter ) {
-		this._listener.listenTo( emitter, 'keydown', ( evt, data ) => {
-			const handled = this.press( data );
-
-			if ( handled ) {
-				data.preventDefault();
-			}
-		} );
-	}
 }

src/plugin.js

@@ -95,7 +95,7 @@ mix( Plugin, ObservableMixin );
  *			static get requires() {
  *				return [ Image ];
  *			}
- *      }
+ *		}
  *
  * @static
  * @readonly

tests/editingkeystrokehandler.js

@@ -8,58 +8,71 @@ import EditingKeystrokeHandler from '../src/editingkeystrokehandler';
 import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
 
 describe( 'EditingKeystrokeHandler', () => {
-	let editor, keystrokes;
+	let editor, keystrokes, executeSpy;
 
 	beforeEach( () => {
 		return VirtualTestEditor.create()
 			.then( newEditor => {
 				editor = newEditor;
 				keystrokes = new EditingKeystrokeHandler( editor );
+				executeSpy = sinon.stub( editor, 'execute' );
 			} );
 	} );
 
-	describe( 'listenTo()', () => {
-		it( 'prevents default when keystroke was handled', () => {
-			const keyEvtData = { keyCode: 1, preventDefault: sinon.spy() };
+	describe( 'set()', () => {
+		describe( 'with a command', () => {
+			it( 'prevents default when the keystroke was handled', () => {
+				const keyEvtData = getCtrlA();
 
-			sinon.stub( keystrokes, 'press' ).returns( true );
+				keystrokes.set( 'Ctrl+A', 'foo' );
+				keystrokes.press( keyEvtData );
 
-			keystrokes.listenTo( editor.editing.view );
-			editor.editing.view.fire( 'keydown', keyEvtData );
+				sinon.assert.calledWithExactly( executeSpy, 'foo' );
+				sinon.assert.calledOnce( keyEvtData.preventDefault );
+				sinon.assert.calledOnce( keyEvtData.stopPropagation );
+			} );
 
-			sinon.assert.calledOnce( keyEvtData.preventDefault );
-		} );
+			it( 'does not prevent default when the keystroke was not handled', () => {
+				const keyEvtData = getCtrlA();
 
-		it( 'does not prevent default when keystroke was not handled', () => {
-			const keyEvtData = { keyCode: 1, preventDefault: sinon.spy() };
+				keystrokes.press( keyEvtData );
 
-			sinon.stub( keystrokes, 'press' ).returns( false );
+				sinon.assert.notCalled( executeSpy );
+				sinon.assert.notCalled( keyEvtData.preventDefault );
+				sinon.assert.notCalled( keyEvtData.stopPropagation );
+			} );
+		} );
 
-			keystrokes.listenTo( editor.editing.view );
-			editor.editing.view.fire( 'keydown', keyEvtData );
+		describe( 'with a callback', () => {
+			it( 'never prevents default', () => {
+				const callback = sinon.spy();
+				const keyEvtData = getCtrlA();
 
-			sinon.assert.notCalled( keyEvtData.preventDefault );
+				keystrokes.set( 'Ctrl+A', callback );
+				keystrokes.press( keyEvtData );
+
+				sinon.assert.calledOnce( callback );
+				sinon.assert.notCalled( keyEvtData.preventDefault );
+				sinon.assert.notCalled( keyEvtData.stopPropagation );
+			} );
 		} );
 	} );
 
 	describe( 'press()', () => {
 		it( 'executes a command', () => {
-			const spy = sinon.stub( editor, 'execute' );
-
-			keystrokes.set( 'ctrl + A', 'foo' );
+			keystrokes.set( 'Ctrl+A', 'foo' );
 
 			const wasHandled = keystrokes.press( getCtrlA() );
 
-			sinon.assert.calledOnce( spy );
-			sinon.assert.calledWithExactly( spy, 'foo' );
+			sinon.assert.calledOnce( executeSpy );
+			sinon.assert.calledWithExactly( executeSpy, 'foo' );
 			expect( wasHandled ).to.be.true;
 		} );
 
 		it( 'executes a callback', () => {
-			const executeSpy = sinon.stub( editor, 'execute' );
 			const callback = sinon.spy();
 
-			keystrokes.set( 'ctrl + A', callback );
+			keystrokes.set( 'Ctrl+A', callback );
 
 			const wasHandled = keystrokes.press( getCtrlA() );
 
@@ -71,5 +84,10 @@ describe( 'EditingKeystrokeHandler', () => {
 } );
 
 function getCtrlA() {
-	return { keyCode: keyCodes.a, ctrlKey: true };
+	return {
+		keyCode: keyCodes.a,
+		ctrlKey: true,
+		preventDefault: sinon.spy(),
+		stopPropagation: sinon.spy()
+	};
 }