Ticket #8675 (closed Bug: fixed)

Opened 3 years ago

Last modified 22 months ago

Deleting cells in nested table removes outer table cell.

Reported by: hjdivad Owned by: a.nowodzinski
Priority: Normal Milestone: CKEditor 4.0.1
Component: Core : Tables Version: 3.6.1
Keywords: HasPatch Cc:

Description

STEPS TO REPRODUCE

  1. Use either Chrome or Firefox. Bug is likely reproducible in IE as well, but I haven't confirmed this.
  2. Navigate to a 3.6.2 cke sample page. Presently, http://ckeditor.com/demo exhibits this error.
  3. Clear any existing content in the editor.
  4. Create a table with 3 rows and 2 columns (OUTER).
  5. In the top left cell, create a nested table with 3 rows and 2 columns (INNER).
  6. In the INNER table, select the top two cells.
  7. Right click to get the context menu and select cell -> delete cells.

EXPECTED RESULT

Only the top two cells of the INNER table are deleted.

ACTUAL RESULT

The top left cell of the OUTER table is removed, thus removing the entire INNER table.

MORE INFO

Please see related ticket #6289. This seems to be a regression from sometime after 3.4.2. Note that #6289 was reported as an IE-only issue, but this ticket affects FF and Chrome (and likely IE as well).

DIAGNOSIS

The two bits of relevant code are in the following files:

http://dev.ckeditor.com/browser/CKEditor/trunk/_source/plugins/tabletools/plugin.js http://dev.ckeditor.com/browser/CKEditor/trunk/_source/core/dom/walker.js

In tabletools, the function getSelectedCells is of interest, especially lines 63-68.

54	while ( ( node = walker.next() ) )
55	{
56	  // If may be possible for us to have a range like this:
57	  // <td>^1</td><td>^2</td>
58	  // The 2nd td shouldn't be included.
59	  //
60	  // So we have to take care to include a td we've entered only when we've
61	  // walked into its children.
62	
63	  var parent = node.getAscendant( 'td' ) || node.getAscendant( 'th' );
64	  if ( parent && !parent.getCustomData( 'selected_cell' ) )
65	  {
66	    CKEDITOR.dom.element.setMarker( database, parent, 'selected_cell', true );
67	    retval.push( parent );
68	  }
69	}

This loop examines each node from the range start to the range end, according to walker.js. I am not entirely clear on the semantics of walker.js so I cannot say for sure whether it is correct, but from tracing it seems to be doing something reasonable.

When one selects two cells, the relevant portion of the DOM looks something like this:

TABLE (outer)
  TR
    TD (call this cell OA)
      BR
      TABLE (inner)
        TR
          TD (call this cell IA)
            BR  (call this node IABR)
          TD (call this cell IB)
            BR  (call this node IBBR)
        TR
          TD (call this cell IC)
            BR  (call this node ICBR)
          TD
            BR
        TR
          TD
            BR
          TD
            BR
    TD
       BR
   TR
     TD
       BR
     TD
       BR
   TR
     TD
       BR
     TD
       BR

When we select the top two cells of the INNER table our range goes from IABR to IBBR. It is also very easy to accidentally select the first cell of the next row, resulting in a range that goes from IABR to ICBR. The walker considers these nodes, but also the table cells IA, IC and their parent TR.

Note that line 63 fails to pass true (i.e. includeself) to getAscendant, and so erroneously includes cell OA in retval.

PROGNOSIS/PATCH

Please find attached a patch for this problem. This patch does two things.

  1. Passes includeself for the two calls to getAscendant.
  2. Ignores a number of table related nodes (TABLE TR TBODY TFOOT THEAD). If the walker were to consider a TBODY element in a nested table, I think the same behaviour would happen without this check.

This patch was made against cke 3.6.2.

OTHER

If anything is unclear about the issue or patch, please do not hesitate to email me at either dhamilton at verticalresponse.com or cke at hjdivad.com. Thank you in advance for your time.

Attachments

cke.nested.table.patch (1.2 KB) - added by hjdivad 3 years ago.
8675.patch (1.0 KB) - added by j.swiderski 3 years ago.
This one should be easier to apply

Change History

Changed 3 years ago by hjdivad

comment:1 Changed 3 years ago by j.swiderski

  • Keywords haspatch removed
  • Status changed from new to confirmed
  • Version changed from 3.6.2 to 3.6.1

Reproducible from CKEditor 3.6.1 rev [6918]

To reproduce either insert table at the end of content or clear contents and insert table.

NOTE:IE9 has problems with inserting nested tables in first cell: #8769

comment:2 Changed 3 years ago by hjdivad

@j.swiderski, why did you drop the haspatch keyword? Do you have some concerns with the patch that I could address?

comment:3 Changed 3 years ago by j.swiderski

  • Keywords HasPatch added

By mistake. Sorry.

Changed 3 years ago by j.swiderski

This one should be easier to apply

comment:4 Changed 3 years ago by hjdivad

No worries; just wanted to fixup the patch if it was needed. Thanks for looking at the issue. _

comment:5 Changed 3 years ago by j.swiderski

I had some problems with running tha patch for the first time but after refreshing the cache patch worked as expected.

comment:6 Changed 22 months ago by hjdivad

@j.swiderski what's preventing this patch from making it upstream? Is there something I can do to help it along?

comment:7 Changed 22 months ago by j.swiderski

@hjdivad the fix looks correct and it looks like the same problem can be reproduced on CKEDitor 4.0.

To get your proposed fix tested and applied (if it is correct) please fork CKEditor 4 code from https://github.com/ckeditor/ckeditor-dev, prepare your fix and make a "pull request" (here is the example list of pull request). In comment please specify link to bug for which this fix is.
This is much faster way to get proposed fix implemented/rejected (if they are invalid). At least in V4.
In CKEditor 3.x we go through proposed SVN patches every couple of weeks (sometimes moths) and decide whether to apply them or not. This of course takes more time then pull requests on github but this is what versioning system offered and we had to adjust to it.

comment:9 Changed 22 months ago by Reinmar

  • Milestone set to CKEditor 4.0.1

comment:10 Changed 22 months ago by a.nowodzinski

  • Status changed from confirmed to assigned
  • Owner set to a.nowodzinski

comment:11 Changed 22 months ago by a.nowodzinski

  • Status changed from assigned to review

comment:12 Changed 22 months ago by Reinmar

  • Status changed from review to review_passed

comment:13 follow-up: ↓ 14 Changed 22 months ago by Reinmar

Tabletools plugin needs refac, because at least code of this method may be optimized, but this requires new ticket.

comment:14 in reply to: ↑ 13 Changed 22 months ago by a.nowodzinski

Replying to Reinmar:

Tabletools plugin needs refac, because at least code of this method may be optimized, but this requires new ticket.

Extracted #9813 with this idea.

comment:15 Changed 22 months ago by a.nowodzinski

Rebased branches.

comment:16 Changed 22 months ago by a.nowodzinski

  • Status changed from review_passed to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy