Opened 13 months ago

Closed 9 months ago

Last modified 9 months ago

#14755 closed Bug (fixed)

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

Reported by: Satya Minnekanti Owned by: Marek 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 13 months ago by Jakub Ś

Keywords: Blink Webkit added
Status: newconfirmed
Version: 4.5.10 (GitHub - master)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 13 months ago by kkrzton

Milestone: CKEditor 4.5.11

comment:3 Changed 12 months ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:4 Changed 12 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 12 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 12 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 12 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 9 months ago by Marek Lewandowski

Pushed tests to t/14755 .

comment:9 Changed 9 months ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:10 Changed 9 months ago by Marek Lewandowski

Status: assignedreview

Pushed to t/14755.

comment:11 Changed 9 months ago by kkrzton

Keywords: IE8 added
Status: reviewreview_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 9 months ago by kkrzton (previous) (diff)

comment:12 Changed 9 months ago by Marek Lewandowski

Status: review_failedreview

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 9 months ago by kkrzton

Status: reviewreview_passed

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

comment:14 Changed 9 months ago by kkrzton

Resolution: fixed
Status: review_passedclosed

Merged to master with 70e047f.

comment:15 Changed 9 months ago by Marek Lewandowski

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

Updated the issue title to match recent findings.

comment:16 Changed 9 months ago by Marek Lewandowski

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