Opened 6 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#14755 closed Bug (fixed)

[Webkit][IE8] Browser hangs when user set Number list & then insert table

Reported by: satya Owned by: m.lewandowski
Priority: Normal Milestone: CKEditor 4.6.1
Component: General Version: 4.4.4
Keywords: IBM Blink Webkit IE8 Cc: irina

Description

Test data:

 <div>aaaaaaaaaaaaaaaa</div>
  <div class="bbbbbbbbbbbbb"><br></div>

To reproduce the defect:

  1. Open nightly build, clear all editor content. set above data as editor content.
  1. Ctrl+A to select all the content.
  1. Click on "Insert/remove number list" to convert 2 lines into number list.
  1. List should be selected.
  1. Click "Table" icon to open Table Properties dialog & click OK to insert table with default values.

Issue: Browser hangs

Change History (16)

comment:1 Changed 6 months ago by j.swiderski

  • Keywords Blink Webkit added
  • Status changed from new to confirmed
  • Version changed from 4.5.10 (GitHub - master) to 4.4.4

Problem can be reproduced from CKEditor 4.4.4 in Blink and Webkit (Mac Safari) browsers.

It has started occurring when inserting a list has started creating two elements. In version 4.4.3 and below only one list item was created in step 3 and no hanging occurred.

comment:2 Changed 6 months ago by k.krzton

  • Milestone set to CKEditor 4.5.11

comment:3 Changed 5 months ago by m.lewandowski

  • Milestone changed from CKEditor 4.5.11 to CKEditor 4.6.1

comment:4 Changed 5 months ago by Tade0

If the class is removed from the second div, the error does not occur. Similarly substituting class for e.g. style also produces no error.

comment:5 Changed 5 months ago by Tade0

There's an infinite loop here: https://github.com/ckeditor/ckeditor-dev/blob/846f2a1a8f45db9b2c59538b59cbbfb1080b84ed/core/editable.js#L433 that keeps splitting the block ad infinitum.

How is this connected with having a class is unclear at the moment.

comment:6 Changed 5 months ago by Tade0

One important difference is that in this line: https://github.com/ckeditor/ckeditor-dev/blob/846f2a1a8f45db9b2c59538b59cbbfb1080b84ed/core/editable.js#L419 the contents don't get deleted the same way when the second element has a class, and this causes the previously mentioned infinite loop.

comment:7 Changed 5 months ago by Tade0

The error is produced as follows:

Right before its occurrence the selection looks like this:

<ol>
  <li>[aaaaaaaaaaaaaaaa</li>
  <li class="bbbbbbbbbbbbb">]<br></li>
</ol>

After range.deleteContents( 1 ); is called the selection looks like this:

<ol>
  <li></li>[]
  <li class="bbbbbbbbbbbbb"></li>
</ol>

Both the range startContainer and endContainer are the <ol> element. Such a selection is not expected, and therefore causes the while loop that follows to get into a state of adding a paragraph/div infinitely.

comment:8 Changed 8 weeks ago by m.lewandowski

Pushed tests to t/14755 .

comment:9 Changed 7 weeks ago by m.lewandowski

  • Owner set to m.lewandowski
  • Status changed from confirmed to assigned

comment:10 Changed 7 weeks ago by m.lewandowski

  • Status changed from assigned to review

Pushed to t/14755.

comment:11 Changed 7 weeks ago by k.krzton

  • Keywords IE8 added
  • Status changed from review to review_failed

LGTM!

Rechecked exactly and this issue occurs on WebKit and Blink browsers and also IE8 browser. There is no need to specify additional if's only for those browsers as the issue happens for the specific selection only present on this browsers for this particular case. The fix operates on this selection so it is well-tailored for this issue.

Just one thing to discuss: There is a small inconsistency, between native behavior and the fix. On browsers without the issue, inserted table replaces the whole list. The fix works in a way that table is inserted inside first list item. While this is an edge case I would say that behavior is fine. However, I would like to ask @m.lewandowski if it was done on purpose or maybe you just have not considered different approach? Is it acceptable from you perspective or we should be 100% consistent with native behaviour?

Last edited 7 weeks ago by k.krzton (previous) (diff)

comment:12 Changed 7 weeks ago by m.lewandowski

  • Status changed from review_failed to review

So here the scope was to prevent the browser from hanging. Also the fix had to be consistent with current CKEditor behavior, when you do the same operation on bare li element, without class attribute - and that's achieved.

The end-user behavior is a matter for other issue, as this is a broader subject. Because of doubts I added a note in the manual test.

comment:13 Changed 7 weeks ago by k.krzton

  • Status changed from review to review_passed

Thanks for the explanation @m.lewandowski. Fair enough.

comment:14 Changed 7 weeks ago by k.krzton

  • Resolution set to fixed
  • Status changed from review_passed to closed

Merged to master with 70e047f.

comment:15 Changed 7 weeks ago by m.lewandowski

  • Summary changed from Chrome: Browser hangs when user set Number list & then insert table to [Webkit,IE8] Browser hangs when user set Number list & then insert table

Updated the issue title to match recent findings.

comment:16 Changed 7 weeks ago by m.lewandowski

  • Summary changed from [Webkit,IE8] Browser hangs when user set Number list & then insert table to [Webkit][IE8] Browser hangs when user set Number list & then insert table
Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy