Opened 6 years ago

Closed 6 years ago

#7900 closed Bug (fixed)

FF: Error thrown when opening table dialog on pasted cells

Reported by: James Cunningham Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.2
Component: Core : Tables Version: 3.0
Keywords: Firefox Cc: Damian, Teresa Monahan, Satya Minnekanti

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 6 years ago.
7900_2.patch (2.1 KB) - added by Garry Yao 6 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by Jakub Ś

Keywords: Firefox added; FF removed
Status: newconfirmed
Version: 3.6.1 (SVN - trunk)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 6 years ago by 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.

comment:3 Changed 6 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 6 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

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

Changed 6 years ago by Garry Yao

Attachment: 7900.patch added

comment:5 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

The dialog shows that the table has two rows.

comment:6 Changed 6 years ago by Garry Yao

Status: review_failedreview

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 6 years ago by Garry Yao

FF bug reported.

comment:8 Changed 6 years ago by Frederico Caldeira Knabben

Component: GeneralCore : Tables
Status: reviewreview_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 6 years ago by Garry Yao

Attachment: 7900_2.patch added

comment:9 Changed 6 years ago by Garry Yao

Status: review_failedreview

comment:10 Changed 6 years ago by Garry Yao

Ticket Test added:
view source.

comment:11 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 6 years ago by Garry Yao

Status: review_failedreview

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 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:14 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.2

comment:15 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7204].

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