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

[ben]

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

[Piotrek Koszuliński]

Confirmed. We'll fix it in 4.3.1.

[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

[Piotrek Koszuliński]

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.

[Olek Nowodziński]

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

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

[Piotrek Koszuliński]

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).

[Olek Nowodziński]

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>

[Piotrek Koszuliński]

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>.

[Piotrek Koszuliński]

Older duplicate closed: #10652.

[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.

[Piotrek Koszuliński]

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.

[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.

[Piotrek Koszuliński]

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