[IE 9...10] Error on removing image from editor.

[Artur Delura]

Browser: [IE 9...10] Since: 4.4.4 ​http://presets.ckeditor.dev/4.4.4/standard/ckeditor/samples/replacebyclass.html

1. Open editor with following content:

   <h1><img src="assets/sample.jpg" />Apollo 11</h1>
   

2. Apply follwoing selection:

   <h1>[<img src="assets/sample.jpg" />]Apollo 11</h1>
   

3. Press backspace button to remove image.

Actual result: Error is throw in console:

SCRIPT5007: Unable to get value of the property 'equalsContent': object is null or undefined 
ckeditor.js, line 993 character 30

[Artur Delura]

Changes on ​branch:t/12327. Tests needed.

[Marek Lewandowski]

Hmm logically lastKeydownImage must be present in keyup event, because it's saved before.

It looks like undoManager.onKeydown ( ​attached here ) was not called for some reason. Unfortunately I need to leave office now, but eventually we might check why it's not called.

[Piotrek Koszuliński]

Keydown can be simply cancelled or propagation of the native event might be stopped. Undo manager must take this into consideration. The meaning is that if event was somehow blocked, its listener should have handled it, so typing manager doesn't have to do that. This seems to make Artur's solution correct. However, such sequence is possible:

• keydown (not blocked, so lastKeydown is set)
• keyup (uses lastKeydown)
• keydown (blocked, so lastKeydown isn't changed)
• keyup (uses lastKeydown, but from 1st keydown, not 2nd)

If I'm right that this is possible, then keyup's listener must reset lastKeydown flag or it must he reset in some other way. Please create a new ticket for this issue if it's real.

[Piotrek Koszuliński]

Another thing - to block a possibility that undo manager's keydown listener is executed before or after some other listener that may cancel it, undo manager should use low priority listeners in order to always listen at the end. This, of course, concerns also other listeners. Separate ticket should be reported for this issue as well.

[Marek Lewandowski]

I agree that undoManager should work gracefully when keydown event was canceled.

Reason for the issue: there seems to be a major code branching for handling backspace (in this case with selected element).

More-less what's happening now is:

1. Editable registers backspace key to ​keystroke handler as a blocked key.
2. KeystrokeHandler fires key event.
3. Event gets canceled ​in custom delete handling for <IE11 by ​returning {{{false}}}
4. Execution goes back to keystrokeHandler which prevents + cancels the event.

#10555 looks like might be related

[Marek Lewandowski]

The simple solution without messing with keystrokeHandler/editable is to simply lower a priority of keydown listener in undoManager.

In addition to that we should merge extra check ​added by Artur, since developer might also cancel some events for whatever reason, with low priority.

Though this change might introduce some weirdness in case multiple keys presser or sth, I need to investigate it a little further.

[Marek Lewandowski]

Alright, ready for review:

Branch contains two changes, both of them would result with fixing mentioned issue:

• An extra check introduced in ​git:cb0abf9304922.
• Lowered keyboard ​listeners priorities, so devs won't be able to unintentionally cause troubles / disable undo.

And a TC for source of that issue (IE specific).

Pushed to ​t/12327 at dev.

[Piotrek Koszuliński]

Lowered keyboard ​listeners priorities, so devs won't be able to unintentionally cause troubles / disable undo.

In comment:11 I asked to report a separate ticket for this change. It may have unpredictable implications, so it's not the right moment. Now, before release, our only goal is to prevent error.

[Marek Lewandowski]

Pushed to ​t/12327 at dev.

A follow-up ticket was created: #12332.

[Olek Nowodziński]

The patch seems to be alright. There are some issues with the code though:

1. ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/undo/integrations.js#L15 and ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/undo/integrations.js#L36 are not coherent.
2. Why restricted to IE only when it passes on FF and Chrome anyway? I wouldn't exclude any browser.
3. ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/undo/integrations.js#L37-L40 wrong code style of comments. Also we usually refer to the ticket instead of explanation in such case (i.e. ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/widget/widgetapi.js#L695).
4. ​https://github.com/cksource/ckeditor-dev/commit/b018a72033d7f2312e2d4962f01ea2e370bb5f05#diff-7ea5746ca98e17925ceceb270b61b4f1R902 also deserves a reference to the ticket.
5. We agreed that the summary of a commit that touches tests/* should match

   "Tests: Foo bar."

   pattern – a rewrite of the message is needed.
6. ​https://github.com/cksource/ckeditor-dev/commit/cbebd8e3d5a812b60e644743b2d819eee69056d2#diff-90963d012c2c22586a947837f8f728bfR8 what does startupData change in this file? IMO it's totally obsolete. bender.editor = true; would be enough.
7. Use bender-include ​https://github.com/benderjs/benderjs/issues/52 instead of separate HTML file.

[Marek Lewandowski]

Replying to a.nowodzinski:

The patch seems to be alright. There are some issues with the code though:

1. ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/undo/integrations.js#L15 and ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/undo/integrations.js#L36 are not coherent.
2. Why restricted to IE only when it passes on FF and Chrome anyway? I wouldn't exclude any browser.
3. ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/undo/integrations.js#L37-L40 wrong code style of comments. Also we usually refer to the ticket instead of explanation in such case (i.e. ​https://github.com/cksource/ckeditor-dev/blob/cbebd8e3d5a812b60e644743b2d819eee69056d2/tests/plugins/widget/widgetapi.js#L695).

Good point.

4. ​https://github.com/cksource/ckeditor-dev/commit/b018a72033d7f2312e2d4962f01ea2e370bb5f05#diff-7ea5746ca98e17925ceceb270b61b4f1R902 also deserves a reference to the ticket.
5. We agreed that the summary of a commit that touches tests/* should match

   "Tests: Foo bar."

   pattern – a rewrite of the message is needed.
6. ​https://github.com/cksource/ckeditor-dev/commit/cbebd8e3d5a812b60e644743b2d819eee69056d2#diff-90963d012c2c22586a947837f8f728bfR8 what does startupData change in this file? IMO it's totally obsolete. bender.editor = true; would be enough.
7. Use bender-include ​https://github.com/benderjs/benderjs/issues/52 instead of separate HTML file.

Cool, I didn't know about that feature.

All the fixes done in two commits. I left them unsquashed, so it will easier for you to read changes.

Pushed to ​t/12327 at dev.

[Olek Nowodziński]

Fixed in master (​git:1ba5105).