t/90 EditingKeystrokeHandler should prevent default only for commands

[oleq]

Suggested merge commit message (convention)

Fix: EditingKeystrokeHandler should prevent default only for commands. Closes ckeditor/ckeditor5#2918.

[Reinmar]

It blows up undo and basic-styles tests.

Besides that, it fixes the issue. But I'd like to understand one thing – why was listenTo() overridden before and why it isn't needed any more? Some leftover?

[oleq]

It blows up undo and basic-styles tests.

• https://github.com/ckeditor/ckeditor5-basic-styles/tree/t/ckeditor5-core/90
• https://github.com/ckeditor/ckeditor5-undo/tree/t/ckeditor5-core/90

why was listenTo() overridden before and why it isn't needed any more? Some leftover?

Before, preventDefault was exclusive for keydown events only.

In my fix, it works at the KH#press() level because the code deciding whether to preventDefault or not has moved to EKH#set(). Thus the custom listenTo is no longer necessary.

At first, I wanted to leave things unchanged with overridden EKH#listenTo() but the problem is there's no way to tell whether it's a "command keystroke" or "callback keystroke" there, no way to decide whether preventDefault() or not without making the code much more complex (press() only tells if was handled or not).

As the logic moved to EKH#set() things became a little bit more complex, e.g. those undo and basic-style tests have blown up because preventDefault() is called in KH#press(), which means

const wasHandled = editor.keystrokes.press( {
	keyCode: keyCodes.b,
	ctrlKey: true
} );

is not enough any more. It was enough was because preventDefault() happened in EKH#listenTo(), not KH#press(). That's why you need to provide the mocks to avoid errors

const wasHandled = editor.keystrokes.press( {
	keyCode: keyCodes.b,
	ctrlKey: true,
	preventDefault: sinon.spy(),
	stopPropagation: sinon.spy()
} );

and TBH I totally hate it but have no other idea how to handle it. To get back to the preventDefault() at the listenTo() level we'd need to pass an information there whether it's a "command keystroke" or "callback keystroke", which would need to be saved first by EKH#set() and then passed via KH#press(). Any ideas?

[Reinmar]

and TBH I totally hate it but have no other idea how to handle it.

I think that the change is good. It makes sense.

I also don't like the fact that press() needs to be called with the entire KeyEventData object, but it should've been called with it in the past too. So the solution to that could be to allow easily mocking this object for tests.