Opened 5 years ago

Closed 5 years ago

#12327 closed Bug (fixed)

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

Reported by: Artur Delura Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.4.4
Component: General Version:
Keywords: Cc:

Description (last modified by Marek Lewandowski)

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

Change History (20)

comment:1 Changed 5 years ago by Artur Delura

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

comment:2 Changed 5 years ago by Artur Delura

Description: modified (diff)

comment:3 Changed 5 years ago by Artur Delura

Description: modified (diff)

comment:4 Changed 5 years ago by Marek Lewandowski

Status: newconfirmed

comment:5 Changed 5 years ago by Marek Lewandowski

Description: modified (diff)

comment:6 Changed 5 years ago by Marek Lewandowski

Description: modified (diff)

comment:7 Changed 5 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:8 Changed 5 years ago by Artur Delura

Status: assignedreview

Changes on branch:t/12327. Tests needed.

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

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

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

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

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

comment:14 Changed 5 years ago by Marek Lewandowski

Alright, ready for review:

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

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

Pushed to t/12327 at dev.

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

comment:16 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

comment:17 Changed 5 years ago by Marek Lewandowski

Owner: changed from Artur Delura to Marek Lewandowski
Status: review_failedreview

Pushed to t/12327 at dev.

A follow-up ticket was created: #12332.

comment:18 Changed 5 years ago by Olek Nowodziński

Status: reviewreview_failed

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.

comment:19 in reply to:  18 Changed 5 years ago by Marek Lewandowski

Status: review_failedreview

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.

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

    "Tests: Foo bar."

    pattern – a rewrite of the message is needed.
  3. 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.
  4. 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.

comment:20 Changed 5 years ago by Olek Nowodziński

Milestone: CKEditor 4.4.5CKEditor 4.4.4
Resolution: fixed
Status: reviewclosed

Fixed in master (git:1ba5105).

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy