FF: Error thrown when opening table dialog on pasted cells

[James Cunningham]

Steps to reproduce the defect:

1. Open the Ajax sample in FF.

2. Insert a table.

3. Enter some text into the cells on the top row.

4. Copy the cells containing the text.

5. Place the cursor below the table and paste the copied cells into the editor.

6. Right click on the pasted cells and open the table dialog from the context menu. See that the rows and columns fields are editable and the columns field is left empty.

7. Enter a value into the columns field and click ok.

Result: The table dialog remains on the screen. If you check in Firebug you can see that the following error is thrown:

this.$ is undefined
j.insertNode(l.remove());k.insertAfter...rn(this.getName=function(){return i;
ckeditor.js (line 16)

This is reproducible in FF 3.6.8 and 3.6.15

[Jakub Ś]

True for FF (3.6 & 4) browsers from CKEditor 3.0.

The JS error is:
selectedTable.$.rows[row].cells[0] is undefined
/ckeditor/_source/plugins/table/dialogs/table.js
Line 352
It shows when you choose “Table properties” from context menu.

When you enter a value into the columns field and click “OK” another error pops out:
this.$ is undefined
/ckeditor/_source/core/dom/element.js
Line 636

[Wiktor Walc]

When copying&pasting a table row, Firefox creates a table with nested <tr> element (reported in ​https://bugzilla.mozilla.org/show_bug.cgi?id=529955)

<table cellspacing="1" cellpadding="1" border="1" _moz_dirty="">
<tbody>
    <tr>
        <tr>
             <td><br></td>
        </tr>
    </tr>
</tbody>
</table>

CKEditor transforms it later into:

<table border="1" cellpadding="1" cellspacing="1">
	<tbody>
		<tr>
		</tr>
		<tr>
			<td>
				&nbsp;</td>
		</tr>
	</tbody>
</table>

and reports various errors when trying to work with such a table, as described above.

[typeof]

My proposition is to fix the data from clipboard just before firing 'paste' event, i.e. just before executing this code:

// clipboard\plugin.js, lines 395-397
var dataTransfer = {};
dataTransfer[ eventData.mode ] = data;
editor.fire( 'paste', dataTransfer );

A pseudocode for the fix would be:

if (we are running in Firefox with this bug)
  if (data starts with '<table' and contains '<tr><tr>') {
    replace all '<tr><tr>' with '<tr>'
    replace all '</tr></tr>' with '</tr>'

I checked the brute-force ;) implementation:

data = data.replace('<tr><tr>', '<tr>');
data = data.replace('</tr></tr>', '</tr>');

and it seems to work fine

[Garry Yao]

The fix would be as simple as making table dialog defensive to empty table row.

[Sa'ar Zac Elias]

The dialog shows that the table has two rows.

[Garry Yao]

Empty row must NOT be ignored, e.g. with the following table, structure changed without having the second TR.

<table>
					<tr>
						<td rowspan=3>cell1</td>
						<td rowspan=2>cell2</td>
					</tr>
					<tr></tr>
					<tr><td>cell3</td></tr>
				</table>

Here it's enough to just eliminate the error.

[Garry Yao]

FF bug ​reported.

[Frederico Caldeira Knabben]

While the patch avoids it throwing a js error, the columns field remain empty in the dialog (due to the fact of the first <tr> being empty) and it's not possible to save changes made on it (e.g. trying to change the border size).

It looks like some parts of the calculation must look for non empty rows, especially considering that empty row is not visible.

[Garry Yao]

Ticket Test added:
​view source.

[Frederico Caldeira Knabben]

The patch contains an (probably) unrelated change to showborders. Can you confirm it's there by mistake?

In such case, go ahead committing it without that change. Otherwise, please just give an explanation for it.

[Garry Yao]

Otherwise, please just give an explanation for it.

It's by intention, the showborder plugin is not working well when changing border from the dialog, it can be confirmed by your tc.

[Garry Yao]

Fixed with [7204].