Opened 6 years ago

Closed 6 years ago

#11183 closed Bug (fixed)

Insert Horizontal Line (horizontal rule) into multiple rows causes browser crash

Reported by: ben Owned by: Piotrek Koszuliński
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.3.1
Component: General Version:
Keywords: Cc:

Description

Can be reproduced on demo page.

To Reproduce:

  • Insert table with 3 rows and 2 columns (exact number not important, as long as there are enough rows to select multiple by dragging)
  • Highlight multiple rows and cells by clicking in one cell and dragging the mouse
  • Click the Insert Horizontal Line button

Notice that the browser is now frozen. The browser did NOT freeze up in version 3.6.5

Change History (14)

comment:1 Changed 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.3.1
Priority: NormalHigh
Status: newconfirmed
Version: 4.3

Confirmed. We'll fix it in 4.3.1.

comment:2 Changed 6 years ago by Jakub Ś

Browser crash can be reproduced in every browser from CKEditor 4.0 beta.

In CKEditor 3.x error was thrown:
TypeError: this.$ is undefined
code: this.$.appendChild( node.$ );
Line: 304
URI: element.js

comment:3 Changed 6 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:4 Changed 6 years ago by Piotrek Koszuliński

Status: assignedreview

I pushed t/11183 on dev and tests.

I fixed only the range position inside table after deleteContents(). I wasn't focused on fixing the final table structure, because it would be extremely hard and in fact it should be somehow handled by table plugin (stuff like headers, scopes, colspans, etc.).

So - there should be no crashes any more, but table structure will be incorrect from UX POV. Fortunately, inserting elements into rows selection is an edge case.

PS. Results on FF are funny, but this is how it worked.

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

Status: reviewreview_failed
walker.evaluator = function( node ) {
	return node.type == CKEDITOR.NODE_ELEMENT;
};

is obsolete, since walker.guard doesn't consider other types of nodes anyway.

comment:6 Changed 6 years ago by Piotrek Koszuliński

Status: review_failedreview

That evaluator is required. Chrome will throw an error without it e.g. when inserting line into two selected rows (2nd and 3rd out of 4).

comment:7 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_failed

IE8:

Test named "GS. insertions into table - block" failed with message: "collapsed (editable: "body" & mode: "insertElement")". Expected:

<table><tbody><tr><td>x<hr />^y</td></tr></tbody></table>

Actual:

<table><tbody><tr><td>x<hr /> ^y</td></tr></tbody></table>

comment:8 Changed 6 years ago by Piotrek Koszuliński

Status: review_failedassigned

Uh... IE8 doesn't like me.

  1. Select two rows (2nd and 3rd) from the table.
  2. Insert HR - it will be inserted in the first column.
  3. Click the right cell (which is empty) and start typing - my IE8 crashes completely.

I wanted to ignore the fact that we leave non-filled cells after insertion, but I guess I'll fix this, because the most probable reason for crash is an empty text node inside <td>.

comment:9 Changed 6 years ago by Piotrek Koszuliński

Older duplicate closed: #10652.

comment:10 Changed 6 years ago by Piotrek Koszuliński

Another lovely TC (FF only):

  1. Select multiple rows.
  2. Insert table.
  3. Error is thrown.

Why?

  1. Ranges contains table cells.
  2. We're inserting table into first cell what, for some reason causes other cells (or maybe just the adjacent one) to be removed.
  3. Then we take second range, which contains a table cell which was removed and we insert table there.
  4. At the end we get the last inserted element and try to set selection inside it. Unfortunately, that element's parent is detached (step 2), so boom!

Since insertHtml ignores multirange selections and inserts content only in the first range and no one has ever complained about this I propose to do the same in insertElement because:

  • we'll avoid such edge cases like this,
  • inserting something into rows/cols selection is a rare case and it's hard to say what's the expected result,
  • it's easier,
  • insertHtml works this way,
  • we have to clone element to insert it into multiple ranges, so unexpectedly the reference is lost - inserting widget into such selection would result in only one of them initialized.
Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:11 Changed 6 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/11183 again. Fixed issues mentioned in comment:8 and comment:10. As I mentioned earlier - result of insertion is far from perfect, but at least there are no crashes or errors thrown now. Not critical issues following this ticket should be extracted to separate tickets.

comment:12 Changed 6 years ago by Piotrek Koszuliński

PS. There are no tests because we don't have an architecture for good table tests. Neither for multirange selection nor even for normal selections.

comment:13 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:14 Changed 6 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:ed52709 on dev and 4485821 on tests.

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