[Umbrella] Keypress listeners should/can be removed or replaced

[Piotrek Koszuliński]

For some historical reasons we use keypress in many places - e.g. in keystroke handler, undo manager and in the toolbar.

From what I know we were forced to do that, because of some bugs or inconsistencies (FF on Mac and Opera?). But now this ​event is finally gone and we ​don't seem to need it.

Related tickets:

• #10926,
• #11611,
• #2140.

[Olek Nowodziński]

cc

[Piotrek Koszuliński]

I'm not sure if it was always like this but currently there's a significant difference when keypress is fired on Firefox and Chrome. On Firefox it is fired for arrow keys, when on Chrome not (see #11611). There may be more problems with it, so since it's deprecated better to get rid of it.

[Piotrek Koszuliński]

I forgot to mention that there's one more reason why I assigned all this to 4.4.2. I noticed recently some problems with making snapshots while typing. I don't remember what was it exactly, but I think it'll be good to review the existing implementation while working on this ticket.

[Marek Lewandowski]

I've written some info on how things works and so one at ​gist.

Current solution still has deletion undo issue which i've mentioned at Friday. But with PK suggestion I was able to reuse the same code for IE (more info in gist).

Pushed to ​t/11739_input at dev and t/11739_input at tests.

[Marek Lewandowski]

Current solution pushed to ​t/11739_input at dev and t/11739_input at tests.

Tests still needs to be ported to Bender, and I need to use function used in #12071 to make tests work for IE8, because it's messy there.

[Marek Lewandowski]

I will put here some overview (both high level and detailed) which was created in linked gist.

Undo keyboard user guide

General overview

How does undo work?

It watches for your keyboard input and triggers saveSnapshot.

Dictionary

• functional keys - keys that modifies content, currently it's backspace and delete
• printable keys - all the keys which input some printable characters to editable
• keystrokes limit - maximum number of keys that can be typed within a snapshot
• selection amend - replacing last snapshot, with the current one if content is the same and selection didn't change

When a snapshot is created?

• When you're exceeding keystrokes limit
• While typing and switching key group

i.e. You typed aaa then you'll press backspace - key group change ocured. Snapshot should be created with aaa content.

• When you'll use navigation key like arrows, home/end to change your caret position (in order to write something elsewhere)
• not really keyboard related, but on click (very similar to navigation keys)

Special cases of snapshot

Amending selection - it occurs upon key navigation

What events are ignored?

Intentionally we ignore drop and paste events. Both of them are setting ignoreInputEvent variable, which forces logic in keyup to ignore the press.

Original implementation enhancements

• make snapshot / override selection at click - is extremely handy at mobile
• make snapshot on moving caret - useful for keyboard, feels buggy other way

Extra notes

• I've set keystrokes limit to 5 for better debugging, when code will be ready i think we should change it back to ~20 and make it configurable.

Remaining things:

deletion bug

Bug which I faced at Friday was following tc:

1. Open snapshot.html dev sample.
2. Set following selection 111 ^222 333 444 555 666 777
3. type aaa ( each char as a separate keystroke )
4. press backspace three times
5. type bbb
6. ctrl + z once

Expected result: 111 222 333 444 555 666 777

Actual: 111 b222 333 444 555 666 777

remove old type() - I haven't done it at Friday, because we need remove all variables involved, like this.modifiersCount, this.typesCount, this.wasCharacter.

Implementation details

We're relaying here on input event, which fires when printable characters are inserted, or removed. input event occures always between keydown and keyup.

With that information we can listen to events:

input - mark inputFired flag keyup - check inputFired flag, if marked then it means that some characters were inserted / deleted

In addition to that we listen to drop and paste events to set ignoreInputEvent, which tells that undesired event occured, like paste.

Exception for delete

Functional keys are handled in keydown event, which does even not have access to inputFired, but it's required to make a snapshot before content removing from dom.

Working with IE

Ie doesn't fire input event in contenteditable element, so in order to reuse functionallity we're using keypress event instead of input event. I've ported this functions to IE at Friday, and they seems to work fine, but tests are bleeding.

[Marek Lewandowski]

And as a reminder, I didn't squash commits, just so it will be it easier for me to bisect if anything would be wrong.

[Marek Lewandowski]

Pushed tests for Bender to ​t/11739_input at dev.

[Piotrek Koszuliński]

Force-pushed rebased branch.

[Piotrek Koszuliński]

I pushed many minor fixes (code style!!!).

---

Not documented and should be:

• undoM.lastKeydownImage
• undoM.strokesRecorded
• undoM.wasFunctionalKey
• undoM.onTypingStart (is it worth to make it public? it's used in one place and hardly reusable)

Should be made public and documented:

• inputFired
• ignoreInputEvent

---

undoManager.lastKeydownImage.contents === editor.getSnapshot()

Should use image.equals()

---

SaveSnapshot on instanceReady - is it really needed? Tests pass without it.

---

var functionalKey = Number( keyCode == 8 || keyCode == 46 )

WAT? :D Please make the logic clear.

---

Isn't amendSelection doing something very similar to update()? Or can't it reuse update()?

---

I would rather make strokesRecorded an object. Now you need to find its doc to understand what's kept in [ 0 ] and what's in [ 1 ].

---

undoManager.amendSelection test(s) should be in more generic place than keystrokes.js (e.g. undo/undo.js)

---

Broken scenarios:

1. Open classic editor. Click somewhere, start typing, press ctrl+z. Selection is moved to the beginning of editable. Should be at the click location (where typing started). Reproduced on Chrome and IE9.
2. Open snapshot.html sample. Place caret in the text, click bold button, start typing, press ctrl+z twice (once to undo typing and one to undo bold). Then press ctrl+z twice. Selection should end at the end of typed text, but it's put at the beginning. Reproduced on Chrome.
3. When typing pretty fast snapshots are not recorder (see screencast). Reproduced on Chrome and IE9.

[Piotrek Koszuliński]

One more thing - on every keydown you make an image. Do we really need that? AFAICT we need only one when user starts typing. Note that making images isn't a lightweight operation.

---

Would be good to move the entire contentDom listener to a separate function because it's not intermixed with a totally different logic (snapshots, modes, commands, etc).

[Marek Lewandowski]

As of given scenarios:

1. valid should be fixed
2. it's the way that it was already done, but still it's wort to give a shot
3. this one is very interesting, and I've missed that issue. I'll focus on providing a fix for that case.

[Piotrek Koszuliński]

One more scenario. On FF native image resize fires input event. But when I open editor and resize an image (do not type, do not do anything else), resize is not recorded.

[Piotrek Koszuliński]

This patch needs few more days to become mature, so I'm postponing it to 4.4.3.

[Marek Lewandowski]

One more scenario. On FF native image resize fires input event. But when I open editor and resize an image (do not type, do not do anything else), resize is not recorded.

This one seems to be kinda tricky, this is because FF fires:

• mousedown
• (mousemove ...)
• DOMAttrModified
• DOMSubtreeModified
• input
• mouseup

input does not have any interesting target / srcElement, because it points to editable element.

We might eventually listen to mouseup event and decrease inputFired like:

editable.attachListener( editable, 'mouseup', function() {

if ( inputFired > 0 ) {

inputFired --;

}

} );

But then again it feels hacky. I will leave this issue to later stage.

[Piotrek Koszuliński]

If input is followed by keyup we record it (in certain cases). If input is followed by mouseup we should too. I don't find it hacky at all.

Unless I don't understand something :D

[Marek Lewandowski]

Pushed to ​t/11739_input at dev.

[Marek Lewandowski]

Few words regarding images resize in FF - the reason why i said it seems to be hacky is that i thought we'll need to do similar logic to keyup listener in mouseup, to detect increased input counter, which might be fired between mousedown and mouseup events.

But as I was testing this concept I found out that if you resize image, and end draging "outside" of the editable (lets say that you want your image be really big), it wont fire mousedown, so still it's not reliable. Confirmed in classic editor, might be not present in inline.

[Piotrek Koszuliński]

Replying to m.lewandowski:

Few words regarding images resize in FF - the reason why i said it seems to be hacky is that i thought we'll need to do similar logic to keyup listener in mouseup, to detect increased input counter, which might be fired between mousedown and mouseup events.

But as I was testing this concept I found out that if you resize image, and end draging "outside" of the editable (lets say that you want your image be really big), it wont fire mousedown, so still it's not reliable. Confirmed in classic editor, might be not present in inline.

Ehh.. Again hit by incomplete implementation ;/. Ok, let's ignore this - it's becoming less and less important since we introduced image2.

[Piotrek Koszuliński]

Force pushed ​branch:t/11739_input rebased on master.

[Piotrek Koszuliński]

I pushed some clean up commits. Please read them, because some of the issues are recurring. I haven't done a manual review - so far I was only checking code.

1. updateSelection method does not update currentImage property.
2. updateSelection tests should be in the undo/undo.js tests, because this *should be* a general method.
3. ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L868 - you don't need bookmarks here, so create image without them.
4. ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L865 - should be extracted from this function.
5. ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L424 - should be moved to the editing handler class.
6. The editing handler class should use prototype - otherwise it's impossible to override anything in it.
7. uM.lastKeydownImage is still not documented. Additionally, it would make more sensie in editing handler class.
8. It would be good if lastKeydownImage could contain only contents, without selection. I see that the only place where it's used with its selection is [inside uM.type](​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L365-L371), but it's strange. If, as it says, we need to save snapshot for the content before change, then why don't we do that on keydown? This way we would be able to simply this and improve performance (because selection won't be needed on every keydown).
9. BTW. Isn't the code I mentioned in 8. conflicting with this: ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L845-L851
10. wasFunctionalKey is a mess. It is documented to defaults to 0, when it's undefined, it says that something "was", so it should be a boolean, but it's a number. Please clean this.

[Piotrek Koszuliński]

Oups, I've changed milestone by a mistake. I was considering moving this ticket to 4.5 because it introduces a lot of changes, but I don't think that it's necessary yet. These changes should be backward compatible because are related to private parts only.

[Marek Lewandowski]

Force pushed to ​t/11739_input at dev.

Replying to Reinmar:

I pushed some clean up commits. Please read them, because some of the issues are recurring. I haven't done a manual review - so far I was only checking code.

1. updateSelection method does not update currentImage property.

Done.

2. updateSelection tests should be in the undo/undo.js tests, because this *should be* a general method.

Done.

3. ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L868 - you don't need bookmarks here, so create image without them.

Done.

4. ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L865 - should be extracted from this function.

are you sure we want to extract it, it's really small? I don't belive it deserves for special function.

5. ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L424 - should be moved to the editing handler class.

Done.

6. The editing handler class should use prototype - otherwise it's impossible to override anything in it.

Done.

7. uM.lastKeydownImage is still not documented. Additionally, it would make more sensie in editing handler class.

Good idea, done.

8. It would be good if lastKeydownImage could contain only contents, without selection. I see that the only place where it's used with its selection is [inside uM.type](​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L365-L371), but it's strange. If, as it says, we need to save snapshot for the content before change, then why don't we do that on keydown? This way we would be able to simply this and improve performance (because selection won't be needed on every keydown).

Removing bookmark from lastKeydownImage:

Each time when key group changes, the snapshot lastKeydownImage has to ​be saved. If we'll remove bookmarks from it, it will put selection in unexpected position.

Sample TC assuming plugin has been changed not to save bookmarks for lastKeydownImage:

1. type "foobar"
2. press backspace 6 times
3. type "aa"
4. press ctrl + z

Moving to keydown listener: We can't easily move save function into keydown listener.

Its occurence is determined by keyGroupChanged, so information if keyGroup of pressed key changed. But we don't know even if this key should be handled (because we base on input event). The only one exception are navigation keys, we recognize them based on keyCode.

9. BTW. Isn't the code I mentioned in 8. conflicting with this: ​https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L845-L851
10. wasFunctionalKey is a mess. It is documented to defaults to 0, when it's undefined, it says that something "was", so it should be a boolean, but it's a number. Please clean this.

Done.

Additionally I would suggest adding config variable config.keyStrokesPerSnapshot to get rid of ​25 magic number and give developers a way to configure this.

[Piotrek Koszuliński]

• You could squash commits removing unnecessary CSS and debugger statements even before review ;)
• I don't like this ​git:833d7fd5495 for two reasons. The order of tags in docstr is incorrect and we do not use createClass if there's no reason and since there's never any reason for that, we never use it. In this case it only made the review harder for me because combined diff is less clean.
• prevKeyGroup -> previousKeyGroup
• When you have a property of type number assigning null to it is a mistake. Null means lack of object (that's why typeof null == 'object'). If you've got number as "something", then if you want to say "nothing" use -1 or 0 (depends on a case).
• Q: are you sure we want to extract it, it's really small? I don't belive it deserves for special function.

R: Yes, I'm sure - it's a const. Calculating it on every keydown is wasting resources.

[Marek Lewandowski]

Force pushed to ​t/11739_input at dev.

Replying to Reinmar:

• You could squash commits removing unnecessary CSS and debugger statements even before review ;)
• I don't like this ​git:833d7fd5495 for two reasons. The order of tags in docstr is incorrect and we do not use createClass if there's no reason and since there's never any reason for that, we never use it. In this case it only made the review harder for me because combined diff is less clean.

Done.

• prevKeyGroup -> previousKeyGroup

Done.

• When you have a property of type number assigning null to it is a mistake. Null means lack of object (that's why typeof null == 'object'). If you've got number as "something", then if you want to say "nothing" use -1 or 0 (depends on a case).

Done.

• Q: are you sure we want to extract it, it's really small? I don't belive it deserves for special function.

R: Yes, I'm sure - it's a const. Calculating it on every keydown is wasting resources.

We've discussed that on meeting, and since keyCode is involved in this condition, we'll not extract/hoist it.

[Piotrek Koszuliński]

The 4.4.3 milestone was shortened, so the remaining tickets must be postponed.

[Piotrek Koszuliński]

Ekhm? ​https://github.com/cksource/ckeditor-dev/commit/ef32188dc238d927b0db7580c613182bde6ac7a2#diff-7ea5746ca98e17925ceceb270b61b4f1R973

Second thing - update "@since 4.4.3" to 4.4.4.

Except that the code looks good now. I can't hover run tests on all browsers and perform again manual tests. Please, ask PJ for that and if he can't find any blocker, you can close this ticket and all tickets that will be automatically solved by it (remember to link to these tickets with a comment like "Fixed also: #XXX").

You can skip changelog entry. We'll generate it later.

[Piotrek Koszuliński]

A control question - why do we use input event if we have to listen to keydown and keyup anyway and we can't save a snapshot based on input only (the image resize on FF case)? Additionally, we are struggling with ignoring paste and drop events... Is there any reason why we use input event that I'm not aware of?

[Marek Lewandowski]

Input event usage was present in our conception from the very begining.

We relay on this event to determine if content really was changed. That's why we don't have to care about key codes and if content really changed (except for IE in case of backspace/del).

[Marek Lewandowski]

Replying to Reinmar:

Ekhm? ​https://github.com/cksource/ckeditor-dev/commit/ef32188dc238d927b0db7580c613182bde6ac7a2#diff-7ea5746ca98e17925ceceb270b61b4f1R973

I presume 2 things: Either you ment commented code or reverting this change in separate commit.

I've squashed commits history not to contain commit with CKEDITOR.tools.createClass usage.

Second thing - update "@since 4.4.3" to 4.4.4.

Fixed.

Pushed to ​t/11739_input at dev.

[Marek Lewandowski]

There is an issue with tests in Safari, it's caused by strict violation in other part of undo manager. Since undo strict mode was just added to undo manager, this issue just shown up.

Request for fixing this problem was issued in #12219.

Pushed quickfix branch to ​t/11739_safari_fix.

[Piotrek Koszuliński]

Fixed on master with ​git:f59504d2. It's a huge change... Kudos. This patch fixed also #11611, #12219 and most likely #10926 (waiting for confirmation).

During final phase of manual tests I found one issue: #12300.