#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 ) ¶
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:
Change History (37)
comment:1 Changed 11 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
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
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
Owner: | set to Marek Lewandowski |
---|---|
Status: | confirmed → assigned |
comment:6 Changed 11 years ago by
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
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
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:
- Open snapshot.html dev sample.
- Set following selection
111 ^222 333 444 555 666 777
- type
aaa
( each char as a separate keystroke ) - press backspace three times
- type
bbb
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
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
Status: | assigned → review |
---|
Pushed tests for Bender to t/11739_input at dev.
comment:12 Changed 11 years ago by
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:
- 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.
- 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.
- When typing pretty fast snapshots are not recorder (see screencast). Reproduced on Chrome and IE9.
Changed 11 years ago by
Attachment: | typingfast.webm added |
---|
comment:13 Changed 11 years ago by
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
As of given scenarios:
- valid should be fixed
- it's the way that it was already done, but still it's wort to give a shot
- this one is very interesting, and I've missed that issue. I'll focus on providing a fix for that case.
comment:15 Changed 11 years ago by
Status: | review → review_failed |
---|
comment:16 Changed 11 years ago by
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.
comment:17 Changed 11 years ago by
Milestone: | CKEditor 4.4.2 → CKEditor 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
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
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
comment:21 follow-up: 22 Changed 11 years ago by
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 Changed 11 years ago by
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 betweenmousedown
andmouseup
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:24 follow-up: 26 Changed 11 years ago by
Milestone: | CKEditor 4.4.3 → CKEditor 4.5.0 |
---|---|
Status: | review → review_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.
updateSelection
method does not updatecurrentImage
property.updateSelection
tests should be in the undo/undo.js tests, because this *should be* a general method.- https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L868 - you don't need bookmarks here, so create image without them.
- https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L865 - should be extracted from this function.
- https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L424 - should be moved to the editing handler class.
- The editing handler class should use prototype - otherwise it's impossible to override anything in it.
uM.lastKeydownImage
is still not documented. Additionally, it would make more sensie in editing handler class.- 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 [insideuM.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). - 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
wasFunctionalKey
is a mess. It is documented to defaults to0
, when it'sundefined
, 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
Milestone: | CKEditor 4.5.0 → CKEditor 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 Changed 10 years ago by
Status: | review_failed → review |
---|
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.
updateSelection
method does not updatecurrentImage
property.
Done.
updateSelection
tests should be in the undo/undo.js tests, because this *should be* a general method.
Done.
- 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.
- 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.
- https://github.com/cksource/ckeditor-dev/blob/e2a2ab6f89452be41b4b943fef7202368bca641d/plugins/undo/plugin.js#L424 - should be moved to the editing handler class.
Done.
- The editing handler class should use prototype - otherwise it's impossible to override anything in it.
Done.
uM.lastKeydownImage
is still not documented. Additionally, it would make more sensie in editing handler class.
Good idea, done.
- 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 [insideuM.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
:
- type "foobar"
- press backspace 6 times
- type "aa"
- 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
.
- 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
wasFunctionalKey
is a mess. It is documented to defaults to0
, when it'sundefined
, 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 follow-up: 28 Changed 10 years ago by
Status: | review → review_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 whytypeof null == 'object'
). If you've got number as "something", then if you want to say "nothing" use-1
or0
(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 Changed 10 years ago by
Status: | review_failed → review |
---|
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 whytypeof null == 'object'
). If you've got number as "something", then if you want to say "nothing" use-1
or0
(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
Milestone: | CKEditor 4.4.3 → CKEditor 4.4.4 |
---|
The 4.4.3 milestone was shortened, so the remaining tickets must be postponed.
comment:30 follow-up: 34 Changed 10 years ago by
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
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
Description: | modified (diff) |
---|
comment:33 Changed 10 years ago by
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).
comment:34 Changed 10 years ago by
Replying to Reinmar:
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
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
Resolution: | → fixed |
---|---|
Status: | review → closed |
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.
cc