Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#11739 closed Task (fixed)

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

Reported by: Piotrek Koszuliński Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.4.4
Component: General Version:
Keywords: Cc:

Description (last modified by Marek Lewandowski)

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:

Find this issue on GitHub

Change History (37)

comment:1 Changed 11 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 11 years ago by Olek Nowodziński

cc

comment:3 Changed 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.2

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.

comment:4 Changed 11 years ago by 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.

comment:5 Changed 11 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:6 Changed 11 years ago by 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.

comment:7 Changed 11 years ago by 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.

comment:8 Changed 11 years ago by 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.

comment:9 Changed 11 years ago by 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.

comment:10 Changed 11 years ago by Marek Lewandowski

Status: assignedreview

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

comment:11 Changed 11 years ago by Piotrek Koszuliński

Force-pushed rebased branch.

comment:12 Changed 11 years ago by 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.

Changed 11 years ago by Piotrek Koszuliński

Attachment: typingfast.webm added

comment:13 Changed 11 years ago by 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).

comment:14 Changed 11 years ago by 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.
Last edited 11 years ago by Marek Lewandowski (previous) (diff)

comment:15 Changed 11 years ago by Piotrek Koszuliński

Status: reviewreview_failed

comment:16 Changed 11 years ago by 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.

Last edited 11 years ago by Piotrek Koszuliński (previous) (diff)

comment:17 Changed 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.2CKEditor 4.4.3

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

comment:18 Changed 11 years ago by 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.

comment:19 Changed 11 years ago by 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

Last edited 11 years ago by Piotrek Koszuliński (previous) (diff)

comment:20 Changed 11 years ago by Marek Lewandowski

Status: review_failedreview

Pushed to t/11739_input at dev.

comment:21 Changed 11 years ago by 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.

comment:22 in reply to:  21 Changed 11 years ago by 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.

comment:23 Changed 11 years ago by Piotrek Koszuliński

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

comment:24 Changed 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.3CKEditor 4.5.0
Status: reviewreview_failed

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.

comment:25 Changed 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.0CKEditor 4.4.3

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.

comment:26 in reply to:  24 Changed 10 years ago by Marek Lewandowski

Status: review_failedreview

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.

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

Done.

  1. 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.

  1. 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.

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

Done.

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

Done.

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

Good idea, done.

  1. 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.

  1. 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
  2. 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.

comment:27 Changed 10 years ago by Piotrek Koszuliński

Status: reviewreview_failed
  • 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.

comment:28 in reply to:  27 Changed 10 years ago by Marek Lewandowski

Status: review_failedreview

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.

comment:29 Changed 10 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.3CKEditor 4.4.4

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

comment:30 Changed 10 years ago by 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.

comment:31 Changed 10 years ago by 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?

comment:32 Changed 10 years ago by Marek Lewandowski

Description: modified (diff)

comment:33 Changed 10 years ago by 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).

Last edited 10 years ago by Marek Lewandowski (previous) (diff)

comment:34 in reply to:  30 Changed 10 years ago by 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.

comment:35 Changed 10 years ago by 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.

comment:36 Changed 10 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

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.

Last edited 10 years ago by Piotrek Koszuliński (previous) (diff)
Find this issue on GitHub
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy