Opened 5 years ago

Closed 5 years ago

#7900 closed Bug (fixed)

FF: Error thrown when opening table dialog on pasted cells

Reported by: jamescun Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.2
Component: Core : Tables Version: 3.0
Keywords: Firefox Cc: damo, tmonahan, satya

Description

Steps to reproduce the defect:

  1. Open the Ajax sample in FF.
  1. Insert a table.
  1. Enter some text into the cells on the top row.
  1. Copy the cells containing the text.
  1. Place the cursor below the table and paste the copied cells into the editor.
  1. 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.
  1. 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

Attachments (2)

7900.patch (740 bytes) - added by garry.yao 5 years ago.
7900_2.patch (2.1 KB) - added by garry.yao 5 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by j.swiderski

  • Keywords Firefox added; FF removed
  • Status changed from new to confirmed
  • Version changed from 3.6.1 (SVN - trunk) to 3.0

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

comment:2 Changed 5 years ago by wwalc

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.

comment:3 Changed 5 years ago by 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

comment:4 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to review

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

Changed 5 years ago by garry.yao

comment:5 Changed 5 years ago by Saare

  • Status changed from review to review_failed

The dialog shows that the table has two rows.

comment:6 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

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.

comment:7 Changed 5 years ago by garry.yao

FF bug reported.

comment:8 Changed 5 years ago by fredck

  • Component changed from General to Core : Tables
  • Status changed from review to review_failed

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.

Changed 5 years ago by garry.yao

comment:9 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

comment:10 Changed 5 years ago by garry.yao

Ticket Test added:
view source.

comment:11 Changed 5 years ago by fredck

  • Status changed from review to review_failed

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.

comment:12 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

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.

comment:13 Changed 5 years ago by fredck

  • Status changed from review to review_passed

comment:14 Changed 5 years ago by fredck

  • Milestone set to CKEditor 3.6.2

comment:15 Changed 5 years ago by garry.yao

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

Fixed with [7204].

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