The keystrokes are no longer conflicting on MacOS #9057
Conversation
| @@ -23,7 +23,7 @@ export default class EnterObserver extends Observer { | |||
| const doc = this.document; | |||
|
|
|||
| doc.on( 'keydown', ( evt, data ) => { | |||
| if ( this.isEnabled && data.keyCode == keyCodes.enter ) { | |||
| if ( this.isEnabled && data.keyCode == keyCodes.enter && !data.ctrlKey && !data.metaKey ) { | |||
There was a problem hiding this comment.
Curious why we want to not act on Ctrl+Enter/Cmd+Enter. After a brief check I didn't find it very consistent across editors:
- Google Docs creates a new paragraph on Cmd+Enter,
- Slack / Apple Pages create a new paragraph on Ctrl+Enter,
- Notion does the same as the new code above :)
There was a problem hiding this comment.
It's because editor.keystrokes is not stopping the original event so it splits the list item while pressing Cmd+Enter. Now I'm wondering if we should change the KeystrokeHandler behavior or this code.
There was a problem hiding this comment.
I'm changing it, to-do list will use 'keydown' listener so it can be stopped in the context of to-do list.
| @@ -97,6 +102,7 @@ export function parseKeystroke( keystroke ) { | |||
|
|
|||
| return keystroke | |||
| .map( key => ( typeof key == 'string' ) ? getCode( key ) : key ) | |||
| .map( key => env.isMac && key == keyCodes.ctrl ? keyCodes.cmd : key ) | |||
There was a problem hiding this comment.
Does this line mean you cannot assign Ctrl+... keystroke on a Mac? Oh, I see in the old code it was automatically assumed that Ctrl on Windows equals ⌘. Still surprising :)
There was a problem hiding this comment.
Yes. Otherwise, we would need to provide some "force" specifier or provide keystroke alternative for MacOS.
There was a problem hiding this comment.
Does this line means you cannot assign Ctrl+... keystroke on a Mac?
I'm worried about existing integrations here. Changing keystrokes in our features to resolve conflicts is a breaking change but disabling keystrokes in 3rd-party integrations is a whole new level. Just to clarify: there will be no way to use Ctrl+whatever on Mac after this change?
There was a problem hiding this comment.
After a talk with @Reinmar we decided that we will introduce Ctrl! as a forced Ctrl key that won't get mapped to Cmd on macOS. This is because some integrators or external features might want to use keystrokes that are not available after mapping to Cmd (for example Cmd+Q, Cmd+T, Cmd+W, etc.) but are available as Ctrl+Q, Ctrl+T, Ctrl+W.
|
Ready for another review round. |
packages/ckeditor5-typing/tests/utils/injectunsafekeystrokeshandling.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Maksymilian Barnaś <maxbarnas@users.noreply.github.com>
Suggested merge commit message (convention)
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 Command on macOS.Other (engine): The
KeyObservershould provide information aboutmetaKeybeing pressed.Other (list): The to-do list item toggle keystroke changed to Ctrl+Enter (Cmd+Enter on Mac).
Internal (typing): Any keypress while the Cmd key is pressed is not a typing keystroke.
BREAKING CHANGE: On macOS keystrokes with the Ctrl modifier will not be handled unless the modifier is registered as the forced one (for example:
Ctrl!+Awill not be translated toCmd+Aon macOS).BREAKING CHANGE (list): The to-do list item toggle keystroke changed to Ctrl+Enter (Cmd+Enter on Mac).
Additional information