We are migrating CKEditor issue tracking to GitHub. Please, use GitHub to report any new issues.

The former tracking system (this website) will still be available in the read-only mode. All issues reported in the past will still be available publicly and can be referenced.

Important: we decided not to transfer all the tickets to GitHub, as many of them are not reproducible anymore or simply no longer requested by the community. If the issue you are interested in, can be still reproduced in the latest version of CKEditor, feel free to report it again on GitHub. At the same time please note that issues reported on this website are still taken into consideration when picking up candidates for next milestones.

Change History (37)

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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").

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?