Merge pull request #9057 from ckeditor/i/5705

Fix (utils): The keystrokes are no longer conflicting on macOS. Closes #5705. Feature (utils): Added forced modifier key (`Ctrl!`) for keystrokes that should not be mapped to <kbd>Command</kbd> on macOS. Other (engine): The `KeyObserver` should provide information about `metaKey` being pressed. Other (list): The to-do list item toggle keystroke changed to <kbd>Ctrl</kbd>+<kbd>Enter</kbd> (<kbd>Cmd</kbd>+<kbd>Enter</kbd> on Mac). Internal (typing): Any keypress while the <kbd>Cmd</kbd> key is pressed is not a typing keystroke. BREAKING CHANGE: On macOS keystrokes with the <kbd>Ctrl</kbd> modifier will not be handled unless the modifier is registered as the forced one (for example: `Ctrl!+A` will not be translated to `Cmd+A` on macOS). BREAKING CHANGE (list): The to-do list item toggle keystroke changed to <kbd>Ctrl</kbd>+<kbd>Enter</kbd> (<kbd>Cmd</kbd>+<kbd>Enter</kbd> on Mac).
ckeditor · Feb 23, 2021 · 8dac3a9 · 8dac3a9
2 parents 959c1d6 + 344fc54
commit 8dac3a9
Show file tree

Hide file tree

Showing 10 changed files with 245 additions and 85 deletions.
diff --git a/packages/ckeditor5-engine/src/view/observer/keyobserver.js b/packages/ckeditor5-engine/src/view/observer/keyobserver.js
@@ -29,8 +29,9 @@ export default class KeyObserver extends DomEventObserver {
 			keyCode: domEvt.keyCode,
 
 			altKey: domEvt.altKey,
-			ctrlKey: domEvt.ctrlKey || domEvt.metaKey,
+			ctrlKey: domEvt.ctrlKey,
 			shiftKey: domEvt.shiftKey,
+			metaKey: domEvt.metaKey,
 
 			get keystroke() {
 				return getCode( this );

diff --git a/packages/ckeditor5-engine/tests/view/observer/keyobserver.js b/packages/ckeditor5-engine/tests/view/observer/keyobserver.js
@@ -84,17 +84,28 @@ describe( 'KeyObserver', () => {
 			expect( getCode( data ) ).to.be.greaterThan( 111 );
 		} );
 
-		it( 'should fire keydown ctrlKey set to true one meta (cmd) was pressed', () => {
+		it( 'should fire keydown with ctrlKey set to true once ctrl was pressed', () => {
 			const spy = sinon.spy();
 
 			viewDocument.on( 'keydown', spy );
 
-			observer.onDomEvent( { type: 'keydown', target: document.body, keyCode: 111, metaKey: true } );
+			observer.onDomEvent( { type: 'keydown', target: document.body, keyCode: 111, ctrlKey: true } );
 
 			const data = spy.args[ 0 ][ 1 ];
 			expect( data ).to.have.property( 'ctrlKey', true );
 		} );
 
+		it( 'should fire keydown with metaKey set to true once meta (cmd) was pressed', () => {
+			const spy = sinon.spy();
+
+			viewDocument.on( 'keydown', spy );
+
+			observer.onDomEvent( { type: 'keydown', target: document.body, keyCode: 111, metaKey: true } );
+
+			const data = spy.args[ 0 ][ 1 ];
+			expect( data ).to.have.property( 'metaKey', true );
+		} );
+
 		it( 'should fire keyup with the target and key info', () => {
 			const spy = sinon.spy();
 

diff --git a/packages/ckeditor5-list/docs/features/todo-lists.md b/packages/ckeditor5-list/docs/features/todo-lists.md
@@ -16,7 +16,7 @@ To-do lists can be introduced using the dedicated toolbar button. Thanks to the
 
 ## Keyboard support
 
-You can check and uncheck a list item by using the <kbd>Ctrl</kbd> + <kbd>Space</kbd> shortcut when the selection is in that item.
+You can check and uncheck a list item by using the <kbd>Ctrl</kbd> + <kbd>Enter</kbd> (<kbd>⌘</kbd> + <kbd>Enter</kbd> on Mac) shortcut when the selection is in that item.
 
 ## Installation
 

diff --git a/packages/ckeditor5-list/src/todolistediting.js b/packages/ckeditor5-list/src/todolistediting.js
@@ -8,7 +8,11 @@
  */
 
 import { Plugin } from 'ckeditor5/src/core';
-import { getLocalizedArrowKeyCodeDirection } from 'ckeditor5/src/utils';
+import {
+	getCode,
+	parseKeystroke,
+	getLocalizedArrowKeyCodeDirection
+} from 'ckeditor5/src/utils';
 
 import ListCommand from './listcommand';
 import ListEditing from './listediting';
@@ -22,6 +26,8 @@ import {
 	modelViewInsertion
 } from './todolistconverters';
 
+const ITEM_TOGGLE_KEYSTROKE = parseKeystroke( 'Ctrl+Enter' );
+
 /**
  * The engine of the to-do list feature. It handles creating, editing and removing to-do lists and their items.
  *
@@ -113,7 +119,12 @@ export default class TodoListEditing extends Plugin {
 		this.listenTo( editing.view.document, 'keydown', jumpOverCheckmarkOnSideArrowKeyPress( model, editor.locale ) );
 
 		// Toggle check state of selected to-do list items on keystroke.
-		editor.keystrokes.set( 'Ctrl+space', () => editor.execute( 'checkTodoList' ) );
+		this.listenTo( editing.view.document, 'keydown', ( evt, data ) => {
+			if ( getCode( data ) === ITEM_TOGGLE_KEYSTROKE ) {
+				editor.execute( 'checkTodoList' );
+				evt.stop();
+			}
+		}, { priority: 'high' } );
 
 		// Remove `todoListChecked` attribute when a host element is no longer a to-do list item.
 		const listItemsToFix = new Set();

diff --git a/packages/ckeditor5-list/tests/todolistediting.js b/packages/ckeditor5-list/tests/todolistediting.js
@@ -23,12 +23,16 @@ import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils
 import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
 import { assertEqualMarkup } from '@ckeditor/ckeditor5-utils/tests/_utils/utils';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
+import { env } from '@ckeditor/ckeditor5-utils';
 
 /* global Event, document */
 
 describe( 'TodoListEditing', () => {
 	let editor, model, modelDoc, modelRoot, view, viewDoc;
 
+	testUtils.createSinonSandbox();
+
 	beforeEach( () => {
 		return VirtualTestEditor
 			.create( {
@@ -1154,30 +1158,55 @@ describe( 'TodoListEditing', () => {
 				sinon.assert.notCalled( domEvtDataStub.preventDefault );
 				sinon.assert.notCalled( domEvtDataStub.stopPropagation );
 			} );
+
+			it( 'should do nothing when other arrow key was pressed', () => {
+				setModelData( model, '<listItem listIndent="0" listType="todo">[]bar</listItem>' );
+
+				domEvtDataStub = {
+					keyCode: getCode( 'arrowDown' ),
+					preventDefault: sinon.spy(),
+					stopPropagation: sinon.spy(),
+					domTarget: {
+						ownerDocument: {
+							defaultView: {
+								getSelection: () => ( { rangeCount: 0 } )
+							}
+						}
+					}
+				};
+
+				viewDoc.fire( 'keydown', domEvtDataStub );
+
+				sinon.assert.notCalled( domEvtDataStub.preventDefault );
+				sinon.assert.notCalled( domEvtDataStub.stopPropagation );
+			} );
 		}
 	} );
 
-	describe( 'Ctrl+space keystroke handling', () => {
-		let domEvtDataStub;
+	describe( 'Ctrl+enter keystroke handling', () => {
+		it( 'should execute CheckTodoListCommand', () => {
+			const command = editor.commands.get( 'checkTodoList' );
+
+			sinon.spy( command, 'execute' );
 
-		beforeEach( () => {
-			domEvtDataStub = {
-				keyCode: getCode( 'space' ),
-				ctrlKey: true,
+			const domEvtDataStub = {
+				keyCode: getCode( 'enter' ),
 				preventDefault: sinon.spy(),
 				stopPropagation: sinon.spy()
 			};
-		} );
 
-		it( 'should execute CheckTodoListCommand', () => {
-			const command = editor.commands.get( 'checkTodoList' );
-
-			sinon.spy( command, 'execute' );
+			if ( env.isMac ) {
+				domEvtDataStub.metaKey = true;
+			} else {
+				domEvtDataStub.ctrlKey = true;
+			}
 
+			// First call.
 			viewDoc.fire( 'keydown', domEvtDataStub );
 
 			sinon.assert.calledOnce( command.execute );
 
+			// Second call.
 			viewDoc.fire( 'keydown', domEvtDataStub );
 
 			sinon.assert.calledTwice( command.execute );

diff --git a/packages/ckeditor5-typing/src/utils/injectunsafekeystrokeshandling.js b/packages/ckeditor5-typing/src/utils/injectunsafekeystrokeshandling.js
@@ -171,8 +171,8 @@ for ( let code = 112; code <= 135; code++ ) {
  * @returns {Boolean}
  */
 export function isNonTypingKeystroke( keyData ) {
-	// Keystrokes which contain Ctrl don't represent typing.
-	if ( keyData.ctrlKey ) {
+	// Keystrokes which contain Ctrl or Cmd don't represent typing.
+	if ( keyData.ctrlKey || keyData.metaKey ) {
 		return true;
 	}
 

diff --git a/packages/ckeditor5-typing/tests/utils/injectunsafekeystrokeshandling.js b/packages/ckeditor5-typing/tests/utils/injectunsafekeystrokeshandling.js
@@ -17,6 +17,11 @@ describe( 'unsafe keystroke handling utils', () => {
 			expect( isNonTypingKeystroke( { keyCode: keyCodes[ 0 ], ctrlKey: true } ), 'Ctrl+0' ).to.be.true;
 		} );
 
+		it( 'should return "true" for any keystroke with the Cmd key', () => {
+			expect( isNonTypingKeystroke( { keyCode: keyCodes.a, metaKey: true } ), '⌘a' ).to.be.true;
+			expect( isNonTypingKeystroke( { keyCode: keyCodes[ 0 ], metaKey: true } ), '⌘0' ).to.be.true;
+		} );
+
 		it( 'should return "true" for all arrow keys', () => {
 			expect( isNonTypingKeystroke( { keyCode: keyCodes.arrowup } ), 'arrow up' ).to.be.true;
 			expect( isNonTypingKeystroke( { keyCode: keyCodes.arrowdown } ), 'arrow down' ).to.be.true;

diff --git a/packages/ckeditor5-utils/src/keyboard.js b/packages/ckeditor5-utils/src/keyboard.js
@@ -12,16 +12,17 @@
 import CKEditorError from './ckeditorerror';
 import env from './env';
 
-const macGlyphsToModifiers = {
-	'⌘': 'ctrl',
-	'⇧': 'shift',
-	'⌥': 'alt'
+const modifiersToGlyphsMac = {
+	ctrl: '⌃',
+	cmd: '⌘',
+	alt: '⌥',
+	shift: '⇧'
 };
 
-const modifiersToMacGlyphs = {
-	'ctrl': '⌘',
-	'shift': '⇧',
-	'alt': '⌥'
+const modifiersToGlyphsNonMac = {
+	ctrl: 'Ctrl+',
+	alt: 'Alt+',
+	shift: 'Shift+'
 };
 
 /**
@@ -38,6 +39,10 @@ const modifiersToMacGlyphs = {
  */
 export const keyCodes = generateKnownKeyCodes();
 
+const keyCodeNames = Object.fromEntries(
+	Object.entries( keyCodes ).map( ( [ name, code ] ) => [ code, name.charAt( 0 ).toUpperCase() + name.slice( 1 ) ] )
+);
+
 /**
  * Converts a key name or a {@link module:utils/keyboard~KeystrokeInfo keystroke info} into a key code.
  *
@@ -66,7 +71,8 @@ export function getCode( key ) {
 		keyCode = key.keyCode +
 			( key.altKey ? keyCodes.alt : 0 ) +
 			( key.ctrlKey ? keyCodes.ctrl : 0 ) +
-			( key.shiftKey ? keyCodes.shift : 0 );
+			( key.shiftKey ? keyCodes.shift : 0 ) +
+			( key.metaKey ? keyCodes.cmd : 0 );
 	}
 
 	return keyCode;
@@ -96,7 +102,7 @@ export function parseKeystroke( keystroke ) {
 	}
 
 	return keystroke
-		.map( key => ( typeof key == 'string' ) ? getCode( key ) : key )
+		.map( key => ( typeof key == 'string' ) ? getEnvKeyCode( key ) : key )
 		.reduce( ( key, sum ) => sum + key, 0 );
 }
 
@@ -108,22 +114,21 @@ export function parseKeystroke( keystroke ) {
  * @returns {String} Keystroke text specific for the environment.
  */
 export function getEnvKeystrokeText( keystroke ) {
-	if ( !env.isMac ) {
-		return keystroke;
-	}
+	let keystrokeCode = parseKeystroke( keystroke );
+
+	const modifiersToGlyphs = Object.entries( env.isMac ? modifiersToGlyphsMac : modifiersToGlyphsNonMac );
+
+	const modifiers = modifiersToGlyphs.reduce( ( modifiers, [ name, glyph ] ) => {
+		// Modifier keys are stored as a bit mask so extract those from the keystroke code.
+		if ( ( keystrokeCode & keyCodes[ name ] ) != 0 ) {
+			keystrokeCode &= ~keyCodes[ name ];
+			modifiers += glyph;
+		}
+
+		return modifiers;
+	}, '' );
 
-	return splitKeystrokeText( keystroke )
-		// Replace modifiers (e.g. "ctrl") with Mac glyphs (e.g. "⌘") first.
-		.map( key => modifiersToMacGlyphs[ key.toLowerCase() ] || key )
-
-		// Decide whether to put "+" between keys in the keystroke or not.
-		.reduce( ( value, key ) => {
-			if ( value.slice( -1 ) in macGlyphsToModifiers ) {
-				return value + key;
-			} else {
-				return value + '+' + key;
-			}
-		} );
+	return modifiers + ( keystrokeCode ? keyCodeNames[ keystrokeCode ] : '' );
 }
 
 /**
@@ -169,6 +174,23 @@ export function getLocalizedArrowKeyCodeDirection( keyCode, contentLanguageDirec
 	}
 }
 
+// Converts a key name to the key code with mapping based on the env.
+//
+// See: {@link module:utils/keyboard~getCode}.
+//
+// @param {String} key The key name (see {@link module:utils/keyboard~keyCodes}).
+// @returns {Number} Key code.
+function getEnvKeyCode( key ) {
+	// Don't remap modifier key for forced modifiers.
+	if ( key.endsWith( '!' ) ) {
+		return getCode( key.slice( 0, -1 ) );
+	}
+
+	const code = getCode( key );
+
+	return env.isMac && code == keyCodes.ctrl ? keyCodes.cmd : code;
+}
+
 /**
  * Determines if the provided key code moves the {@link module:engine/model/documentselection~DocumentSelection selection}
  * forward or backward considering the language direction of the editor content.
@@ -203,11 +225,9 @@ function generateKnownKeyCodes() {
 		// The idea about these numbers is that they do not collide with any real key codes, so we can use them
 		// like bit masks.
 		ctrl: 0x110000,
-		// Has the same code as ctrl, because their behaviour should be unified across the editor.
-		// See http://ckeditor.github.io/editor-recommendations/general-policies#ctrl-vs-cmd
-		cmd: 0x110000,
 		shift: 0x220000,
-		alt: 0x440000
+		alt: 0x440000,
+		cmd: 0x880000
 	};
 
 	// a-z
@@ -249,17 +269,23 @@ function splitKeystrokeText( keystroke ) {
 /**
  * Whether the <kbd>Alt</kbd> modifier was pressed.
  *
- * @member {Bolean} module:utils/keyboard~KeystrokeInfo#altKey
+ * @member {Boolean} module:utils/keyboard~KeystrokeInfo#altKey
  */
 
 /**
- * Whether the <kbd>Ctrl</kbd> or <kbd>Cmd</kbd> modifier was pressed.
+ * Whether the <kbd>Ctrl</kbd> modifier was pressed.
  *
- * @member {Bolean} module:utils/keyboard~KeystrokeInfo#ctrlKey
+ * @member {Boolean} module:utils/keyboard~KeystrokeInfo#ctrlKey
  */
 
 /**
  * Whether the <kbd>Shift</kbd> modifier was pressed.
  *
- * @member {Bolean} module:utils/keyboard~KeystrokeInfo#shiftKey
+ * @member {Boolean} module:utils/keyboard~KeystrokeInfo#shiftKey
+ */
+
+/**
+ * Whether the <kbd>Cmd</kbd> modifier was pressed.
+ *
+ * @member {Boolean} module:utils/keyboard~KeystrokeInfo#metaKey
  */